-
-
Notifications
You must be signed in to change notification settings - Fork 119
fix(react-transform): Fix JSX detection leaking to sibling functions #814
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
…data. We only want to look at the specific function node for containsJSX or maybeUsesSignals
…nt or custom hook or return null Previously it would the last function it found, but that contributed to the bug since another path could see that and think it needed to be transformed even though it wasn't in a component or custom hook.
|
✅ Deploy Preview for preact-signals-demo ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Size Change: +1.03 kB (+0.98%) Total Size: 107 kB
ℹ️ View Unchanged
|
|
Hmm all but one test is passing locally... Not sure why there are more failures here. |
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:
The
incrementmethod insideCountModelwas being incorrectly transformed because the plugin detected it "contained JSX" due to the siblingComponentfunction.When This Bug Manifests
This bug requires the component to be nested inside another function scope. The parent function scope is where the
containsJSXflag 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()orit()blocks—which is how I discovered the bug.Root Cause
The bug was multi-faceted:
Function search logic was checking
containsJSXbefore it was set: TheisComponentFunctionhelper expectedcontainsJSXto 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 withcontainsJSX, the current function wouldn't be recognized as a component, causing the search to continue upward.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.Scope data inheritance: The critical issue was that
containsJSXandmaybeUsesSignalwere being set onscoperather than on the function node itself. Babel'sscope.getData()searches parent scopes for data. This meant that when we setcontainsJSXon 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 usingpath.setData()/path.getData()directly on the function node paths. This ensures thatcontainsJSXandmaybeUsesSignalflags are isolated to the specific function that actually contains JSX or signal usage, preventing leakage to sibling functions.Refactored
getComponentFunctionDeclarationtofindParentComponentOrHookThis function is mostly the same but with a couple bug fixes:
containsJSXwhich may not be set yetnullinstead of the last seen function.Moved
isComponentFunctioncheck insideshouldTransformSince
isComponentFunctionneeds to checkcontainsJSX, it should only be called after the function body has been fully traversed. Moving it insideshouldTransform(which is called on function exit) ensures the data is available.Additional Improvements
getFunctionNamenow handles default exports: The function now takes afilenameparameter and resolvesDefaultExportSymbolto the filename internally, simplifying call sites.Added
isCustomHookCallbackhelper: 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:verbosedebug namespace for more detailed logging during development and debugging.Test infrastructure improvements: Added ability to set
DEBUG_TEST_IDS = falseto skip all generated tests when debugging specific test cases.