Merge pull request #570 from preactjs/fix/react-unmanaged-out-of-orde…

…r-effect

Fix out-of-order effect error when suspending in React Native

.changeset/gorgeous-moose-hope.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals-react": patch
+---
+
+Fix out-of-order effect error when suspending in React Native

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;
 		});
 	});

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<void> | 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;
 }

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<Root["render"]>[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 <p>{signal1.value}</p>;
+		}
+
+		function Middle({ children }: React.PropsWithChildren) {
+			useEverything();
+			const value = signal1.value;
+			useLayoutEffect(() => {
+				signal1.value++;
+				signal1.value--;
+			}, []);
+			if (!middlePromResolved) throw middleProm;
+			return <div data-foo={value}>{children}</div>;
+		}
+
+		function LazyComponent() {
+			useEverything();
+			return <span>lazy</span>;
+		}
+
+		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 (
+				<Suspense fallback={<span>loading...</span>}>
+					<Child />
+					<Middle>
+						<SuspendingComponent />
+					</Middle>
+				</Suspense>
+			);
+		}
+
+		await render(<Parent />);
+		expect(scratch.innerHTML).to.be.oneOf([
+			// react 17+
+			`<span>loading...</span>`,
+			// react 16
+			`<p style="display: none !important;">0</p><span>loading...</span>`,
+		]);
+
+		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+
+			`<span>loading...</span>`,
+			// react 16
+			`<p style="display: none !important;">0</p><div data-foo="0" style="display: none !important;"></div><span>loading...</span>`,
+		]);
+
+		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(
+			`<p>0</p><div data-foo="0"><span>lazy</span></div>`
+		);
+
+		await act(async () => {
+			signal1.value++;
+			signal2.value++;
+		});
+		expect(scratch.innerHTML).to.equal(
+			`<p>1</p><div data-foo="1"><span>lazy</span></div>`
+		);
+	});
+});

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.