[React] Don't track signals read during SSR render (#640)

* Run react/runtime mocha tests

* Add test validating signal targets are cleaned up properly

* If window is not defined, don't track signal usage in component render

* Add changeset

* Attempt to fix react-signals/runtime node tests in CI

* Attempt to fix running test in CI again

* Add note to Readme about React SSR

Author: andrewiggins

.changeset/dirty-poets-cover.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals-react": patch
+---
+
+Don't track signals read during an SSR render (e.g. renderToString, renderToStaticMarkup)

packages/react/README.md

@@ -98,6 +98,10 @@ function Counter() {
 }
 ```
 
+### Using signals with React's SSR APIs
+
+Components rendered using SSR APIs (e.g. `renderToString`) in a server environment (i.e. an environment without a global `window` object) will not track signals used during render. Components generally don't rerender when using SSR APIs so tracking signal usage is useless since changing these signals can't trigger a rerender.
+
 ### Rendering optimizations
 
 The React adapter ships with several optimizations it can apply out of the box to skip virtual-dom rendering entirely. If you pass a signal directly into JSX, it will bind directly to the DOM `Text` node that is created and update that whenever the signal changes.
@@ -132,7 +136,7 @@ To opt into this optimization, simply pass the signal directly instead of access
 
 This version of React integration does not support passing signals as DOM attributes. Support for this may be added at a later date.
 
-Using signals into render props is not recommended. In this situation, the component that reads the signal is the component that calls the render prop, which may or may not be hooked up to track signals. For example:
+Using signals in render props is not recommended. In this situation, the component that reads the signal is the component that calls the render prop, which may or may not be hooked up to track signals. For example:
 
 ```js
 const count = signal(0);

packages/react/runtime/src/index.ts

@@ -285,6 +285,33 @@ function createEffectStore(_usage: EffectStoreUsage): EffectStore {
 	};
 }
 
+const noop = () => {};
+
+function createEmptyEffectStore(): EffectStore {
+	return {
+		_usage: UNMANAGED,
+		effect: {
+			_sources: undefined,
+			_callback() {},
+			_start() {
+				return /* endEffect */ noop;
+			},
+			_dispose() {},
+		},
+		subscribe() {
+			return /* unsubscribe */ noop;
+		},
+		getSnapshot() {
+			return 0;
+		},
+		_start() {},
+		f() {},
+		[symDispose]() {},
+	};
+}
+
+const emptyEffectStore = createEmptyEffectStore();
+
 const _queueMicroTask = Promise.prototype.then.bind(Promise.resolve());
 
 let finalCleanup: Promise<void> | undefined;
@@ -312,7 +339,11 @@ export function _useSignalsImplementation(
 
 	const storeRef = useRef<EffectStore>();
 	if (storeRef.current == null) {
-		storeRef.current = createEffectStore(_usage);
+		if (typeof window === "undefined") {
+			storeRef.current = emptyEffectStore;
+		} else {
+			storeRef.current = createEffectStore(_usage);
+		}
 	}
 
 	const store = storeRef.current;

packages/react/runtime/test/browser/useSignals.test.tsx

@@ -453,6 +453,32 @@ describe("useSignals", () => {
 		expect(scratch.innerHTML).to.equal("<div>Hello John!</div>");
 	});
 
+	it("should clean up signal dependencies after unmounting", async () => {
+		const getTargets = (s: any): any => s.t ?? s._targets;
+
+		const sig = signal(0);
+		function App() {
+			const effectStore = useSignals(/* MANAGED_COMPONENT */ 1);
+			try {
+				return <p>{sig.value}</p>;
+			} finally {
+				effectStore.f();
+			}
+		}
+
+		expect(getTargets(sig)).to.be.undefined;
+
+		await render(<App />);
+		expect(scratch.innerHTML).to.equal("<p>0</p>");
+		expect(getTargets(sig)).to.be.not.null.and.not.undefined;
+
+		act(() => root.unmount());
+		expect(scratch.innerHTML).to.equal("");
+
+		await Promise.resolve();
+		expect(getTargets(sig)).to.be.undefined;
+	});
+
 	// TODO: Figure out what to do here...
 	it.skip("(managed) should work with components that use render props", async () => {
 		function AutoFocusWithin({

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

@@ -1,4 +1,5 @@
 import { signal, useSignalEffect } from "@preact/signals-react";
+import { useSignals } from "@preact/signals-react/runtime";
 import { createElement } from "react";
 import { renderToStaticMarkup } from "react-dom/server";
 import { mountSignalsTests } from "../shared/mounting";
@@ -19,4 +20,26 @@ describe("renderToStaticMarkup", () => {
 		expect(html).to.equal("<p>foo</p>");
 		expect(spy.called).to.be.false;
 	});
+
+	it("should clean up signal dependencies after executing", async () => {
+		const getTargets = (s: any): any => s.t ?? s._targets;
+
+		const sig = signal(0);
+		function App() {
+			const effectStore = useSignals(/* MANAGED_COMPONENT */ 1);
+			try {
+				return <p>{sig.value}</p>;
+			} finally {
+				effectStore.f();
+			}
+		}
+
+		expect(getTargets(sig)).to.be.undefined;
+
+		const html = renderToStaticMarkup(<App />);
+		expect(html).to.equal("<p>0</p>");
+
+		await Promise.resolve();
+		expect(getTargets(sig)).to.be.undefined;
+	});
 });