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..d8317fcd9 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 = prevStore ? prevStore._sub : realSubscribe; + 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 = prevStore ? prevStore._sub : realSubscribe; 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..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,8 +165,386 @@ describe("Suspense", () => { signal1.value++; signal2.value++; }); + expect(Signal.prototype._subscribe).to.equal(originalSubscribe); + expect(scratch.innerHTML).to.equal( `

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(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"); + + // 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Loading second..."); + + // Resolve second suspense + await act(async () => { + resolveSecondProm(); + 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"); + + // Update signal to verify it's still being watched + 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"'); + 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, 100)); + + // 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(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"); + + // 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + expect(scratch.innerHTML).to.contain("First"); + expect(scratch.innerHTML).to.contain("Loading second..."); + + // Resolve second suspense + await act(async () => { + resolveSecondProm(); + 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"); + + // Update signal to verify it's still being watched + 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"'); + 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + expect(scratch.innerHTML).to.equal(""); + + // Wait for cleanup to complete + await new Promise(resolve => setTimeout(resolve, 100)); + + // 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + + 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + 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(Signal.prototype._subscribe).to.equal(originalSubscribe); + expect(scratch.innerHTML).to.equal(""); + + // Wait for cleanup to complete + await new Promise(resolve => setTimeout(resolve, 100)); + + // 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(""); + }); }); 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 () => {