diff --git a/.changeset/gorgeous-moose-hope.md b/.changeset/gorgeous-moose-hope.md new file mode 100644 index 000000000..3a7606540 --- /dev/null +++ b/.changeset/gorgeous-moose-hope.md @@ -0,0 +1,5 @@ +--- +"@preact/signals-react": patch +--- + +Fix out-of-order effect error when suspending in React Native diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 2aa1697ea..fe4c45e7a 100644 --- a/packages/core/test/signal.test.tsx +++ b/packages/core/test/signal.test.tsx @@ -1333,6 +1333,7 @@ describe("computed()", () => { (gc as () => void)(); await new Promise(resolve => setTimeout(resolve, 0)); + (gc as () => void)(); expect(ref.deref()).to.be.undefined; }); @@ -1352,6 +1353,7 @@ describe("computed()", () => { dispose(); (gc as () => void)(); await new Promise(resolve => setTimeout(resolve, 0)); + (gc as () => void)(); expect(ref.deref()).to.be.undefined; }); }); diff --git a/packages/react/runtime/src/index.ts b/packages/react/runtime/src/index.ts index 5672173d7..8c7e2050a 100644 --- a/packages/react/runtime/src/index.ts +++ b/packages/react/runtime/src/index.ts @@ -5,7 +5,7 @@ import { Signal, ReadonlySignal, } from "@preact/signals-core"; -import { useRef, useMemo, useEffect } from "react"; +import { useRef, useMemo, useEffect, useLayoutEffect } from "react"; import { useSyncExternalStore } from "use-sync-external-store/shim/index.js"; import { isAutoSignalTrackingInstalled } from "./auto"; @@ -268,8 +268,9 @@ function createEffectStore(_usage: EffectStoreUsage): EffectStore { } }, f() { - endEffect?.(); + const end = endEffect; endEffect = undefined; + end?.(); }, [symDispose]() { this.f(); @@ -307,12 +308,13 @@ const _queueMicroTask = Promise.prototype.then.bind(Promise.resolve()); let finalCleanup: Promise | undefined; export function ensureFinalCleanup() { if (!finalCleanup) { - finalCleanup = _queueMicroTask(() => { - finalCleanup = undefined; - currentStore?.f(); - }); + finalCleanup = _queueMicroTask(cleanupTrailingStore); } } +function cleanupTrailingStore() { + finalCleanup = undefined; + currentStore?.f(); +} /** * Custom hook to create the effect to track signals used during render and @@ -331,6 +333,8 @@ export function _useSignalsImplementation( const store = storeRef.current; useSyncExternalStore(store.subscribe, store.getSnapshot, store.getSnapshot); store._start(); + // note: _usage is a constant here, so conditional is okay + if (_usage === UNMANAGED) useLayoutEffect(cleanupTrailingStore); return store; } diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx new file mode 100644 index 000000000..5142ada70 --- /dev/null +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -0,0 +1,164 @@ +// @ts-expect-error +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + +import { createElement, lazy, useLayoutEffect, Suspense } from "react"; +import { signal } from "@preact/signals-core"; +import { + useComputed, + useSignalEffect, + useSignals, +} from "@preact/signals-react/runtime"; +import { + Root, + createRoot, + act, + checkHangingAct, + getConsoleErrorSpy, +} from "../../../test/shared/utils"; + +describe("Suspense", () => { + let scratch: HTMLDivElement; + let root: Root; + + async function render(element: Parameters[0]) { + await act(() => root.render(element)); + } + + beforeEach(async () => { + scratch = document.createElement("div"); + document.body.appendChild(scratch); + root = await createRoot(scratch); + getConsoleErrorSpy().resetHistory(); + }); + + afterEach(async () => { + await act(() => root.unmount()); + scratch.remove(); + + // TODO: Consider re-enabling, though updates during finalCleanup are not + // wrapped in act(). + // + // checkConsoleErrorLogs(); + checkHangingAct(); + }); + + it("should handle suspending and unsuspending", async () => { + const signal1 = signal(0); + const signal2 = signal(0); + function Child() { + useEverything(); + return

{signal1.value}

; + } + + function Middle({ children }: React.PropsWithChildren) { + useEverything(); + const value = signal1.value; + useLayoutEffect(() => { + signal1.value++; + signal1.value--; + }, []); + if (!middlePromResolved) throw middleProm; + return
{children}
; + } + + function LazyComponent() { + useEverything(); + return lazy; + } + + let resolveMiddleProm!: () => void; + let middlePromResolved = false; + const middleProm = new Promise(resolve => { + resolveMiddleProm = () => { + middlePromResolved = true; + resolve(undefined); + }; + }); + let unsuspend!: () => void; + let prom = new Promise<{ default: React.ComponentType }>(resolve => { + unsuspend = () => resolve({ default: LazyComponent }); + }); + const SuspendingComponent = lazy(() => prom); + + function useEverything() { + useSignals(); + signal1.value; + signal2.value; + const comp = useComputed(() => ({ + s1: signal1.value, + s2: signal2.value, + })); + comp.value; + useSignalEffect(() => { + signal1.value; + signal2.value; + }); + useSignals(); + signal1.value; + signal2.value; + } + + function Parent() { + useEverything(); + return ( + loading...}> + + + + + + ); + } + + await render(); + expect(scratch.innerHTML).to.be.oneOf([ + // react 17+ + `loading...`, + // react 16 + `

0

loading...`, + ]); + + await act(async () => { + signal1.value++; + signal2.value++; + }); + await act(async () => { + signal1.value--; + signal2.value--; + }); + + await act(async () => { + resolveMiddleProm(); + await middleProm; + }); + + expect(scratch.innerHTML).to.be.oneOf([ + // react 17+ + `loading...`, + // react 16 + `

0

loading...`, + ]); + + await act(async () => { + unsuspend(); + await prom; + }); + + // 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. + scratch + .querySelectorAll('[style=""]') + .forEach(node => node.removeAttribute("style")); + expect(scratch.innerHTML).to.equal( + `

0

lazy
` + ); + + await act(async () => { + signal1.value++; + signal2.value++; + }); + expect(scratch.innerHTML).to.equal( + `

1

lazy
` + ); + }); +}); diff --git a/packages/react/test/shared/utils.ts b/packages/react/test/shared/utils.ts index 7da904caf..9ad51d052 100644 --- a/packages/react/test/shared/utils.ts +++ b/packages/react/test/shared/utils.ts @@ -68,10 +68,22 @@ export function checkHangingAct() { } } +function realActShim(cb: any) { + const ret = realAct(cb); + if (String(ret.then).includes("it is not a Promise.")) { + return { + then(r: any) { + r(); + }, + }; + } + return ret; +} + export const act = process.env.NODE_ENV === "production" ? (prodActShim as typeof realAct) - : realAct; + : (realActShim as typeof realAct); /** * `console.log` supports formatting strings with `%s` for string substitutions.