Skip to content

Conversation

@andrewiggins
Copy link
Member

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:

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.

…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.
@changeset-bot
Copy link

changeset-bot bot commented Jan 15, 2026

⚠️ No Changeset found

Latest commit: 393b713

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@netlify
Copy link

netlify bot commented Jan 15, 2026

Deploy Preview for preact-signals-demo ready!

Name Link
🔨 Latest commit 393b713
🔍 Latest deploy log https://app.netlify.com/projects/preact-signals-demo/deploys/69689e2900dd1d000810444f
😎 Deploy Preview https://deploy-preview-814--preact-signals-demo.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@github-actions
Copy link
Contributor

Size Change: +1.03 kB (+0.98%)

Total Size: 107 kB

Filename Size Change
packages/react-transform/dist/signals-*********.js 7.24 kB +345 B (+5%) 🔍
packages/react-transform/dist/signals-transform.mjs 6.44 kB +344 B (+5.64%) 🔍
packages/react-transform/dist/signals-transform.umd.js 7.35 kB +344 B (+4.91%) 🔍
ℹ️ View Unchanged
Filename Size
docs/dist/assets/bench.********.js 1.59 kB
docs/dist/assets/client.********.js 46.2 kB
docs/dist/assets/devtools.********.js 1.03 kB
docs/dist/assets/index.********.js 7.04 kB
docs/dist/assets/jsxRuntime.module.********.js 297 B
docs/dist/assets/preact.module.********.js 4.76 kB
docs/dist/assets/signals-core.module.********.js 1.56 kB
docs/dist/assets/signals.module.********.js 2.57 kB
docs/dist/assets/style.********.css 1.27 kB
docs/dist/assets/style.********.js 21 B
docs/dist/basic-********.js 246 B
docs/dist/nesting-********.js 1.13 kB
docs/dist/react-********.js 242 B
packages/core/dist/signals-core.js 1.59 kB
packages/core/dist/signals-core.mjs 1.6 kB
packages/debug/dist/debug.js 3.64 kB
packages/debug/dist/debug.mjs 3.18 kB
packages/preact-transform/dist/signals-*********.js 1.3 kB
packages/preact-transform/dist/signals-transform.mjs 1.29 kB
packages/preact-transform/dist/signals-transform.umd.js 1.42 kB
packages/preact/dist/signals.js 1.71 kB
packages/preact/dist/signals.mjs 1.67 kB
packages/react/dist/signals.js 188 B
packages/react/dist/signals.mjs 150 B

compressed-size-action

@andrewiggins
Copy link
Member Author

Hmm all but one test is passing locally... Not sure why there are more failures here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants