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

[react-hooks/exhaustive-deps] missed warning when passing a function #16573

Closed
whatisaphone opened this issue Aug 26, 2019 · 4 comments · Fixed by #18435
Closed

[react-hooks/exhaustive-deps] missed warning when passing a function #16573

whatisaphone opened this issue Aug 26, 2019 · 4 comments · Fixed by #18435

Comments

@whatisaphone
Copy link

Do you want to request a feature or report a bug?

Arguably a bug in eslint-plugin-react-hooks

What is the current behavior?

The exhaustive-deps rule does not catch the following case:

const FooContext = React.createContext(() => {});

// Meanwhile, somewhere deep in the component tree
function Bar() {
  const foo = useContext(FooContext);
  useEffect(foo, []);
  return <div>A div walks into a Bar…</div>;
}

What is the expected behavior?

I think the plugin should suggest adding the dependency on foo:

useEffect(foo, [foo]);

It already makes this suggestion for the verbose form of the code:

// This code:
useEffect(() => foo(), []);
// is fixed to:
useEffect(() => foo(), [foo]);

Which versions of React, and which browser / OS are affected by this issue? Did this work in previous versions of React?

  • eslint@6.2.2
  • eslint-plugin-react-hooks@2.0.1
  • react@16.9.0
@threepointone
Copy link
Contributor

threepointone commented Sep 3, 2019

It's a bit of a niche usecase where a function is used directly as useEffect's callback (consider in your example, that the callback would have no access to state or any local variables.) That said, I can see how it would be useful when the need does arise. I think we'd accept a PR for this.

@simbathesailor
Copy link

@threepointone : Hey I have made good progress in this. Can I take up this issue?.. I have done the fix , but now I have to give it polishes with test cases. I will keep it posted here !!. I will try to make it according to existing code standard by this weekend. And we will take it from there !!.

@stale
Copy link

stale bot commented Feb 17, 2020

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contribution.

@stale stale bot added the Resolution: Stale Automatically closed due to inactivity label Feb 17, 2020
@gaearon gaearon added Type: Bug and removed Resolution: Stale Automatically closed due to inactivity Type: Enhancement labels Feb 17, 2020
@gaearon
Copy link
Collaborator

gaearon commented Feb 17, 2020

The fact that it doesn't suggest it is a bug.

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