From 3ff3a5c62ead3464ab5cd79f833d0568c786f8c1 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Tue, 11 Jun 2024 22:38:21 -0400 Subject: [PATCH 1/8] fix gc() clearing WeakRef asynchronously in Chrome 125 --- packages/core/test/signal.test.tsx | 2 ++ 1 file changed, 2 insertions(+) diff --git a/packages/core/test/signal.test.tsx b/packages/core/test/signal.test.tsx index 2aa1697e..fe4c45e7 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; }); }); From 0bed42d71929d26ecfdc1e48a16c92a0e6280563 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Tue, 11 Jun 2024 22:39:39 -0400 Subject: [PATCH 2/8] fix(react): ensure tracking context is closed prior to effects running via a layout effect, and ensure nested effects don't get left open --- packages/react/runtime/src/index.ts | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/packages/react/runtime/src/index.ts b/packages/react/runtime/src/index.ts index 5672173d..8c7e2050 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; } From d4bab22b1c8d8816e1470cfadf3a78287369d1d1 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Tue, 11 Jun 2024 22:40:12 -0400 Subject: [PATCH 3/8] chore(react): add suspense test --- .../runtime/test/browser/suspense.test.tsx | 146 ++++++++++++++++++ 1 file changed, 146 insertions(+) create mode 100644 packages/react/runtime/test/browser/suspense.test.tsx 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 00000000..fdd1c01c --- /dev/null +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -0,0 +1,146 @@ +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.only("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.equal("loading..."); + + act(() => { + signal1.value++; + signal2.value++; + }); + act(() => { + signal1.value--; + signal2.value--; + }); + + await act(async () => { + resolveMiddleProm(); + await middleProm; + }); + + expect(scratch.innerHTML).to.equal("loading..."); + + await act(async () => { + unsuspend(); + await prom; + }); + + expect(scratch.innerHTML).to.equal( + `

0

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

1

lazy
` + ); + }); +}); From 9073ef480788fa5bcede0156438294f23b5683e2 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 12 Jun 2024 08:22:31 -0400 Subject: [PATCH 4/8] remove describe.only Co-authored-by: Jovi De Croock --- packages/react/runtime/test/browser/suspense.test.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx index fdd1c01c..65f996ec 100644 --- a/packages/react/runtime/test/browser/suspense.test.tsx +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -13,7 +13,7 @@ import { getConsoleErrorSpy, } from "../../../test/shared/utils"; -describe.only("Suspense", () => { +describe("Suspense", () => { let scratch: HTMLDivElement; let root: Root; From 7a399a64e2a2a49d35efe635a333be58c0e615f8 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 12 Jun 2024 08:26:09 -0400 Subject: [PATCH 5/8] fix prod test act timing --- packages/react/runtime/test/browser/suspense.test.tsx | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx index 65f996ec..6d5139d6 100644 --- a/packages/react/runtime/test/browser/suspense.test.tsx +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -1,3 +1,6 @@ +// @ts-expect-error +globalThis.IS_REACT_ACT_ENVIRONMENT = true; + import { createElement, lazy, useLayoutEffect, Suspense } from "react"; import { signal } from "@preact/signals-core"; import { @@ -110,11 +113,11 @@ describe("Suspense", () => { await render(); expect(scratch.innerHTML).to.equal("loading..."); - act(() => { + await act(async () => { signal1.value++; signal2.value++; }); - act(() => { + await act(async () => { signal1.value--; signal2.value--; }); @@ -135,7 +138,7 @@ describe("Suspense", () => { `

0

lazy
` ); - act(() => { + await act(async () => { signal1.value++; signal2.value++; }); From d65345152cf4160cdda602830d7486a619949aa5 Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 12 Jun 2024 08:27:23 -0400 Subject: [PATCH 6/8] Add changeset --- .changeset/gorgeous-moose-hope.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/gorgeous-moose-hope.md diff --git a/.changeset/gorgeous-moose-hope.md b/.changeset/gorgeous-moose-hope.md new file mode 100644 index 00000000..3a760654 --- /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 From 1203c32be9da25228397dc0757505ef19c01b57d Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 12 Jun 2024 09:16:14 -0400 Subject: [PATCH 7/8] silence act() warnings in react 16 --- packages/react/test/shared/utils.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/react/test/shared/utils.ts b/packages/react/test/shared/utils.ts index 7da904ca..9ad51d05 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. From 6d049c1f7d693246688f33bddad712975e8fc6df Mon Sep 17 00:00:00 2001 From: Jason Miller Date: Wed, 12 Jun 2024 09:16:26 -0400 Subject: [PATCH 8/8] fix suspense tests in react 16 --- .../runtime/test/browser/suspense.test.tsx | 19 +++++++++++++++++-- 1 file changed, 17 insertions(+), 2 deletions(-) diff --git a/packages/react/runtime/test/browser/suspense.test.tsx b/packages/react/runtime/test/browser/suspense.test.tsx index 6d5139d6..5142ada7 100644 --- a/packages/react/runtime/test/browser/suspense.test.tsx +++ b/packages/react/runtime/test/browser/suspense.test.tsx @@ -111,7 +111,12 @@ describe("Suspense", () => { } await render(); - expect(scratch.innerHTML).to.equal("loading..."); + expect(scratch.innerHTML).to.be.oneOf([ + // react 17+ + `loading...`, + // react 16 + `

0

loading...`, + ]); await act(async () => { signal1.value++; @@ -127,13 +132,23 @@ describe("Suspense", () => { await middleProm; }); - expect(scratch.innerHTML).to.equal("loading..."); + 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
` );