Skip to content
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

Closed
kilianc opened this issue May 5, 2020 · 7 comments
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug

Comments

@kilianc
Copy link

kilianc commented May 5, 2020

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.

const useFoo = <R>(
  selector: (state: S) => R,
  dependencies: DependencyList = []
) => {
  const memoizedSelector = useCallback(selector, dependencies)
  // React Hook useCallback has a missing dependency: 'selector'. Either include it or remove the dependency array  react-hooks/exhaustive-deps
  ...
}

My my selector function changes every time, and adding it to the dependencies 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

@kilianc kilianc added the Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug label May 5, 2020
@gaearon
Copy link
Collaborator

gaearon commented May 5, 2020

The purpose of the linter is to verify that you don't have stale closures. If you pass invisible selector and dependencies we can't verify that. You can add an ignore there if you're convinced the deps are correct, but the warning itself is right — it says something may be unsafe. E.g. in your example, nothing will actually verify that useSelector receives correct deps.

@gaearon gaearon closed this as completed May 5, 2020
@kilianc
Copy link
Author

kilianc commented May 5, 2020

@gaearon if I understand your feedback correctly this is not about the function being passsed to useCallback but to the deps array.

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?

@gaearon
Copy link
Collaborator

gaearon commented May 5, 2020

Hmm. I don’t understand your question.

Maybe we can do this backwards. What do you expect the linter to help you with?

@kilianc
Copy link
Author

kilianc commented May 6, 2020

I don't understand why the linter is complaining about the function selector not being part of the dependencies array. Your first feedback here just mentions the inability to check the dependencies because they are not a literal but a variable / function argument. I get that. But the warning is about selector not being a dep, which to me doesn't make sense, why should it be?

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 useCallback or useEffect, I am most likely going to end up in this situation. Do I just need to disable the line? Seems counter intuitive and feels weird.

@gaearon
Copy link
Collaborator

gaearon commented May 6, 2020

Consider this situation.

function Button({ onPressChange, isPressed }) {
  useEffect(onPressChange, [isPressed]);
}

In your opinion, should this warn or not?

@kilianc
Copy link
Author

kilianc commented May 6, 2020

no, I don't think so

<Button onPressChange={() => console.log('pressed-change')} />

what am I missing?

@kilianc
Copy link
Author

kilianc commented May 6, 2020

To be more precise, I understand how

function Button({ onPressChange, isPressed }) {
  useEffect(onPressChange, [isPressed]);
}

could trigger a warning considering the inability to statically analyze onPressChange and the relation with isPressed.

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
}
  • why am I a getting a warning at all
  • why is the warning telling me to add the function itself to the dependencies?

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])
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Unconfirmed A potential issue that we haven't yet confirmed as a bug
Projects
None yet
Development

No branches or pull requests

2 participants