-
Notifications
You must be signed in to change notification settings - Fork 47.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 Hooks Plugin] false positive with useCallback and not inline function body #18833
Comments
The purpose of the linter is to verify that you don't have stale closures. If you pass invisible |
@gaearon if I understand your feedback correctly this is not about the function being passsed to const useFoo = <R>(selector: (state: S) => R) => {
const memoizedSelector = useCallback(selector, [])
// React Hook useCallback has a missing dependency: 'selector'. Either include it or remove the dependency array react-hooks/exhaustive-deps
...
} This yields the same error. Do you mind elaborating? |
Hmm. I don’t understand your question. Maybe we can do this backwards. What do you expect the linter to help you with? |
I don't understand why the linter is complaining about the function If I put it there, the hook will re-run every time, since the function is inlined upstream, and I don't want that obviously. In general, If I am writing my own hook that internally calls |
Consider this situation. function Button({ onPressChange, isPressed }) {
useEffect(onPressChange, [isPressed]);
} In your opinion, should this warn or not? |
no, I don't think so <Button onPressChange={() => console.log('pressed-change')} /> what am I missing? |
To be more precise, I understand how function Button({ onPressChange, isPressed }) {
useEffect(onPressChange, [isPressed]);
} could trigger a warning considering the inability to statically analyze What I am referring to here, given this example: const useUseCallback = (fn, deps) => {
return useCallback(fn, deps)
// React Hook useCallback has a missing dependency: 'fn'. Either include it or remove the dependency array react-hooks/exhaustive-deps
}
The warning is telling me there is a fix I need to apply to my code when In reality, given your feedback, the only path forward is disabling eslint for that line, am I correct? Why is this code passing linting? const useUseCallback = (fn, deps) => {
return useCallback(fn, [...deps, fn])
} |
I just received
eslint-plugin-react-hooks@4.0
through dependabot, and I am a little confused by the error and the explanation in this PR #18435. I might be wrong but I don't think I should be getting a lint warning/error.My my
selector
function changes every time, and adding it to thedependencies
list defeats the whole purpose of useCallback in the first place. Maybe I am missing something here.I tried to create a codesandbox, but apparently custom eslint configs are not supported. Let me know what I can doo to help.
This is the relevant code: https://github.com/kilianc/mozzarella/blob/master/src/create-store.ts#L45-L70
This is the PR that complaints: kilianc/mozzarella#8
This is the error from github actions: https://github.com/kilianc/mozzarella/runs/644435406?check_suite_focus=true
The text was updated successfully, but these errors were encountered: