-
Notifications
You must be signed in to change notification settings - Fork 48.4k
[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
Comments
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 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? |
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. |
Thanks for posting! This is an interesting example to consider.
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. |
Concretely, the feature request here is to detect that dependencies are invalid — always recreated — and skip memoization. |
Sounds like you want an ESLint rule to warn about incorrect usage, such as placing Also, keep in mind that the dependency array is an optional argument in |
In my actual use case I didn't put 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 |
What kind of issue is this?
Link to repro
https://github.com/guillaumebrunerie/react-compiler-bug
Repro steps
npm run dev
."use no memo"
directive insrc/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
useHover
to always returntrue
makes the issue go away,refContainer
by the ref itself makes the issue go away,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
The text was updated successfully, but these errors were encountered: