From 1151f463bf925250cadf341f81fa688f649e8e3e Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 12 Nov 2025 18:35:51 +0100 Subject: [PATCH 1/4] Delay subscribing to a component until its fully mounted --- .changeset/nasty-rings-float.md | 5 + packages/react/runtime/src/index.ts | 19 + .../runtime/test/browser/suspense.test.tsx | 363 ++++++++++++++++++ 3 files changed, 387 insertions(+) create mode 100644 .changeset/nasty-rings-float.md diff --git a/.changeset/nasty-rings-float.md b/.changeset/nasty-rings-float.md new file mode 100644 index 000000000..c606c5a20 --- /dev/null +++ b/.changeset/nasty-rings-float.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-react": major +--- + +Delay subscribing to signals until the component is actually mounted diff --git a/packages/react/runtime/src/index.ts b/packages/react/runtime/src/index.ts index f7985d6a7..454049d16 100644 --- a/packages/react/runtime/src/index.ts +++ b/packages/react/runtime/src/index.ts @@ -94,6 +94,8 @@ export interface EffectStore { getSnapshot(): number; /** startEffect - begin tracking signals used in this component */ _start(): void; + _subscribers: Array<{ signal: Signal; node: any }>; + _sub: typeof Signal.prototype._subscribe; /** finishEffect - stop tracking the signals used in this component */ f(): void; [symDispose](): void; @@ -101,10 +103,15 @@ export interface EffectStore { let currentStore: EffectStore | undefined; +const realSubscribe = Signal.prototype._subscribe; function startComponentEffect( prevStore: EffectStore | undefined, nextStore: EffectStore ) { + nextStore._sub = Signal.prototype._subscribe; + Signal.prototype._subscribe = function (this: Signal, node: any) { + nextStore._subscribers.push({ signal: this, node }); + }; const endEffect = nextStore.effect._start(); currentStore = nextStore; @@ -116,6 +123,7 @@ function finishComponentEffect( prevStore: EffectStore | undefined, endEffect: () => void ) { + Signal.prototype._subscribe = this._sub; endEffect(); currentStore = prevStore; } @@ -157,6 +165,8 @@ function createEffectStore(_usage: EffectStoreUsage): EffectStore { return { _usage, + _subscribers: [], + _sub: realSubscribe, effect: effectInstance, subscribe(onStoreChange) { onChangeNotifyReact = onStoreChange; @@ -291,6 +301,8 @@ const noop = () => {}; function createEmptyEffectStore(): EffectStore { return { + _subscribers: [], + _sub: noop as any, _usage: UNMANAGED, effect: { _sources: undefined, @@ -354,6 +366,13 @@ export function _useSignalsImplementation( // note: _usage is a constant here, so conditional is okay if (_usage === UNMANAGED) useIsomorphicLayoutEffect(cleanupTrailingStore); + useIsomorphicLayoutEffect(() => { + store._subscribers.forEach(({ signal, node }) => { + realSubscribe.call(signal, node); + }); + store._subscribers = []; + }); + return store; } diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx index 5142ada70..0cb1be2ea 100644 --- a/packages/react/runtime/test/browser/suspense.test.tsx +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -161,4 +161,367 @@ describe("Suspense", () => { `

1

lazy
` ); }); + + it("should clean up signals after unmount with multiple suspense boundaries", async () => { + let watchedCallCount = 0; + let unwatchedCallCount = 0; + + // Create a signal with watched/unwatched callbacks to track cleanup + const trackedSignal = signal(0, { + name: "trackedSignal", + watched: function () { + watchedCallCount++; + }, + unwatched: function () { + unwatchedCallCount++; + }, + }); + + let resolveFirstProm!: () => void; + let firstPromResolved = false; + const firstProm = new Promise(resolve => { + resolveFirstProm = () => { + firstPromResolved = true; + resolve(undefined); + }; + }); + + let resolveSecondProm!: () => void; + let secondPromResolved = false; + const secondProm = new Promise(resolve => { + resolveSecondProm = () => { + secondPromResolved = true; + resolve(undefined); + }; + }); + + function FirstSuspendingComponent() { + useSignals(0); + // Access the signal before any suspense + const value = trackedSignal.value; + if (!firstPromResolved) throw firstProm; + return
First
; + } + + function SecondSuspendingComponent() { + useSignals(); + // Access the signal after first suspense + const value = trackedSignal.value; + if (!secondPromResolved) throw secondProm; + return
Second
; + } + + function RegularComponent() { + useSignals(); + // Access the signal normally + return
Regular
; + } + + function Parent() { + useSignals(); + // Access the signal at the top level + const value = trackedSignal.value; + return ( +
+ + Loading first...}> + + + Loading second...}> + + +
+ ); + } + + // Initial render - should trigger watched callback + await render(); + expect(scratch.innerHTML).to.contain("Loading first..."); + expect(scratch.innerHTML).to.contain("Loading second..."); + expect(scratch.innerHTML).to.contain("Regular"); + + // Signal should be watched by now + expect(watchedCallCount).to.be.greaterThan(0); + expect(unwatchedCallCount).to.equal(0); + + // Resolve first suspense + await act(async () => { + resolveFirstProm(); + await firstProm; + }); + + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Loading second..."); + + // Resolve second suspense + await act(async () => { + resolveSecondProm(); + await secondProm; + }); + + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Second"); + expect(scratch.innerHTML).to.contain("Regular"); + + // Update signal to verify it's still being watched + await act(async () => { + trackedSignal.value = 42; + }); + + expect(scratch.innerHTML).to.contain('data-parent="42"'); + expect(scratch.innerHTML).to.contain('data-regular="42"'); + expect(scratch.innerHTML).to.contain('data-first="42"'); + expect(scratch.innerHTML).to.contain('data-second="42"'); + + // Now unmount the entire tree + await act(async () => { + root.unmount(); + }); + + expect(scratch.innerHTML).to.equal(""); + + // Wait for cleanup to complete + await new Promise(resolve => setTimeout(resolve, 10)); + + // After unmount, the signal should be unwatched + expect(unwatchedCallCount).to.be.greaterThan(0); + + // Verify the signal is no longer being watched by trying to update it + // (this won't trigger any re-renders since no components are subscribed) + trackedSignal.value = 999; + + // The signal value should have changed but no components should re-render + expect(trackedSignal.value).to.equal(999); + expect(scratch.innerHTML).to.equal(""); + }); + + it("should clean up signals after unmount with multiple suspense boundaries and use of try catch", async () => { + let watchedCallCount = 0; + let unwatchedCallCount = 0; + + // Create a signal with watched/unwatched callbacks to track cleanup + const trackedSignal = signal(0, { + name: "trackedSignal", + watched: function () { + watchedCallCount++; + }, + unwatched: function () { + unwatchedCallCount++; + }, + }); + + let resolveFirstProm!: () => void; + let firstPromResolved = false; + const firstProm = new Promise(resolve => { + resolveFirstProm = () => { + firstPromResolved = true; + resolve(undefined); + }; + }); + + let resolveSecondProm!: () => void; + let secondPromResolved = false; + const secondProm = new Promise(resolve => { + resolveSecondProm = () => { + secondPromResolved = true; + resolve(undefined); + }; + }); + + function FirstSuspendingComponent() { + const store = useSignals(1); + try { + // Access the signal before any suspense + const value = trackedSignal.value; + if (!firstPromResolved) throw firstProm; + return
First
; + } finally { + store.f(); + } + } + + function SecondSuspendingComponent() { + const store = useSignals(1); + try { + // Access the signal after first suspense + const value = trackedSignal.value; + if (!secondPromResolved) throw secondProm; + return
Second
; + } finally { + store.f(); + } + } + + function RegularComponent() { + const store = useSignals(1); + try { + // Access the signal normally + return
Regular
; + } finally { + store.f(); + } + } + + function Parent() { + const store = useSignals(1); + try { + // Access the signal at the top level + const value = trackedSignal.value; + return ( +
+ + Loading first...}> + + + Loading second...}> + + +
+ ); + } finally { + store.f(); + } + } + + // Initial render - should trigger watched callback + await render(); + expect(scratch.innerHTML).to.contain("Loading first..."); + expect(scratch.innerHTML).to.contain("Loading second..."); + expect(scratch.innerHTML).to.contain("Regular"); + + // Signal should be watched by now + expect(watchedCallCount).to.be.greaterThan(0); + expect(unwatchedCallCount).to.equal(0); + + // Resolve first suspense + await act(async () => { + resolveFirstProm(); + await firstProm; + }); + + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Loading second..."); + + // Resolve second suspense + await act(async () => { + resolveSecondProm(); + await secondProm; + }); + + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Second"); + expect(scratch.innerHTML).to.contain("Regular"); + + // Update signal to verify it's still being watched + await act(async () => { + trackedSignal.value = 42; + }); + + expect(scratch.innerHTML).to.contain('data-parent="42"'); + expect(scratch.innerHTML).to.contain('data-regular="42"'); + expect(scratch.innerHTML).to.contain('data-first="42"'); + expect(scratch.innerHTML).to.contain('data-second="42"'); + + // Now unmount the entire tree + await act(async () => { + root.unmount(); + }); + + expect(scratch.innerHTML).to.equal(""); + + // Wait for cleanup to complete + await new Promise(resolve => setTimeout(resolve, 10)); + + // After unmount, the signal should be unwatched + expect(unwatchedCallCount).to.be.greaterThan(0); + + // Verify the signal is no longer being watched by trying to update it + // (this won't trigger any re-renders since no components are subscribed) + trackedSignal.value = 999; + + // The signal value should have changed but no components should re-render + expect(trackedSignal.value).to.equal(999); + expect(scratch.innerHTML).to.equal(""); + }); + + it("should maintain signal watching and clean up after unmount", async () => { + let watchedCallCount = 0; + let unwatchedCallCount = 0; + + // Create a signal with watched/unwatched callbacks to track cleanup + const trackedSignal = signal(0, { + name: "trackedSignal", + watched: function () { + watchedCallCount++; + }, + unwatched: function () { + unwatchedCallCount++; + }, + }); + + function RegularComponent() { + useSignals(); + // Access the signal normally + return
Regular
; + } + + function Parent() { + useSignals(); + // Access the signal at the top level + const value = trackedSignal.value; + return ( +
+ +
+ ); + } + + // Initial render - should trigger watched callback + await render(); + expect(scratch.innerHTML).to.contain("Regular"); + + // Signal should be watched by now + expect(watchedCallCount).to.be.greaterThan(0); + expect(unwatchedCallCount).to.equal(0); + + // Update signal - should work normally + await act(async () => { + trackedSignal.value = 10; + }); + + expect(scratch.innerHTML).to.contain('data-parent="10"'); + expect(scratch.innerHTML).to.contain('data-regular="10"'); + + // Update signal again + await act(async () => { + trackedSignal.value = 20; + }); + + expect(scratch.innerHTML).to.contain('data-parent="20"'); + expect(scratch.innerHTML).to.contain('data-regular="20"'); + + // Signal should still be watched (no unwatched calls yet) + expect(unwatchedCallCount).to.equal(0); + + // Now unmount the entire tree + await act(async () => { + root.unmount(); + }); + + expect(scratch.innerHTML).to.equal(""); + + // Wait for cleanup to complete + await new Promise(resolve => setTimeout(resolve, 10)); + + // After unmount, the signal should be unwatched + expect(unwatchedCallCount).to.be.greaterThan(0); + + // Verify the signal is no longer being watched by trying to update it + // (this won't trigger any re-renders since no components are subscribed) + trackedSignal.value = 999; + + // The signal value should have changed but no components should re-render + expect(trackedSignal.value).to.equal(999); + expect(scratch.innerHTML).to.equal(""); + }); }); From 69e6c7ff3f32df8decf55e4cac3add0e370e816e Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 12 Nov 2025 19:58:59 +0100 Subject: [PATCH 2/4] Update packages/react/runtime/src/index.ts --- packages/react/runtime/src/index.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/runtime/src/index.ts b/packages/react/runtime/src/index.ts index 454049d16..e609a7397 100644 --- a/packages/react/runtime/src/index.ts +++ b/packages/react/runtime/src/index.ts @@ -108,7 +108,7 @@ function startComponentEffect( prevStore: EffectStore | undefined, nextStore: EffectStore ) { - nextStore._sub = Signal.prototype._subscribe; + nextStore._sub = prevStore ? prevStore._sub : Signal.prototype._subscribe; Signal.prototype._subscribe = function (this: Signal, node: any) { nextStore._subscribers.push({ signal: this, node }); }; From 54f6443e066d99ac6f1b3da1207ebfdb5a8b17cd Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 12 Nov 2025 20:09:12 +0100 Subject: [PATCH 3/4] Add tests --- .../runtime/test/browser/suspense.test.tsx | 31 ++++++++++++++++--- .../runtime/test/browser/useSignals.test.tsx | 3 ++ 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx index 0cb1be2ea..d924255c4 100644 --- a/packages/react/runtime/test/browser/suspense.test.tsx +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -2,7 +2,7 @@ globalThis.IS_REACT_ACT_ENVIRONMENT = true; import { createElement, lazy, useLayoutEffect, Suspense } from "react"; -import { signal } from "@preact/signals-core"; +import { signal, Signal } from "@preact/signals-core"; import { useComputed, useSignalEffect, @@ -19,6 +19,7 @@ import { describe("Suspense", () => { let scratch: HTMLDivElement; let root: Root; + let originalSubscribe: typeof Signal.prototype._subscribe; async function render(element: Parameters[0]) { await act(() => root.render(element)); @@ -29,6 +30,7 @@ describe("Suspense", () => { document.body.appendChild(scratch); root = await createRoot(scratch); getConsoleErrorSpy().resetHistory(); + originalSubscribe = Signal.prototype._subscribe; }); afterEach(async () => { @@ -40,6 +42,7 @@ describe("Suspense", () => { // // checkConsoleErrorLogs(); checkHangingAct(); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); }); it("should handle suspending and unsuspending", async () => { @@ -122,15 +125,19 @@ describe("Suspense", () => { signal1.value++; signal2.value++; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); + await act(async () => { signal1.value--; signal2.value--; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); await act(async () => { resolveMiddleProm(); await middleProm; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.be.oneOf([ // react 17+ @@ -143,6 +150,7 @@ describe("Suspense", () => { unsuspend(); await prom; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); // react 16 uses `style.setProperty()` to clear display value, which leaves an empty style attr in innerHTML. // react 17 does not do this, so we normalize 16 behavior to 17 here. @@ -157,6 +165,8 @@ describe("Suspense", () => { signal1.value++; signal2.value++; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); + expect(scratch.innerHTML).to.equal( `

1

lazy
` ); @@ -236,6 +246,7 @@ describe("Suspense", () => { // Initial render - should trigger watched callback await render(); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("Loading first..."); expect(scratch.innerHTML).to.contain("Loading second..."); expect(scratch.innerHTML).to.contain("Regular"); @@ -249,6 +260,7 @@ describe("Suspense", () => { resolveFirstProm(); await firstProm; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("First"); expect(scratch.innerHTML).to.contain("Loading second..."); @@ -258,6 +270,7 @@ describe("Suspense", () => { resolveSecondProm(); await secondProm; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("First"); expect(scratch.innerHTML).to.contain("Second"); @@ -267,6 +280,7 @@ describe("Suspense", () => { await act(async () => { trackedSignal.value = 42; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain('data-parent="42"'); expect(scratch.innerHTML).to.contain('data-regular="42"'); @@ -281,7 +295,7 @@ describe("Suspense", () => { expect(scratch.innerHTML).to.equal(""); // Wait for cleanup to complete - await new Promise(resolve => setTimeout(resolve, 10)); + await new Promise(resolve => setTimeout(resolve, 100)); // After unmount, the signal should be unwatched expect(unwatchedCallCount).to.be.greaterThan(0); @@ -385,6 +399,7 @@ describe("Suspense", () => { // Initial render - should trigger watched callback await render(); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("Loading first..."); expect(scratch.innerHTML).to.contain("Loading second..."); expect(scratch.innerHTML).to.contain("Regular"); @@ -399,6 +414,7 @@ describe("Suspense", () => { await firstProm; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("First"); expect(scratch.innerHTML).to.contain("Loading second..."); @@ -408,6 +424,7 @@ describe("Suspense", () => { await secondProm; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("First"); expect(scratch.innerHTML).to.contain("Second"); expect(scratch.innerHTML).to.contain("Regular"); @@ -417,6 +434,7 @@ describe("Suspense", () => { trackedSignal.value = 42; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain('data-parent="42"'); expect(scratch.innerHTML).to.contain('data-regular="42"'); expect(scratch.innerHTML).to.contain('data-first="42"'); @@ -427,10 +445,11 @@ describe("Suspense", () => { root.unmount(); }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.equal(""); // Wait for cleanup to complete - await new Promise(resolve => setTimeout(resolve, 10)); + await new Promise(resolve => setTimeout(resolve, 100)); // After unmount, the signal should be unwatched expect(unwatchedCallCount).to.be.greaterThan(0); @@ -478,6 +497,7 @@ describe("Suspense", () => { // Initial render - should trigger watched callback await render(); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain("Regular"); // Signal should be watched by now @@ -488,6 +508,7 @@ describe("Suspense", () => { await act(async () => { trackedSignal.value = 10; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain('data-parent="10"'); expect(scratch.innerHTML).to.contain('data-regular="10"'); @@ -497,6 +518,7 @@ describe("Suspense", () => { trackedSignal.value = 20; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.contain('data-parent="20"'); expect(scratch.innerHTML).to.contain('data-regular="20"'); @@ -508,10 +530,11 @@ describe("Suspense", () => { root.unmount(); }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); expect(scratch.innerHTML).to.equal(""); // Wait for cleanup to complete - await new Promise(resolve => setTimeout(resolve, 10)); + await new Promise(resolve => setTimeout(resolve, 100)); // After unmount, the signal should be unwatched expect(unwatchedCallCount).to.be.greaterThan(0); diff --git a/packages/react/runtime/test/browser/useSignals.test.tsx b/packages/react/runtime/test/browser/useSignals.test.tsx index 987faec9c..8457fbdd3 100644 --- a/packages/react/runtime/test/browser/useSignals.test.tsx +++ b/packages/react/runtime/test/browser/useSignals.test.tsx @@ -15,6 +15,7 @@ const MANAGED_HOOK = 2; describe("useSignals", () => { let scratch: HTMLDivElement; let root: Root; + let originalSubscribe: typeof Signal.prototype._subscribe; async function render(element: Parameters[0]) { await act(() => root.render(element)); @@ -53,6 +54,7 @@ describe("useSignals", () => { document.body.appendChild(scratch); root = await createRoot(scratch); getConsoleErrorSpy().resetHistory(); + originalSubscribe = Signal.prototype._subscribe; }); afterEach(async () => { @@ -64,6 +66,7 @@ describe("useSignals", () => { // // checkConsoleErrorLogs(); checkHangingAct(); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); }); it("should rerender components when signals they use change", async () => { From 2f22abefbc5712ebe179b1b2827e1ea0ed24d335 Mon Sep 17 00:00:00 2001 From: Jovi De Croock Date: Wed, 12 Nov 2025 20:10:28 +0100 Subject: [PATCH 4/4] Ensure we do things correctly in finish effect handoff --- packages/react/runtime/src/index.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/react/runtime/src/index.ts b/packages/react/runtime/src/index.ts index e609a7397..d8317fcd9 100644 --- a/packages/react/runtime/src/index.ts +++ b/packages/react/runtime/src/index.ts @@ -108,7 +108,7 @@ function startComponentEffect( prevStore: EffectStore | undefined, nextStore: EffectStore ) { - nextStore._sub = prevStore ? prevStore._sub : Signal.prototype._subscribe; + nextStore._sub = prevStore ? prevStore._sub : realSubscribe; Signal.prototype._subscribe = function (this: Signal, node: any) { nextStore._subscribers.push({ signal: this, node }); }; @@ -123,7 +123,7 @@ function finishComponentEffect( prevStore: EffectStore | undefined, endEffect: () => void ) { - Signal.prototype._subscribe = this._sub; + Signal.prototype._subscribe = prevStore ? prevStore._sub : realSubscribe; endEffect(); currentStore = prevStore; }