Skip to content

[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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mofeiZ
Copy link
Contributor

@mofeiZ mofeiZ commented Apr 22, 2025

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

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) {
Copy link
Contributor Author

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]);
Copy link
Contributor Author

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)

@mofeiZ mofeiZ requested review from Copilot and jbrown215 April 22, 2025 21:09
Copy link

@Copilot Copilot AI left a 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;

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

Successfully merging this pull request may close these issues.

2 participants