-
Notifications
You must be signed in to change notification settings - Fork 48.3k
[compiler] Patch for reactive refs in inferred effect dependencies #32991
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
Inferred effect dependencies and inlined jsx (both experimental features) rely on `InferReactivePlaces` to determine their dependencies. Since adding type inference for phi nodes (#30796), we have been incorrectly inferring stable-typed value blocks (e.g. `props.cond ? setState1 : setState2`) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly
const { children, ref } = t0; | ||
let t1; | ||
if ($[0] !== children) { | ||
if ($[0] !== children || $[1] !== ref) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is what we want -- note that ref is reactive as it comes from props
} else { | ||
t3 = $[3]; | ||
} | ||
useEffect(t3, [derived]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
derived
is now correctly added as a dependency. Prior to this PR, this deps array was empty (playground link)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR fixes the inference of reactive refs in experimental inferred effect dependencies and inline JSX transforms after type inference for phi nodes was added. Key changes include:
- Updating the inline JSX transform expected output to handle both children and ref dependencies.
- Adding new fixtures and expectation files for reactive ref ternary operations.
- Enhancing the reactive places inference by introducing a StableSidemap to account for stable-typed value blocks, and updating HIR utility functions to support stable type container checks.
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/inline-jsx-transform.expect.md | Updated expected output to validate new dependency checks for children and ref. |
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.js | Added fixture for testing reactive refs in ternary operations. |
compiler/packages/babel-plugin-react-compiler/src/tests/fixtures/compiler/infer-effect-dependencies/reactive-ref-ternary.expect.md | Updated expected output accordingly. |
compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts | Revised inference to incorporate stable type checks using a new StableSidemap helper. |
compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts | Added functions to detect stable type containers and evaluate stability for hook calls. |
Comments suppressed due to low confidence (2)
compiler/packages/babel-plugin-react-compiler/src/Inference/InferReactivePlaces.ts:39
- [nitpick] Consider renaming 'StableSidemap' to 'StableSideMap' for improved readability and consistency.
class StableSidemap {
compiler/packages/babel-plugin-react-compiler/src/HIR/HIR.ts:1742
- [nitpick] Consider renaming 'type_' to a more conventional name if possible, unless the underscore is necessary to avoid conflicts with reserved words.
const type_ = id.type;
Inferred effect dependencies and inlined jsx (both experimental features) rely on
InferReactivePlaces
to determine their dependencies.Since adding type inference for phi nodes (#30796), we have been incorrectly inferring stable-typed value blocks (e.g.
props.cond ? setState1 : setState2
) as non-reactive. This fix patches InferReactivePlaces instead of adding a new pass since we want non-reactivity propagated correctly