[ESLint] Feedback for 'exhaustive-deps' lint rule

[gaearon]

Common Answers

💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡

We analyzed the comments on this post to provide some guidance: #14920 (comment).

💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡💡

---

What is this

This is a new ESLint rule that verifies the list of dependencies for Hooks like useEffect and similar, protecting against the stale closure pitfalls. For most cases it has an autofix. We'll add more documentation over the next weeks.

Installation

yarn add eslint-plugin-react-hooks@next
# or
npm install eslint-plugin-react-hooks@next

ESLint config:

{
  "plugins": ["react-hooks"],
  // ...
  "rules": {
    "react-hooks/rules-of-hooks": 'error',
    "react-hooks/exhaustive-deps": 'warn' // <--- THIS IS THE NEW RULE
  }
}

Simple test case to verify the rule works:

function Foo(props) {
  useEffect(() => {
    console.log(props.name);
  }, []); // <-- should error and offer autofix to [props.name]
}

The lint rule complains but my code is fine!

If this new react-hooks/exhaustive-deps lint rule fires for you but you think your code is correct, please post in this issue.

---

BEFORE YOU POST A COMMENT

Please include these three things:

1. A CodeSandbox demonstrating a minimal code example that still expresses your intent (not "foo bar" but actual UI pattern you're implementing).
2. An explanation of the steps a user does and what you expect to see on the screen.
3. An explanation of the intended API of your Hook/component.

But my case is simple, I don't want to include those things!

It might be simple to you — but it’s not at all simple to us. If your comment doesn't include either of them (e.g. no CodeSandbox link), we will hide your comment because it’s very hard to track the discussion otherwise. Thank you for respecting everyone’s time by including them.

The end goal of this thread is to find common scenarios and transform them into better docs and warnings. This can only happen when enough details are available. Drive-by comments with incomplete code snippets significantly drive down the quality of the discussion — to the point that it's not worth it.

[billyjanitsch]

This example has a reply: #14920 (comment)

CodeSandbox

This is an uncontrolled Checkbox component which takes a defaultIndeterminate prop to set the indeterminate status on initial render (which can only be done in JS using a ref because there's no indeterminate element attribute). This prop is intended to behave like defaultValue, where its value is used only on initial render.

The rule complains that defaultIndeterminate is missing from the dependency array, but adding it would cause the component to incorrectly overwrite the uncontrolled state if a new value is passed. The dependency array can't be removed entirely because it would cause the indeterminate status to be fully controlled by the prop.

I don't see any way of distinguishing between this and the type of case that the rule is meant to catch, but it would be great if the final rule documentation could include a suggested workaround. 🙂

[gaearon]

Re: #14920 (comment)

@billyjanitsch Would this work instead? https://codesandbox.io/s/jpx1pmy7ry

I added useState for indeterminate which gets initialized to defaultIndeterminate. The effect then accepts [indeterminate] as an argument. You currently don't change it — but if you did it later, I guess that would work too? So the code anticipates future possible use case a bit better.

[dan-lee]

So I got this following (edge?) case where I pass some html and use it with dangerouslySetInnerHtml to update my component (some editorial content).
I am not using the html prop but the ref where I can use ref.current.querySelectorAll to do some magic.
Now I need to add html to my dependencies in useEffect even though I am not explicitly using it. Is this a use case where I actually should disable the rule?

The idea is to intercept all the link clicks from the editorial content and track some analytics or do other relevant stuff.

This is a watered down example from a real component:
https://codesandbox.io/s/8njp0pm8v2

[joelmoss]

I'm using react-redux, so when passing down an action creator in the props from mapDispatchToProps, and using that action creator in a hook, I get a exhaustive-deps warning.

So I can obviously add the redux action to the dependency array, but because the redux action is a function and never changes, this is unneccessary right?

[lukyth]

const onSubmit = React.useCallback(
  () => {
    props.onSubmit(emails);
  },
  [emails, props]
);

I expect the lint to fix the deps into [emails, props.onSubmit], but right now it always fix the deps into [emails, props].

1. A CodeSandbox demonstrating a minimal code example that still expresses your intent (not "foo bar" but actual UI pattern you're implementing).

https://codesandbox.io/s/xpr69pllmz

2. An explanation of the steps a user does and what you expect to see on the screen.

A user will add an email and invite those email to the app. I intentionally remove the rest of the UI to just the button because they're irrelevant to my issue.

3. An explanation of the intended API of your Hook/component.

My component has a single prop, onSubmit: (emails: string[]) => void. It'll be called with the emails state when a user submit the form.

---

EDIT: Answered in #14920 (comment)

This is because technically props.foo() passes props itself as this to foo call. So foo might implicitly depend on props. We'll need a better message for this case though. The best practice is always destructuring.

[atomiks]

CodeSandbox

It doesn't consider that mount and update can be distinctly different when integrating when 3rd party libs. The update effect can't be included in the mount one (and removing the array altogether), because the instance shouldn't be destroyed on every render

[sylvainbaronnet]

Hi,

Not sure what's wrong with my code here :

const [client, setClient] = useState(0);

useEffect(() => {
    getClient().then(client => setClient(client));
  }, ['client']);

I got React Hook useEffect has a complex expression in the dependency array. Extract it to a separate variable so it can be statically checked

[gaearon]

@joelmoss @sylvainbaronnet I appreciate your feedback but I hid your comments because they didn't include the information we asked for at the top of the issue. That makes this discussion unnecessarily difficult for everyone because there's missing context. I'd be happy to continue the conversation if you post again and include all the relevant information (see the top post). Thank you for understanding.

[btholt]

1. CodeSandbox
2. User can select a first name and it forces the last name to change. User can select a last name and it does not force the first name to change.
3. There's a custom hook which returns a piece of state, a select component that updates that state, and the state hook's update method that updates the state programmatically. As demonstrated, I don't always want to use the updater function so I left it to the be last item in returned array.

I believe the code as-is shouldn't error. It does right now on line 35, saying that setLastName should be included in the array. Any thoughts on what to do about that? Am I doing something unexpected?