Merge pull request #355 from preactjs/react-adapter-fixes

Fix React adapter in SSR and when rerendering

.changeset/lazy-badgers-happen.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals-react": patch
+---
+
+Fix React adapter in SSR and when rerendering

.github/workflows/main.yml

@@ -42,12 +42,12 @@ jobs:
       - name: Install dependencies
         run: pnpm install
 
-      - name: Tests
-        run: pnpm test
-
       - name: Build
         run: pnpm build
 
+      - name: Tests
+        run: pnpm test
+
       - name: Test production build
         run: pnpm test:minify
 

package.json

@@ -12,11 +12,16 @@
 		"postbuild:react": "cd packages/react/dist && mv -f react/src/index.d.ts signals.d.ts && rm -dr react",
 		"postbuild": "node ./scripts/node-13-exports.js",
 		"lint": "eslint 'packages/**/*.{ts,tsx,js,jsx}'",
-		"test": "cross-env COVERAGE=true karma start karma.conf.js --single-run",
-		"test:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run",
-		"test:watch": "karma start karma.conf.js --no-single-run",
-		"test:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run",
-		"test:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run",
+		"test": "pnpm test:karma && pnpm test:mocha",
+		"test:minify": "pnpm test:karma:minify && pnpm test:mocha",
+		"test:prod": "pnpm test:karma:prod && pnpm test:mocha:prod",
+		"test:karma": "cross-env COVERAGE=true karma start karma.conf.js --single-run",
+		"test:karma:minify": "cross-env COVERAGE=true MINIFY=true karma start karma.conf.js --single-run",
+		"test:karma:watch": "karma start karma.conf.js --no-single-run",
+		"test:karma:prod": "cross-env MINIFY=true NODE_ENV=production karma start karma.conf.js --single-run",
+		"test:karma:prod:watch": "cross-env NODE_ENV=production karma start karma.conf.js --no-single-run",
+		"test:mocha": "cross-env COVERAGE=true mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx",
+		"test:mocha:prod": "cross-env COVERAGE=true NODE_ENV=production mocha --require packages/react/test/node/setup.js --recursive packages/react/test/node/**.test.tsx",
 		"docs:start": "cd docs && pnpm start",
 		"docs:build": "cd docs && pnpm build",
 		"docs:preview": "cd docs && pnpm preview",
@@ -34,6 +39,7 @@
 		"@babel/preset-env": "^7.19.1",
 		"@babel/preset-react": "^7.18.6",
 		"@babel/preset-typescript": "^7.18.6",
+		"@babel/register": "^7.21.0",
 		"@changesets/changelog-github": "^0.4.6",
 		"@changesets/cli": "^2.24.2",
 		"@types/chai": "^4.3.3",

packages/react/test/browser/mounts.test.tsx

@@ -0,0 +1,32 @@
+import { mountSignalsTests } from "../shared/mounting";
+import {
+	Root,
+	createRoot,
+	act,
+	getConsoleErrorSpy,
+	checkConsoleErrorLogs,
+} from "../shared/utils";
+
+describe("@preact/signals-react mounting", () => {
+	let scratch: HTMLDivElement;
+	let root: Root;
+
+	async function render(element: JSX.Element | null): Promise<string> {
+		await act(() => root.render(element));
+		return scratch.innerHTML;
+	}
+
+	beforeEach(async () => {
+		scratch = document.createElement("div");
+		document.body.appendChild(scratch);
+		root = await createRoot(scratch);
+		getConsoleErrorSpy().resetHistory();
+	});
+
+	afterEach(async () => {
+		scratch.remove();
+		checkConsoleErrorLogs();
+	});
+
+	mountSignalsTests(render);
+});

packages/react/test/react-router.test.tsx → packages/react/test/browser/react-router.test.tsx

@@ -5,7 +5,7 @@ import { signal } from "@preact/signals-react";
 import { createElement } from "react";
 import * as ReactRouter from "react-router-dom";
 
-import { act, checkHangingAct, createRoot, Root } from "./utils";
+import { act, checkHangingAct, createRoot, Root } from "../shared/utils";
 
 const MemoryRouter = ReactRouter.MemoryRouter;
 const Routes = ReactRouter.Routes

packages/react/test/index.test.tsx → packages/react/test/browser/updates.test.tsx

@@ -16,6 +16,9 @@ import {
 	memo,
 	StrictMode,
 	createRef,
+	useState,
+	useContext,
+	createContext,
 } from "react";
 
 import { renderToStaticMarkup } from "react-dom/server";
@@ -26,9 +29,11 @@ import {
 	checkHangingAct,
 	isReact16,
 	isProd,
-} from "./utils";
+	getConsoleErrorSpy,
+	checkConsoleErrorLogs,
+} from "../shared/utils";
 
-describe("@preact/signals-react", () => {
+describe("@preact/signals-react updating", () => {
 	let scratch: HTMLDivElement;
 	let root: Root;
 
@@ -40,12 +45,15 @@ describe("@preact/signals-react", () => {
 		scratch = document.createElement("div");
 		document.body.appendChild(scratch);
 		root = await createRoot(scratch);
+		getConsoleErrorSpy().resetHistory();
 	});
 
 	afterEach(async () => {
-		checkHangingAct();
 		await act(() => root.unmount());
 		scratch.remove();
+
+		checkConsoleErrorLogs();
+		checkHangingAct();
 	});
 
 	describe("Text bindings", () => {
@@ -330,6 +338,102 @@ describe("@preact/signals-react", () => {
 				`<pre><code>-1</code><code>${count.value * 2}</code></pre>`
 			);
 		});
+
+		it("should not fail when a component calls setState while rendering", async () => {
+			let increment: () => void;
+			function App() {
+				const [state, setState] = useState(0);
+				increment = () => setState(state + 1);
+
+				if (state > 0 && state < 2) {
+					setState(state + 1);
+				}
+
+				return <div>{state}</div>;
+			}
+
+			await render(<App />);
+			expect(scratch.innerHTML).to.equal("<div>0</div>");
+
+			await act(() => {
+				increment();
+			});
+			expect(scratch.innerHTML).to.equal("<div>2</div>");
+		});
+
+		it("should not fail when a component calls setState multiple times while rendering", async () => {
+			let increment: () => void;
+			function App() {
+				const [state, setState] = useState(0);
+				increment = () => setState(state + 1);
+
+				if (state > 0 && state < 5) {
+					setState(state + 1);
+				}
+
+				return <div>{state}</div>;
+			}
+
+			await render(<App />);
+			expect(scratch.innerHTML).to.equal("<div>0</div>");
+
+			await act(() => {
+				increment();
+			});
+			expect(scratch.innerHTML).to.equal("<div>5</div>");
+		});
+
+		it("should not fail when a component only uses state-less hooks", async () => {
+			// This test is suppose to trigger a condition in React where the
+			// HooksDispatcherOnMountWithHookTypesInDEV is used. This dispatcher is
+			// used in the development build of React if a component has hook types
+			// defined but no memoizedState, meaning no stateful hooks (e.g. useState)
+			// are used. `useContext` is an example of a state-less hook because it
+			// does not mount any hook state onto the fiber's memoizedState field.
+			//
+			// However, as of writing, because our react adapter inserts a
+			// useSyncExternalStore into all components, all components have memoized
+			// state and so this condition is never hit. However, I'm leaving the test
+			// to capture this unique behavior to hopefully catch any errors caused by
+			// not understanding or handling this in the future.
+
+			const sig = signal(0);
+			const MyContext = createContext(0);
+
+			function Child() {
+				const value = useContext(MyContext);
+				return (
+					<div>
+						{sig} {value}
+					</div>
+				);
+			}
+
+			let updateContext: () => void;
+			function App() {
+				const [value, setValue] = useState(0);
+				updateContext = () => setValue(value + 1);
+
+				return (
+					<MyContext.Provider value={value}>
+						<Child />
+					</MyContext.Provider>
+				);
+			}
+
+			await render(<App />);
+			expect(scratch.innerHTML).to.equal("<div>0 0</div>");
+
+			await act(() => {
+				sig.value++;
+			});
+			expect(scratch.innerHTML).to.equal("<div>1 0</div>");
+
+			await act(() => {
+				updateContext();
+			});
+			expect(scratch.innerHTML).to.equal("<div>1 1</div>");
+		});
 	});
 
 	describe("useSignal()", () => {
@@ -479,7 +583,9 @@ describe("@preact/signals-react", () => {
 			expect(spy).to.have.been.calledOnceWith("foo", child);
 			spy.resetHistory();
 
-			await render(null);
+			await act(() => {
+				root.unmount();
+			});
 
 			expect(scratch.innerHTML).to.equal("");
 			expect(spy).not.to.have.been.called;

packages/react/test/node/renderToStaticMarkup.test.tsx

@@ -0,0 +1,24 @@
+import { signal, useSignalEffect } from "@preact/signals-react";
+import { expect } from "chai";
+import { createElement } from "react";
+import { renderToStaticMarkup } from "react-dom/server";
+import sinon from "sinon";
+import { mountSignalsTests } from "../shared/mounting";
+
+describe("renderToStaticMarkup", () => {
+	mountSignalsTests(renderToStaticMarkup);
+
+	it("should not invoke useSignalEffect", async () => {
+		const spy = sinon.spy();
+		const sig = signal("foo");
+
+		function App() {
+			useSignalEffect(() => spy(sig.value));
+			return <p>{sig.value}</p>;
+		}
+
+		const html = await renderToStaticMarkup(<App />);
+		expect(html).to.equal("<p>foo</p>");
+		expect(spy.called).to.be.false;
+	});
+});

packages/react/test/node/setup.js

@@ -0,0 +1,52 @@
+// import register from "@babel/register";
+const register = require("@babel/register").default;
+
+const coverage = String(process.env.COVERAGE) === "true";
+
+// @babel/register doesn't hook into the experimental NodeJS ESM loader API so
+// we need all test files to run as CommonJS modules in Node
+const env = [
+	"@babel/preset-env",
+	{
+		targets: {
+			node: "current",
+		},
+		loose: true,
+		modules: "commonjs",
+	},
+];
+
+const jsx = [
+	"@babel/preset-react",
+	{
+		runtime: "classic",
+		pragma: "createElement",
+		pragmaFrag: "Fragment",
+	},
+];
+
+const ts = [
+	"@babel/preset-typescript",
+	{
+		jsxPragma: "createElement",
+		jsxPragmaFrag: "Fragment",
+	},
+];
+
+register({
+	extensions: [".js", ".mjs", ".ts", ".tsx", ".mts", ".mtsx"],
+	cache: true,
+
+	sourceMaps: "inline",
+	presets: [ts, jsx, env],
+	plugins: [
+		coverage && [
+			"istanbul",
+			{
+				// TODO: Currently NodeJS tests always run against dist files. Should we
+				// change this?
+				// include: minify ? "**/dist/**/*.js" : "**/src/**/*.{js,jsx,ts,tsx}",
+			},
+		],
+	].filter(Boolean),
+});