Skip to content

[React] Don't track signals read during SSR render #640

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
Jan 9, 2025
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dirty-poets-cover.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@preact/signals-react": patch
---

Don't track signals read during an SSR render (e.g. renderToString, renderToStaticMarkup)
4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,8 +26,8 @@
"test:karma:watch": "karma start karma.conf.js --no-single-run",
"test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run",
"test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run",
"test:mocha": "cross-env COVERAGE=true mocha --require test/node/setup.js --recursive packages/*/test/node/**.test.tsx",
"test:mocha:prod": "cross-env COVERAGE=true NODE_ENV=production mocha --require test/node/setup.js --recursive packages/*/test/node/**.test.tsx",
"test:mocha": "cross-env COVERAGE=true mocha --require test/node/setup.js --recursive packages/*/test/node/**.test.tsx --recursive packages/react/runtime/test/node/**.test.tsx",
"test:mocha:prod": "cross-env COVERAGE=true NODE_ENV=production mocha --require test/node/setup.js --recursive packages/*/test/node/**.test.tsx --recursive packages/react/runtime/test/node/**.test.tsx",
"docs:start": "cd docs && pnpm start",
"docs:build": "cd docs && pnpm build",
"docs:preview": "cd docs && pnpm preview",
Expand Down
33 changes: 32 additions & 1 deletion packages/react/runtime/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,33 @@ function createEffectStore(_usage: EffectStoreUsage): EffectStore {
};
}

const noop = () => {};

function createEmptyEffectStore(): EffectStore {
return {
_usage: UNMANAGED,
effect: {
_sources: undefined,
_callback() {},
_start() {
return /* endEffect */ noop;
},
_dispose() {},
},
subscribe() {
return /* unsubscribe */ noop;
},
getSnapshot() {
return 0;
},
_start() {},
f() {},
[symDispose]() {},
};
}

const emptyEffectStore = createEmptyEffectStore();

const _queueMicroTask = Promise.prototype.then.bind(Promise.resolve());

let finalCleanup: Promise<void> | undefined;
Expand Down Expand Up @@ -312,7 +339,11 @@ export function _useSignalsImplementation(

const storeRef = useRef<EffectStore>();
if (storeRef.current == null) {
storeRef.current = createEffectStore(_usage);
if (typeof window === "undefined") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing I am worried about here is react-native and others, shouldn't we detect this with i.e. layout-effect/effect firing?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's not really possible so I'd add a global so we have an escape hatch, like a function enableX which defaults to the typeof window check

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmmm true true...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that may be a decent escape hatch, but Imma sit on this for a day or two more to see if some other solution presents itself.

What I'd really like is some hook or something that React would call as an effect in both SSR and client-side renders but I couldn't find one...

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

storeRef.current = emptyEffectStore;
} else {
storeRef.current = createEffectStore(_usage);
}
}

const store = storeRef.current;
Expand Down
26 changes: 26 additions & 0 deletions packages/react/runtime/test/browser/useSignals.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -453,6 +453,32 @@ describe("useSignals", () => {
expect(scratch.innerHTML).to.equal("<div>Hello John!</div>");
});

it("should clean up signal dependencies after unmounting", async () => {
const getTargets = (s: any): any => s.t ?? s._targets;

const sig = signal(0);
function App() {
const effectStore = useSignals(/* MANAGED_COMPONENT */ 1);
try {
return <p>{sig.value}</p>;
} finally {
effectStore.f();
}
}

expect(getTargets(sig)).to.be.undefined;

await render(<App />);
expect(scratch.innerHTML).to.equal("<p>0</p>");
expect(getTargets(sig)).to.be.not.null.and.not.undefined;

act(() => root.unmount());
expect(scratch.innerHTML).to.equal("");

await Promise.resolve();
expect(getTargets(sig)).to.be.undefined;
});

// TODO: Figure out what to do here...
it.skip("(managed) should work with components that use render props", async () => {
function AutoFocusWithin({
Expand Down
25 changes: 24 additions & 1 deletion packages/react/runtime/test/node/renderToStaticMarkup.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { signal, useSignalEffect } from "@preact/signals-react";
import { signal, useSignalEffect } from "../../../";
import { useSignals } from "./../../../runtime";
import { createElement } from "react";
import { renderToStaticMarkup } from "react-dom/server";
import { mountSignalsTests } from "../../../test/shared/mounting";
Expand All @@ -21,4 +22,26 @@ describe("@preact/signals-react/runtime", () => {
expect(spy.called).to.be.false;
});
});

it("should clean up signal dependencies after executing", async () => {
const getTargets = (s: any): any => s.t ?? s._targets;

const sig = signal(0);
function App() {
const effectStore = useSignals(/* MANAGED_COMPONENT */ 1);
try {
return <p>{sig.value}</p>;
} finally {
effectStore.f();
}
}

expect(getTargets(sig)).to.be.undefined;

const html = renderToStaticMarkup(<App />);
expect(html).to.equal("<p>0</p>");

await Promise.resolve();
expect(getTargets(sig)).to.be.undefined;
});
});
Loading