Skip to content

Commit a5d0a6e

Browse files
andrewigginsandrewiggins
authored
fix(react-transform): Fix JSX detection leaking to sibling functions (#814)
# Fix: JSX detection leaking outside of component scope ## The Bug The Babel transform was incorrectly transforming non-component functions that used signals when a sibling component contained JSX. For example, in the following code: ```js function wrapper() { function Component() { return <div>Hello</div>; } const CountModel = createModel(() => ({ count: signal(0), increment() { this.count.value++; // Uses .value }, })); } ``` The `increment` method inside `CountModel` was being incorrectly transformed because the plugin detected it "contained JSX" due to the sibling `Component` function. ### When This Bug Manifests This bug requires the component to be nested inside another function scope. The parent function scope is where the `containsJSX` flag would be incorrectly stored, allowing sibling functions to find it when searching their scope chain. For most application code where components are declared at the module's root scope, this bug wouldn't manifest since there's no parent function scope to hold the leaked flag. However, this is a common pattern in **test files** where components and helper functions are declared inside `describe()` or `it()` blocks—which is how I discovered the bug. ### Root Cause The bug was multi-faceted: 1. **Function search logic was checking `containsJSX` before it was set**: The `isComponentFunction` helper expected `containsJSX` to already be set on the scope, but it was being called during the function path traversal that would set it. So when looking for a component candidate to mark with `containsJSX`, the current function wouldn't be recognized as a component, causing the search to continue upward. 2. **Fallback to highest-level component**: When searching for a component candidate, the setter function would fallback to the highest level component-like function, which in some scenarios (e.g., test suites with `describe()`) isn't a valid component. 3. **Scope data inheritance**: The critical issue was that `containsJSX` and `maybeUsesSignal` were being set on `scope` rather than on the function node itself. Babel's `scope.getData()` searches parent scopes for data. This meant that when we set `containsJSX` on a parent scope, sibling functions in that same scope would also find that data when querying their own scope, making them appear to "contain JSX" when they didn't. ## The Fix ### Core Change: Store data on function nodes, not scopes Changed from using `scope.setData()`/`scope.getData()` to using `path.setData()`/`path.getData()` directly on the function node paths. This ensures that `containsJSX` and `maybeUsesSignal` flags are isolated to the specific function that actually contains JSX or signal usage, preventing leakage to sibling functions. ### Refactored `getComponentFunctionDeclaration` to `findParentComponentOrHook` This function is mostly the same but with a couple bug fixes: - Find and return the parent component or custom hook function path (not scope) - Check if a function is a component or hook based on its name, rather than relying on `containsJSX` which may not be set yet - If a matching component or hook function is now found, return `null` instead of the last seen function. ### Moved `isComponentFunction` check inside `shouldTransform` Since `isComponentFunction` needs to check `containsJSX`, it should only be called after the function body has been fully traversed. Moving it inside `shouldTransform` (which is called on function exit) ensures the data is available. ## Additional Improvements - **`getFunctionName` now handles default exports**: The function now takes a `filename` parameter and resolves `DefaultExportSymbol` to the filename internally, simplifying call sites. - **Added `isCustomHookCallback` helper**: New utility function that detects when a function is being passed as a parameter to a custom hook call (e.g., `useCustomHook(() => { ... })`). - **Added verbose logging**: New `signals:react-transform:verbose` debug namespace for more detailed logging during development and debugging. - **Test infrastructure improvements**: Added ability to set `DEBUG_TEST_IDS = false` to skip all generated tests when debugging specific test cases.
1 parent de905d2 commit a5d0a6e

4 files changed

Lines changed: 273 additions & 128 deletions

File tree

‎.changeset/proud-tomatoes-flow.md‎

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,22 @@
1+
---
2+
"@preact/signals-react-transform": patch
3+
---
4+
5+
Fix JSX detection leaking to non-component functions in the same scope
6+
7+
Previously, when a component containing JSX was defined inside another function, the JSX detection could incorrectly "leak" to sibling functions or the parent function, causing non-components to be transformed. This was especially problematic in test files where components are defined inside `it()` or `describe()` blocks.
8+
9+
```js
10+
describe("suite", () => {
11+
it("test", () => {
12+
// This arrow function was incorrectly transformed because
13+
// Counter's JSX detection leaked to sibling functions
14+
const CountModel = () => signal.value;
15+
function Counter() {
16+
return <div>Hello</div>;
17+
}
18+
});
19+
});
20+
```
21+
22+
The transform now correctly scopes JSX and signal usage detection to only the containing component or custom hook function.

‎packages/react-transform/src/index.ts‎

Lines changed: 126 additions & 89 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ import {
77
template,
88
} from "@babel/core";
99
import { isModule, addNamed } from "@babel/helper-module-imports";
10-
import type { Scope, VisitNodeObject } from "@babel/traverse";
10+
import type { VisitNodeObject } from "@babel/traverse";
1111
import debug from "debug";
1212

1313
interface PluginArgs {
@@ -36,6 +36,8 @@ type HookUsage =
3636
| typeof MANAGED_HOOK;
3737

3838
const logger = {
39+
verbose: debug("signals:react-transform:verbose"),
40+
fnSearch: debug("signals:react-transform:fn-search"),
3941
transformed: debug("signals:react-transform:transformed"),
4042
skipped: debug("signals:react-transform:skipped"),
4143
};
@@ -45,57 +47,75 @@ const get = (pass: PluginPass, name: any) =>
4547
const set = (pass: PluginPass, name: string, v: any) =>
4648
pass.set(`${dataNamespace}/${name}`, v);
4749

48-
interface DataContainer {
49-
getData(name: string): any;
50-
setData(name: string, value: any): void;
51-
}
52-
const setData = (node: DataContainer, name: string, value: any) =>
50+
const setNodeData = (node: NodePath<unknown>, name: string, value: any) =>
5351
node.setData(`${dataNamespace}/${name}`, value);
54-
const getData = (node: DataContainer, name: string) =>
52+
const getNodeData = (node: NodePath<unknown>, name: string) =>
5553
node.getData(`${dataNamespace}/${name}`);
5654

57-
function getComponentFunctionDeclaration(
55+
/**
56+
* Returns the containing component or hook function path. Examples:
57+
* ```
58+
* function App() { <- returns this path
59+
* <div>{signal.value}</div> <- starting from this
60+
* }
61+
*
62+
* function useCustomHook() { <- returns this path
63+
* <div>{signal.value}</div> <- starting from this
64+
* }
65+
* ```
66+
*
67+
* It will return `null` if the function is passed as a parameter
68+
* to a custom hook function. Example:
69+
* ```
70+
* function Component() {
71+
* useCustomHook(() => {
72+
* <div>{signal.value}</div> <- returns null
73+
* });
74+
* }
75+
* ```
76+
*/
77+
function findParentComponentOrHook(
5878
path: NodePath,
59-
filename: string | undefined,
60-
prev?: Scope
61-
): Scope | null {
62-
const functionScope = path.scope.getFunctionParent();
63-
64-
if (functionScope) {
65-
const parent = functionScope.path.parent;
66-
let functionName = getFunctionName(functionScope.path as any);
67-
if (functionName === DefaultExportSymbol) {
68-
functionName = filename || null;
69-
}
70-
if (isComponentFunction(functionScope.path as any, functionName)) {
71-
return functionScope;
72-
} else if (
73-
parent.type === "CallExpression" &&
74-
parent.callee.type === "Identifier" &&
75-
parent.callee.name.startsWith("use") &&
76-
parent.callee.name[3] === parent.callee.name[3].toUpperCase()
77-
) {
78-
return null;
79-
}
80-
return getComponentFunctionDeclaration(
81-
functionScope.parent.path,
82-
filename,
83-
functionScope
84-
);
85-
} else {
86-
return prev || null;
79+
filename: string | undefined
80+
): NodePath<FunctionLike> | null {
81+
const parentFunctionScope = path.scope.getFunctionParent();
82+
if (!parentFunctionScope) {
83+
logger.fnSearch("No higher function scope found in %s", filename);
84+
return null;
8785
}
86+
87+
const parentFunctionPath = parentFunctionScope.path as NodePath<FunctionLike>;
88+
const fnName = getFunctionName(parentFunctionPath, filename);
89+
logger.fnSearch('Checking parent function "%s" in %s', fnName, filename);
90+
91+
if (isComponentName(fnName) || isCustomHookName(fnName)) {
92+
logger.fnSearch('Found parent function "%s" in %s', fnName, filename);
93+
return parentFunctionPath;
94+
} else if (isCustomHookCallback(parentFunctionPath)) {
95+
logger.fnSearch('Function "%s" is a hook callback arg, stopping', fnName);
96+
return null;
97+
} else if (!parentFunctionPath.parentPath) {
98+
logger.fnSearch('Function "%s" has no parent, stopping', fnName, filename);
99+
return null;
100+
}
101+
102+
return findParentComponentOrHook(parentFunctionPath.parentPath, filename);
88103
}
89104

90-
function setOnFunctionScope(
105+
function setOnParentComponentOrHook(
91106
path: NodePath,
92107
key: string,
93108
value: any,
94109
filename: string | undefined
95110
) {
96-
const functionScope = getComponentFunctionDeclaration(path, filename);
97-
if (functionScope) {
98-
setData(functionScope, key, value);
111+
const parentFn = findParentComponentOrHook(path, filename);
112+
if (parentFn) {
113+
if (logger.verbose.enabled) {
114+
const fnName = getFunctionName(parentFn, filename);
115+
logger.verbose(`Setting "${key}" on "${fnName}" to "${value}"`);
116+
}
117+
118+
setNodeData(parentFn, key, value);
99119
}
100120
}
101121

@@ -202,14 +222,22 @@ function getFunctionNameFromParent(
202222

203223
/* Determine the name of a function */
204224
function getFunctionName(
205-
path: NodePath<FunctionLike>
206-
): string | typeof DefaultExportSymbol | null {
207-
let nodeName = getFunctionNodeName(path.node);
208-
if (nodeName) {
209-
return nodeName;
225+
path: NodePath<FunctionLike>,
226+
filename: string | undefined
227+
): string | null {
228+
let fnName: string | null = getFunctionNodeName(path.node);
229+
if (fnName) {
230+
return fnName;
231+
}
232+
233+
const nameFromParent = getFunctionNameFromParent(path.parentPath);
234+
if (nameFromParent === DefaultExportSymbol) {
235+
fnName = basename(filename) ?? null;
236+
} else {
237+
fnName = nameFromParent;
210238
}
211239

212-
return getFunctionNameFromParent(path.parentPath);
240+
return fnName;
213241
}
214242

215243
function isComponentName(name: string | null): boolean {
@@ -219,6 +247,16 @@ function isCustomHookName(name: string | null): boolean {
219247
return name?.match(/^use[A-Z]/) != null;
220248
}
221249

250+
/** Returns if the given function path is a parameter passed to a custom hook function */
251+
function isCustomHookCallback(path: NodePath<FunctionLike>): boolean {
252+
const parent = path.parent;
253+
return (
254+
parent.type === "CallExpression" &&
255+
parent.callee.type === "Identifier" &&
256+
isCustomHookName(parent.callee.name)
257+
);
258+
}
259+
222260
function hasLeadingComment(path: NodePath, comment: RegExp): boolean {
223261
const comments = path.node.leadingComments;
224262
return comments?.some(c => c.value.match(comment) !== null) ?? false;
@@ -286,21 +324,23 @@ function isOptedOutOfSignalTracking(path: NodePath | null): boolean {
286324
}
287325
}
288326

289-
function isComponentFunction(
290-
path: NodePath<FunctionLike>,
291-
functionName: string | null
292-
): boolean {
293-
return (
294-
getData(path.scope, containsJSX) === true && // Function contains JSX
295-
isComponentName(functionName) // Function name indicates it's a component
296-
);
297-
}
298-
299327
function shouldTransform(
300328
path: NodePath<FunctionLike>,
301329
functionName: string | null,
302330
options: PluginOptions
303331
): boolean {
332+
// This function should only be called after a function's body has been parsed
333+
// and containsJSX and maybeUsesSignal could be set
334+
function isComponentFunction(
335+
path: NodePath<FunctionLike>,
336+
functionName: string | null
337+
): boolean {
338+
return (
339+
getNodeData(path, containsJSX) === true && // Function contains JSX
340+
isComponentName(functionName) // Function name indicates it's a component
341+
);
342+
}
343+
304344
// Opt-out takes first precedence
305345
if (isOptedOutOfSignalTracking(path)) return false;
306346
// Opt-in opts in to transformation regardless of mode
@@ -312,7 +352,7 @@ function shouldTransform(
312352

313353
if (options.mode == null || options.mode === "auto") {
314354
return (
315-
getData(path.scope, maybeUsesSignal) === true && // Function appears to use signals;
355+
getNodeData(path, maybeUsesSignal) === true && // Function appears to use signals;
316356
(isComponentFunction(path, functionName) ||
317357
isCustomHookName(functionName))
318358
);
@@ -556,8 +596,7 @@ function transformFunction(
556596
options: PluginOptions,
557597
path: NodePath<FunctionLike>,
558598
functionName: string | null,
559-
state: PluginPass,
560-
filename: string
599+
state: PluginPass
561600
) {
562601
const isHook = isCustomHookName(functionName);
563602
const isComponent = isComponentName(functionName);
@@ -583,7 +622,7 @@ function transformFunction(
583622
newBody = prependUseSignals(t, path, state, options, functionName);
584623
}
585624

586-
setData(path, alreadyTransformed, true);
625+
setNodeData(path, alreadyTransformed, true);
587626
path.get("body").replaceWith(newBody);
588627
}
589628

@@ -725,6 +764,10 @@ function detectJSXAlternativeImports(
725764
},
726765
});
727766

767+
logger.verbose("Using JSX alternatives: %o", {
768+
identifiers: Array.from(jsxIdentifierSet),
769+
objects: Array.from(jsxObjectMap.entries()),
770+
});
728771
set(state, jsxIdentifiers, jsxIdentifierSet);
729772
set(state, jsxObjects, jsxObjectMap);
730773
}
@@ -790,19 +833,12 @@ function log(
790833
logger.transformed(`${functionName} (${relativePath}:${lineNum})`);
791834
} else {
792835
logger.skipped(`${functionName} (${relativePath}:${lineNum}) %o`, {
793-
hasSignals: getData(path.scope, maybeUsesSignal) ?? false,
794-
hasJSX: getData(path.scope, containsJSX) ?? false,
836+
hasSignals: getNodeData(path, maybeUsesSignal) ?? false,
837+
hasJSX: getNodeData(path, containsJSX) ?? false,
795838
});
796839
}
797840
}
798841

799-
function isComponentLike(
800-
path: NodePath<FunctionLike>,
801-
functionName: string | null
802-
): boolean {
803-
return !getData(path, alreadyTransformed) && isComponentName(functionName);
804-
}
805-
806842
export default function signalsTransform(
807843
{ types: t }: PluginArgs,
808844
options: PluginOptions
@@ -814,24 +850,15 @@ export default function signalsTransform(
814850
// babel pass with plugins on components twice.
815851
const visitFunction: VisitNodeObject<PluginPass, FunctionLike> = {
816852
exit(path, state) {
817-
if (getData(path, alreadyTransformed) === true) return false;
818-
819-
let functionName = getFunctionName(path);
820-
if (functionName === DefaultExportSymbol) {
821-
functionName = basename(this.filename) ?? null;
822-
}
853+
if (getNodeData(path, alreadyTransformed) === true) return false;
823854

855+
const functionName = getFunctionName(path, this.filename);
856+
const isComponentLike =
857+
!getNodeData(path, alreadyTransformed) && isComponentName(functionName);
824858
if (shouldTransform(path, functionName, state.opts)) {
825-
transformFunction(
826-
t,
827-
state.opts,
828-
path,
829-
functionName,
830-
state,
831-
this.filename || ""
832-
);
859+
transformFunction(t, state.opts, path, functionName, state);
833860
log(true, path, functionName, this.filename);
834-
} else if (isComponentLike(path, functionName)) {
861+
} else if (isComponentLike) {
835862
log(false, path, functionName, this.filename);
836863
}
837864
},
@@ -872,7 +899,7 @@ export default function signalsTransform(
872899
CallExpression(path, state) {
873900
if (options.detectTransformedJSX) {
874901
if (isJSXAlternativeCall(path, state)) {
875-
setOnFunctionScope(path, containsJSX, true, this.filename);
902+
setOnParentComponentOrHook(path, containsJSX, true, this.filename);
876903
}
877904
}
878905

@@ -892,21 +919,31 @@ export default function signalsTransform(
892919

893920
MemberExpression(path) {
894921
if (isValueMemberExpression(path)) {
895-
setOnFunctionScope(path, maybeUsesSignal, true, this.filename);
922+
setOnParentComponentOrHook(
923+
path,
924+
maybeUsesSignal,
925+
true,
926+
this.filename
927+
);
896928
}
897929
},
898930

899931
ObjectPattern(path) {
900932
if (hasValuePropertyInPattern(path.node)) {
901-
setOnFunctionScope(path, maybeUsesSignal, true, this.filename);
933+
setOnParentComponentOrHook(
934+
path,
935+
maybeUsesSignal,
936+
true,
937+
this.filename
938+
);
902939
}
903940
},
904941

905942
JSXElement(path) {
906-
setOnFunctionScope(path, containsJSX, true, this.filename);
943+
setOnParentComponentOrHook(path, containsJSX, true, this.filename);
907944
},
908945
JSXFragment(path) {
909-
setOnFunctionScope(path, containsJSX, true, this.filename);
946+
setOnParentComponentOrHook(path, containsJSX, true, this.filename);
910947
},
911948
},
912949
};

0 commit comments

Comments
 (0)