Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies

[gaearon]

For instructions and feedback, see the issue instead:

>>> #14920 <<<

---

There's some polish missing (see checklist below) but a basic version should be working.

---

Supersedes #14048 and later #14052. (Thanks @calebmer for starting this and @jamiebuilds for fixing the issues in previous PR.)

I've decided to largely start the reporting part from scratch. In particular, I feel like the previous reporting that was centered around usage sites was too noisy:

(^^ I don't this is the right approach)

It made it seem like there's something wrong with your function body, but the wrong part is in the dependency list. It was also unclear how autofix would work (would there be one autofix per missing dep?) Instead the ideal experience (IMO) is that you just make edits, and when the deps are wrong, only deps are highlighted. If you really know what you're trying, you just need to eslint-disable that line alone rather than many.

With that insight, the approach I'm taking is to:

• Always report the dependency array itself. This ensures the user blames the array rather than the actual code. It's where you want to go to fix it, and where you can disable the line if you need to.
• Always allow auto-fix. By default, auto-fix should make a reasonable attempt at guessing the deps. We might get it a bit wrong if the inputs are mutable but with special handling for refs (.current) we should be right most cases.
• Make minimal changes. As much as possible, we should preserve the original code and intent. If the user wants to order them as alphabet, cool. If they want to add the new one at the end, fine. Auto-fix should suggest the closest "fix" to the list we already have, and shouldn't flag any other valid lists (e.g. [props] over [props.x], or [x, y] over [y, x]) as needing fixes. User should not need to "fight" with auto-fix except rare use cases with mutability.

With those principles in mind, the algorithm is I'm gathering a list of ideal dependencies first. I collect violations into three buckets (missing, duplicate, unnecessary). Then I report them together. The autofix tries to do minimal possible changes — e.g. it leaves the current order intact for deps that are necessary, and only adds new ones at the end. This algorithm will need some changes as I deal with objects and properties. More on that in TODO below.

Adding the array:

Removing a dependency:

Introducing a dependency:

Editing dependencies:

There's a lot of missing stuff here but I feel like this is the right direction overall.

TODO

• Re-add support for obj.prop / obj.method()
• Make sure obj “satisfies” obj.prop
• Be smart about props.items.slice() (don't suggest [props.items.slice])
• Rewrite and fix tests, including autofix
• Be smart about [ref.current] (suggest [ref])
• Consider not caring about dispatch and state setters if we can prove their origin
  • Possible middle ground: generate them on autofix but don't complain if missing
  • Possible middle ground: don't generate them on autofix but also don't complain if present
• Don't forget to add useImperativeHandle
• Polish: "dependency" when there's just one
• Add a special message for ref.current
• Handle undefined used variables gracefully
• Make sure we bail out unless all dep nodes are foo or foo.bar.baz
• Pick the name

Potential follow-up:

• Consider a heuristic where many deep accesses gives shared ancestor
  • Maybe use <10 deps in array if possible as a guiding principle
• Optimize it
• Go through TODOs in tests and make more decisions
• Autofix shouldn't delete comments other tokens

[sizebot]

Details of bundled changes.

Comparing: 0e67969...8b554ca

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+72.2%	+76.7%	25.51 KB	43.93 KB	5.87 KB	10.37 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	🔺+107.5%	🔺+105.1%	4.97 KB	10.31 KB	1.84 KB	3.77 KB	NODE_PROD
ESLintPluginReactHooks-dev.js	+72.5%	+78.3%	27.14 KB	46.8 KB	6 KB	10.7 KB	FB_WWW_DEV

Generated by 🚫 dangerJS

[Jessidhia]

An advantage of the previous approach is that it gave you a way of marking a certain omission from the inputs array as false positives without disabling the lint rule for the entire array.

...however, the code sample I was about to paste as an example of that exception was actually not an exception. Here's a drastically simplified version of it:

const derivedValue = useMemo(() => derive(propA), [propA])
useEffect(() => { doThing(derivedValue, propB) }, [propA, propB])

The second one would cause a lint warning, which is arguably a false positive. But using derivedValue instead of propA in the second list would still work exactly the same.

---

I do wonder though what would be the right thing to do if propA (and by extension derivedValue) should not be in the useEffect deps list. Maybe you want to use the current value of derivedValue when the effect triggers, but you only want to trigger it when propB changes.

[jamiewinder]

I'm curious about this item on the TO-DOs:

Be smart about [ref.current] (suggest [ref])

I didn't think React treated RefObject-s any differently as dependency / inputs for these hooks. This suggests that specifying a RefObject as a dependency will have it track the current property implicitly. Am I reading this right?

[sophiebits]

I didn't think React treated RefObject-s any differently as dependency / inputs for these hooks. This suggests that specifying a RefObject as a dependency will have it track the current property implicitly. Am I reading this right?

No; you're right that React treats a ref the same as any other in a dependency list. However, passing .current in deps is usually not you want since it gets mutated outside the render phase. (Particularly in the case you use ref={r} with [r.current], your deps array will "notice" that r.current changed on the render after you actually change it – almost never what you want.)

[aickin]

One (maybe bad) idea: perhaps the lint rule could ignore manually added deps that are properties of other deps. This might let programmers who use mutable objects use the rule.

For example, imagine this code:

function Foo(props) {
  const environment = getEnvironment();
  useEffect(() => {
    doSomething(environment):
  }, [environment]); 
}

And further imagine that environment is mutable and that you as the programmer know that doSomething() uses environment.userId. It’d sure be nice if the rule didn’t complain about:

function Foo(props) {
  const environment = getEnvironment();
  useEffect(() => {
    doSomething(environment):
  }, [environment, environment.userId]); 
}

But if you delete environment from the closure, I think it’s right for the rule to complain:

function Foo(props) {
  const environment = getEnvironment();
  useEffect(() => {
    doSomething():
  }, [environment.userId]);  // this should still error out IMO.
}

I may not have thought through all the cases, so feel free to tell me this is unworkable. And thanks so much for your work on this rule; I think it’ll help so many people!

[dan-lee]

With this code:

const Comp = ({ item, onClick }) => {
  const handleClick = useCallback(
    e => {
      save(item)
      onClick(item)
    },
    [item.id, onClick]
  )
}

the linter gives me the following:

React Hook useCallback has missing [item], unnecessary [item.id] dependencies. Either fix or remove the dependency array react-hooks/reactive-deps

My guess this is intentionally, because of:

The array of inputs is not passed as arguments to the effect function. Conceptually, though, that’s what they represent: every value referenced inside the effect function should also appear in the inputs array. In the future, a sufficiently advanced compiler could create this array automatically.

But do I really need to introduce a deep compare effect for this, even if the item.id check would suffice? Couldn't really find much information about this anywhere.

(I need item to be an object, because it's also passed down to the children.)

[gaearon]

But do I really need to introduce a deep compare effect for this, even if the item.id check would suffice?

Note there's technically no deep comparison. It would still be referential: item !== prevItem.

Yes, it seems like you’d need to have item in your deps. Otherwise you risk closing over a stale item. For example, if it gets edited in a component above, but its ID doesn’t change.

[dan-lee]

But I can guarantee the 'stability' of my item object, because it's coming from an API and is not changed in the runtime of my app whatsoever. Only if the ID is changed other contents also did.
While writing this, I realized that I might just pass in id as a prop and skip the effect for that.

[gaearon]

But I can guarantee the 'stability' of my item object, because it's coming from an API and is not changed in the runtime of my app whatsoever.

Sure you can now — but the person using your component in a year might not know that the component makes this assumption. Generally components should be resistant to their props changing.

[gaearon]

Published [email protected] with latest fixes:

• Ref containers, dispatch, and setState won't cause warnings when it's statically visible they are from useRef / useState / useReducer. They'll still be fixed by autofix if you have other violations though.
• Better punctuation and grammar, the message look less overwhelming.
• It warns for useImperativeHandle too now.

Please give it a try! Testing instructions are at the top: #14636 (comment).

[davegomez]

The useEffect warning and fixes is causing infinite loops in my application, is this intentional or a good thing?

One thing that I have notice is that the rule ask you to include all the possible dependencies in the Hook, no matter if is susceptible to change or not, but so far I have found that no matter the case always creates an infinite loop.

[dan-lee]

Thanks for your explanation @gaearon, I see the point.

Moving forward… following gives me a warning:

const item = React.useRef()
useEffect(() => { console.log(item) }, [])

React Hook useEffect has a missing 'item' dependency. Either include it or remove the dependency array

This one doesn't:

import { useRef } from 'react'
const item = useRef()
useEffect(() => { console.log(item) }, [])

Other hooks seem to work just fine and correctly identify the missing the dependency with either useX or React.useX.

[gaearon]

@davegomez

Can you please provide a snippet of code? Unfortunately I can’t guess why it’s causing an infinite loop without seeing the code, and so I can’t judge whether the rule is faulty or something else.

Ideally a CodeSandbox demo would be nice but even just a code snippet would be more helpful than nothing.

[gaearon]

@dan-lee

Thanks. We’ll need to make the detection stronger. Currently it doesn’t check for React.useRef.

[davegomez]

Sure @gaearon, in fact you can have the whole code. https://github.com/davegomez/silky-charts/blob/master/src/hooks/useResize.js

Those are the dependencies suggested by the rule.

[gaearon]

@davegomez I think you bumped into a case where you need a ref to keep the "current" callback. If you want to avoid resubscribing. Same for debouncing function — your debouncing doesn't work because it re-setups the debouncing function on every render.

[gaearon]

This section is messy. I'll extract a function in a refactor before merging.

[bvaughn]

I ran this rule against the work-in-progress DevTools rewrite. I didn't see any invalid warnings! (see bvaughn/react-devtools-experimental@bec0733)

Very minor nits 😄

• I don't like inputs arrays that are longer than necessary. If you forget to add the state, the rule will also auto-add the state updater function (on --fix) which makes the inputs array longer than is strictly necessary. You can manually remove the setter function in this case though and it won't warn.
• The inputs are added in the order they're referenced rather than alpha-sorted.

[bvaughn]

I didn't read the AST stuff super carefully, because it would take me a long time (since I don't read this stuff often). Between running it on the DevTools repo, and your unit tests, and your running it internally at FB– I feel pretty good about the rule. I also tested it a little in AST explorer for some cases I was curious about while reading.

[bvaughn]

How is this intended to be used? (When would you need it?)

[gaearon]

When you have your own Hooks with deps like useMyCustomEffect that you want to check. I'm open to removing this option (it was in first @calebmer's draft) but also seems like it isn't hard to support.

[Jessidhia]

A possible heuristic is to guess that hooks which receive a callback followed by an array as their last argument are following the useMemo pattern

[bvaughn]

TIL "elided"

[gaearon]

Lol I don't remember writing this

[gaearon]

Oh I guess it's from Caleb's old code

[bvaughn]

😮 lol

[gaearon]

Couldn't resist

[bvaughn]

What's an example of code that would fire this warning? The rule supports refs, and e.g. props objects. It seems to be okay with other mutable values you pass it.

[gaearon]

Not sure I understand what you're saying.

You can see the code causing this in tests, e.g.:

react/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleReactiveDeps-test.js

Lines 1306 to 1315 in 0c722bb

	code: `
	function MyComponent(props) {
	const ref1 = useRef();
	const ref2 = useRef();
	useEffect(() => {
	ref1.current.focus();
	console.log(ref2.current.textContent);
	alert(props.someOtherRefs.current.innerHTML);
	fetch(props.color);
	}, [ref1.current, ref2.current, props.someOtherRefs, props.color]);

Passing [ref.current] in deps is almost always a mistake (which leads to rather gnarly bugs) because it's mutated outside the render phase. So during render it will always be "one step behind".

Just like render result, deps should only depend on props/state/globals — not mutable values.

[Jessidhia]

Maybe this could also be space for a future "sorted-deps" rule? 🤔

[gaearon]

One simple compromise would be to only add at the end by default, but alphabetize if the current array is empty.

[gaearon]

Actually we could alphabetize if existing list is alphabetized. 😏

[Jessidhia]

This kinda runs into that issue I mentioned in an earlier comment. It may be valid to use the latest value of a local but only reacting to a specific value changing. I know I wrote something that used this property recently but I can't remember what...

At any rate, it would be good to opt out of specifying a specific value without disabling the rule for the entire array. Perhaps it could be possible to write something to let the rule know you know about it? My first idea would be just having the identifier present inside the array but commented out, but I'm not sure eslint lets you consume comments that easily

[gaearon]

Can you give me a specific example?

[Jessidhia]

Here's some partial source code where I ended up having to break this rule. You can see it in the last useEffect's effect list -- basically, I want this effect to run after the update is called by a dispatch to setCurrentState. However, it should not run on initial mount, or when setCurrentState was invoked by the previous effect which synchronizes the internal-only page with the memoPage.

function useCurrentPage({
  location,
  marker,
  replace
}: Pick<Props, 'location' | 'marker' | 'replace'>) {
  const { hash } = location
  const memoPage = useMemo(() => {
    if (!hash || !hash.startsWith('#')) {
      return 1
    }
    return Math.max(
      1,
      parseInt(hash.slice(1), 10) || getPageFromChapterId(hash) || 1
    )
  }, [hash])
  const [page, setCurrentPage] = useState(memoPage)

  useEffect(() => {
    setCurrentPage(memoPage)
  }, [memoPage])

  useEffect(() => {
    // ignore if it was an update from the synchronization above
    if (page === memoPage) {
      return
    }

    if ((!page || page <= 1) && marker === null) {
      replace({
        ...location,
        hash: ''
      })
    } else {
      replace({
        ...location,
        hash: `#${page}`
      })
    }
    // uses latest data but only triggered by setCurrentPage changes
  }, [page])

  return [memoPage, setCurrentPage] as [typeof memoPage, typeof setCurrentPage]
}

It's certainly possible I just didn't think this through well enough, but it looks unavoidable. Adding marker to the dependencies list could cause unwanted updates because, on the initial pass, page won't have been synchronized yet.

[Jessidhia]

The rubber ducky debugging might have struck again, because it looks like the page === memoPage check in the effect is also sufficient to guard against unwanted updates to marker or location. However, it'd still be a problem to have memoPage in the dependencies list -- the effect would run before the synchronization, used to detect whether setCurrentPage was dispatched from inside or from outside, happens.

[dan-lee]

I think this use case I've got also relates to this:

function LinkInterceptor({ html }) {
  const handleClick = e => {
    e.preventDefault()
    console.log(e.target.href)
  }
  const ref = useRef()

  useEffect(
    () => {
      Array.from(ref.current.querySelectorAll('a')).forEach(node =>
        node.addEventListener('click', handleClick)
      )

      return () => {
        Array.from(ref.current.querySelectorAll('a')).forEach(node =>
          node.removeEventListener('click', handleClick)
        )
      }
    },
    // eslint-disable-next-line react-hooks/exhaustive-deps
    [html]
  )

  return <div ref={ref} dangerouslySetInnerHTML={{ __html: html }} />
}

[gaearon]

Please post both of these in #14920, with the guidelines I mentioned. Thanks.

[dan-lee]

• Consider not caring about dispatch and state setters if we can prove their origin
  • Possible middle ground: generate them on autofix but don't complain if missing

May I ask why is that so? I got confused a few times now and I keep removing them anyway. Is there a reason to keep them?

[gaearon]

May I ask why is that so? I got confused a few times now and I keep removing them anyway. Is there a reason to keep them?

I just pushed a commit that doesn't add them by default. :-)

[gaearon]

Published [email protected] with a few changes. The rule was renamed to exhaustive-deps.

I'll merge this in now but please feel free to add more comments to this PR as you test it.

[gaearon]

If this rule is unexpectedly firing for you, please post to #14920. I'll lock this PR to encourage sharing examples there from now on.