8000 [Compiler]: Effects with always-new dependencies fire less with memoization applied · Issue #33032 · facebook/react · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

[Compiler]: Effects with always-new dependencies fire less with memoization applied #33032

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
1 of 4 tasks
guillaumebrunerie opened this issue Apr 26, 2025 · 7 comments
Open
1 of 4 tasks

Comments

@guillaumebrunerie
Copy link

What kind of issue is this?

  • React Compiler core (the JS output is incorrect, or your app works incorrectly after optimization)
  • babel-plugin-react-compiler (build issue installing or using the Babel plugin)
  • eslint-plugin-react-compiler (build issue installing or using the eslint plugin)
  • react-compiler-healthcheck (build issue installing or using the healthcheck script)

Link to repro

https://github.com/guillaumebrunerie/react-compiler-bug

Repro steps

  • Start the linked repro with npm run dev.
  • Hover the text, and note that the hover text appears to the right.
  • Uncomment the "use no memo" directive in src/App.tsx and notice that the hover text now appears below.

There are no lint errors, but the compiler causes the component to behave differently, so I guess it’s a bug in the compiler?

I noticed this issue when setting up the React compiler in one of my projects, and noticed that some components started behaving incorrectly. It is custom code that implements anchor-like positioning in JS, so somewhat messy code with refs, layout effects, and calculating the size and position of DOM elements. Suddenly one on the popups ended up positioned at (0, 0) with the compiler active, and adding "use no memo" in the affected component made the issue go away.

The repro here is a simplified version, which still behaves differently with and without the compiler. I am not aware of breaking any rules of React, but it is very possible I’m hitting some undefined behavior in the order in which effects run or similar.

I couldn’t simplify it much further, in particular

  • removing useHover to always return true makes the issue go away,
  • replacing the refContainer by the ref itself makes the issue go away,
  • removing hoverRef makes the issue go away.

How often does this bug happen?

Every time

What version of React are you using?

19.1.0

What version of React Compiler are you using?

19.1.0-rc.1

@guillaumebrunerie guillaumebrunerie added Component: Optimizing Compiler Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug Type: Bug labels Apr 26, 2025
@guillaumebrunerie
Copy link
Author
guillaumebrunerie commented Apr 26, 2025

I think I understand why it behaves differently. What I have is basically:

const App = () => {
    const [isHovered, setIsHovered] = useState(false);
    const ref = useRef();
    useEffect(() => {
        /* [...] */
    }, [{ ref }]);
}

In the unoptimized version, every time the hover state changes it triggers a rerender, which creates a new { ref } object, which triggers the effect.
In the optimized version, the { ref } object is memoized (as ref never changes), so the effect is never retriggered (and the position of the hover text is not updated).

So I guess the issue is that I’m using an effect incorrectly, but I still wonder if it’s something the compiler should be able to detect and skip the compilation of that component?

@suhaotian
Copy link
suhaotian commented Apr 29, 2025

Put isHovered as the dependency here, because re-render is triggered by setState, but you use a trick (creating a new object every time) to make useEffect work.

8000

@guillaumebrunerie
Copy link
Author

Put isHovered as the dependency here, because re-render is triggered by setState, but you use a trick (creating a new object every time) to make useEffect work.

Yes, I know that would work, but that is not the point of this issue. The issue is that enabling the compiler silently breaks my app rather than skipping over the problematic component.

It's also a very fragile solution, because I would need to add every other state or things that could trigger a rerender in the dependency array. A better fix would be to implement the effect in a way that it doesn't depend on rerenders.

@josephsavona
Copy link
Contributor

Thanks for posting! This is an interesting example to consider.

So I guess the issue is that I’m using an effect incorrectly, but I still wonder if it’s something the compiler should be able to detect and skip the compilation of that component?

This looks like the issue. Dependencies are meant to be values that are also referenced in the effect body, and a newly created value as a dependency will cause the effect to always re-run. This is something we should be able to detect in the compiler but haven't implemented yet.

@josephsavona josephsavona changed the title [Compiler Bug]: Code with refs behaving differently after optimization [Compiler]: Effects with always-new dependencies fire less with memoization applied Apr 29, 2025
@josephsavona josephsavona added Type: Feature Request and removed Type: Bug Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug labels Apr 29, 2025
@josephsavona
Copy link
Contributor

Concretely, the feature request here is to detect that dependencies are invalid — always recreated — and skip memoization.

@szhsin
Copy link
szhsin commented Apr 29, 2025

Yes, I know that would work, but that is not the point of this issue. The issue is that enabling the compiler silently breaks my app rather than skipping over the problematic component.

It's also a very fragile solution, because I would need to add every other state or things that could trigger a rerender in the dependency array. A better fix would be to implement the effect in a way that it doesn't depend on rerenders.

Sounds like you want an ESLint rule to warn about incorrect usage, such as placing [{ ref }] in the dependency array.

Also, keep in mind that the dependency array is an optional argument in useEffect. If you don't provide it, the effect will run on every render. This will ensure the compiler not to break your code in this case.

@guillaumebrunerie
Copy link
Author

In my actual use case I didn't put { ref } directly in a dependency array, but it was abstracted away in a custom hook:

const useMyCustomHook = (someValue) => {
  useEffect(() => {
    // ... uses someValue
 }, [someValue]);
};

// In a different file
const MyComponent = () => {
  const ref = useRef();
  useMyCustomHook({ ref, someOtherData });
};

I'm not even quite sure which part of this code is problematic. Is it the custom hook that should not pass an object to a dependency array? What makes matters worse is that the type of someValue is a discriminated union, so I can't really just inline all the properties of it in the dependency array without TypeScript complaining.

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

No branches or pull requests

4 participants
0