Improve mixing useSignals called in hooks and components together (#459)

This PR adds tests and supported for nested `useSignals` calls when combined in components & hooks. We generally have two different mechanisms to call useSignals: 1) using react-transform & 2) calling `useSignals()` directly in a component or hook.

To support mixing and matching these scenarios, I've added parameter to `useSignals()` that specifies how this invocation is being used. It primarily exists for the transform to tell `useSignals` it is gonna be manually closed by the code the transform emits.

Using this parameter, when I new store starts, it can examine the previous store and properly handle closing it out or "capture and restoring it" once the current store finishes. See the comment in `runtime/src/index.ts:_start()` for a detailed description of the exact behavior here.

Commits:

* === BEGIN fix-signal-text-2 === Add transform test for signal as text

* Add additional test for running nested try/finallys

* Add some sample test cases to think about in regards to nested useSignals usage

* Add notes about states useSignals calls may transition between

* Add more useSignals tests to go and fix

* Add test ids for generated tests

* Add test for using signals with components that use render props

* Add test for out-of-order effect error in React 16

* Simplify test case a bit

* Update test again to present passing state

* Add notes about possible fix for out-or-order effect

* Encapsulate set/clearCurrentStore into store instance methods

* Implement first pass of handling unmanaged and managed transitions

* Simplify start logic using methods

* Update comments

* Temporarily skip test to fix later

* Update transform to emit usage enum to useSignals

* Remove test id stuff

* Update SignalValue to pass in effect store usage

* Add useSignals usage to auto adapter

* Wrap custom hooks in try/finally

Update transform tests to specify usage

* Re-enable previously failing test

* Ignore React 16 warning about useLayoutEffect on
the server

* Improve usage explanation

.changeset/mighty-crews-burn.md

@@ -0,0 +1,5 @@
+---
+"@preact/signals-react-transform": minor
+---
+
+Wrap custom hooks in try/finally when using react-transform

packages/react-transform/src/index.ts

@@ -25,6 +25,14 @@ const maybeUsesSignal = "maybeUsesSignal";
 const containsJSX = "containsJSX";
 const alreadyTransformed = "alreadyTransformed";
 
+const UNMANAGED = "0";
+const MANAGED_COMPONENT = "1";
+const MANAGED_HOOK = "2";
+type HookUsage =
+	| typeof UNMANAGED
+	| typeof MANAGED_COMPONENT
+	| typeof MANAGED_HOOK;
+
 const logger = {
 	transformed: debug("signals:react-transform:transformed"),
 	skipped: debug("signals:react-transform:skipped"),
@@ -83,15 +91,15 @@ function getObjectPropertyKey(
  * If the function node has a name (i.e. is a function declaration with a
  * name), return that. Else return null.
  */
-function getFunctionNodeName(path: NodePath<FunctionLike>): string | null {
+function getFunctionNodeName(node: FunctionLike): string | null {
 	if (
-		(path.node.type === "FunctionDeclaration" ||
-			path.node.type === "FunctionExpression") &&
-		path.node.id
+		(node.type === "FunctionDeclaration" ||
+			node.type === "FunctionExpression") &&
+		node.id
 	) {
-		return path.node.id.name;
-	} else if (path.node.type === "ObjectMethod") {
-		return getObjectPropertyKey(path.node);
+		return node.id.name;
+	} else if (node.type === "ObjectMethod") {
+		return getObjectPropertyKey(node);
 	}
 
 	return null;
@@ -156,7 +164,7 @@ function getFunctionNameFromParent(
 function getFunctionName(
 	path: NodePath<FunctionLike>
 ): string | typeof DefaultExportSymbol | null {
-	let nodeName = getFunctionNodeName(path);
+	let nodeName = getFunctionNodeName(path.node);
 	if (nodeName) {
 		return nodeName;
 	}
@@ -284,7 +292,7 @@ function isValueMemberExpression(
 	);
 }
 
-const tryCatchTemplate = template.statements`var STORE_IDENTIFIER = HOOK_IDENTIFIER();
+const tryCatchTemplate = template.statements`var STORE_IDENTIFIER = HOOK_IDENTIFIER(HOOK_USAGE);
 try {
 	BODY
 } finally {
@@ -294,7 +302,8 @@ try {
 function wrapInTryFinally(
 	t: typeof BabelTypes,
 	path: NodePath<FunctionLike>,
-	state: PluginPass
+	state: PluginPass,
+	hookUsage: HookUsage
 ): FunctionLike {
 	const stopTrackingIdentifier = path.scope.generateUidIdentifier("effect");
 
@@ -303,6 +312,7 @@ function wrapInTryFinally(
 		tryCatchTemplate({
 			STORE_IDENTIFIER: stopTrackingIdentifier,
 			HOOK_IDENTIFIER: get(state, getHookIdentifier)(),
+			HOOK_USAGE: hookUsage,
 			BODY: t.isBlockStatement(path.node.body)
 				? path.node.body.body // TODO: Is it okay to elide the block statement here?
 				: t.returnStatement(path.node.body),
@@ -340,19 +350,20 @@ function transformFunction(
 	functionName: string | null,
 	state: PluginPass
 ) {
+	const isHook = isCustomHookName(functionName);
+	const isComponent = isComponentName(functionName);
+	const hookUsage = options.experimental?.noTryFinally
+		? UNMANAGED
+		: isHook
+		? MANAGED_HOOK
+		: isComponent
+		? MANAGED_COMPONENT
+		: UNMANAGED;
+
 	let newFunction: FunctionLike;
-	if (isCustomHookName(functionName) || options.experimental?.noTryFinally) {
-		// For custom hooks, we don't need to wrap the function body in a
-		// try/finally block because later code in the function's render body could
-		// read signals and we want to track and associate those signals with this
-		// component. The try/finally in the component's body will stop tracking
-		// signals for us instead.
-		newFunction = prependUseSignals(t, path, state);
-	} else if (isComponentName(functionName)) {
-		newFunction = wrapInTryFinally(t, path, state);
+	if (hookUsage !== UNMANAGED) {
+		newFunction = wrapInTryFinally(t, path, state, hookUsage);
 	} else {
-		// Since we can't determine if this function is a component/hook or not,
-		// we'll just prepend the useSignals call so it will work as either
 		newFunction = prependUseSignals(t, path, state);
 	}
 

packages/react-transform/test/browser/e2e.test.tsx

@@ -66,6 +66,22 @@ describe("React Signals babel transfrom - browser E2E tests", () => {
 		checkHangingAct();
 	});
 
+	it("should rerender components when using signals as text", async () => {
+		const { App } = await createComponent(`
+			export function App({ name }) {
+				return <div>Hello {name}</div>;
+			}`);
+
+		const name = signal("John");
+		await render(<App name={name} />);
+		expect(scratch.innerHTML).to.equal("<div>Hello John</div>");
+
+		await act(() => {
+			name.value = "Jane";
+		});
+		expect(scratch.innerHTML).to.equal("<div>Hello Jane</div>");
+	});
+
 	it("should rerender components when signals they use change", async () => {
 		const { App } = await createComponent(`
 			export function App({ name }) {
@@ -374,6 +390,47 @@ describe("React Signals babel transfrom - browser E2E tests", () => {
 		expect(scratch.innerHTML).to.equal("<div>Hello Jane</div>");
 	});
 
+	it("should work when an ambiguous function is manually transformed and used as a hook", async () => {
+		// TODO: Warn/Error when manually opting in ambiguous function into transform
+		const { App, greeting, name } = await createComponent(`
+			import { signal } from "@preact/signals-core";
+
+			export const greeting = signal("Hello");
+			export const name = signal("John");
+
+			// Ambiguous if this function is gonna be a hook or component
+			/** @trackSignals */
+			function usename() {
+				return name.value;
+			}
+
+			export function App() {
+				const name = usename();
+				return <div>{greeting.value} {name}</div>;
+			}`);
+
+		await render(<App name={name} />);
+		expect(scratch.innerHTML).to.equal("<div>Hello John</div>");
+
+		await act(() => {
+			greeting.value = "Hi";
+		});
+		expect(scratch.innerHTML).to.equal("<div>Hi John</div>");
+
+		await act(() => {
+			name.value = "Jane";
+		});
+		expect(scratch.innerHTML).to.equal("<div>Hi Jane</div>");
+
+		await act(() => {
+			batch(() => {
+				greeting.value = "Hello";
+				name.value = "John";
+			});
+		});
+		expect(scratch.innerHTML).to.equal("<div>Hello John</div>");
+	});
+
 	it("loads useSignals from a custom source", async () => {
 		const { App } = await createComponent(
 			`