-
Notifications
You must be signed in to change notification settings - Fork 46.4k
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
Bug: [eslint-plugin-react-hooks] exhaustive-deps false positive on "unnecessary" dependency if its a React component #18051
Comments
This looks like a legit bug to me. Thank you for reporting! |
I am interested in looking into a fix, I had a look but I think there's something implicit that I'm missing. I'd appreciate a few pointers. |
I tried finding out the location where , it can be solved. So basically
gatherDependenciesRecursively right now is now not able to gather JSXElement as the dependency. currentScope.references and currentScope.through does not contain JSXElement reference. I tried finding out , how we can pick the JSXElement dependency. But no success. Any pointers . @gaearon @zeorin ? |
I’m getting the same error while doing something like this: const useExample = (data) => useMemo(() => ({ ...data, sortBy }), [data, sortBy]) Warning message:
|
Hi! I would like to give this a shot if nobody is already working on it.
provides an empty
is not able to retrieve the I don't know if I am getting the point but I would be happy to try to fix this issue. |
I have made some tests related to the behavior reported by @martinschaer . I have simplified a bit the code to be tested and this is what I am using: function Example({ sortBy }) {
const useExample = () => useMemo(() => console.log(sortBy), [sortBy])
} According to what I have understood, this is what happens and sounds valuable to me:
The third point is something I was expecting for, after I noticed this bug, because I supposed that the I am not so sure that this problem and the one related to the JSX component have the same root. If anyone could share his/her opinion I would be glad to proceed with these issues. |
…18051), update yarn.lock
Hi all! |
I noticed right now that this is the same issue discussed in #18937 . |
If you're using |
Great, thank you very much @eps1lon ! |
Hey all, it's worth noting that this seems to affect the React Compiler, the Compiler ESLint plugin will warn about a de-opt if this the exhaustive deps lint rule is suppressed. Current workaround: function Foo({ component: Component }) {
const memoized = useMemo(() => ({
render: () => (Component, <Component />)
}), [Component]);
return memoized.render();
} In case it's not clear what's happening here: I'm using the comma operator to reference |
Steps to reproduce
useMemo
Link to code example: https://github.com/zeorin/eslint-plugin-react-hooks-repro
The current behavior
The expected behavior
No lint errors.
More details
A simple repro (taken from the link above) is:
Workarounds
If one changes the component to lowercase, the lint error goes away. It does also mean that we need to change the way we render the component:
Alternatively we can decide not to use JSX, in which case the lint rule functions correctly, too:
Impact
Currently it is hard to use props that are components in a JSX expression if one is using the
exhaustive-deps
rule.This is also compounded by the fact that this rule has a ESLint fix that removes the dependency, thus changing the behaviour of the code and leading to bugs. See #16313 for that bug report.
The text was updated successfully, but these errors were encountered: