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

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

Merged
merged 33 commits into from
Feb 20, 2019

Conversation

gaearon
Copy link
Collaborator

@gaearon gaearon commented Jan 19, 2019

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:

screen shot 2019-01-19 at 12 06 48 am

(^^ 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:

adding the array

Removing a dependency:

removing a dep

Introducing a dependency:

introducing a dep

Editing dependencies:

editing deps

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
Copy link

sizebot commented Jan 19, 2019

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
Copy link
Contributor

Jessidhia commented Jan 23, 2019

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
Copy link

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
Copy link
Collaborator

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.)

@gaearon gaearon force-pushed the eslint-2.1 branch 2 times, most recently from 4d45e7e to 7d4e944 Compare February 14, 2019 21:55
@aickin
Copy link
Contributor

aickin commented Feb 16, 2019

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
Copy link

dan-lee commented Feb 18, 2019

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
Copy link
Collaborator Author

gaearon commented Feb 18, 2019

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
Copy link

dan-lee commented Feb 18, 2019

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
Copy link
Collaborator Author

gaearon commented Feb 18, 2019

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
Copy link
Collaborator Author

gaearon commented Feb 18, 2019

Published eslint-plugin-react-hooks@1.1.0-alpha.1 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
Copy link

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
Copy link

dan-lee commented Feb 19, 2019

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
Copy link
Collaborator Author

gaearon commented Feb 19, 2019

@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
Copy link
Collaborator Author

gaearon commented Feb 19, 2019

@dan-lee

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

@davegomez
Copy link

davegomez commented Feb 19, 2019

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
Copy link
Collaborator Author

gaearon commented Feb 19, 2019

@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.

if (
reference.resolved != null &&
Array.isArray(reference.resolved.defs)
) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bvaughn bvaughn self-assigned this Feb 19, 2019
@bvaughn
Copy link
Contributor

bvaughn commented Feb 19, 2019

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.

Copy link
Contributor

@bvaughn bvaughn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

node.name === 'useImperativeHandle'
) {
return 1;
} else if (options && options.additionalHooks) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

});
} else {
declaredDependenciesNode.elements.forEach(declaredDependencyNode => {
// Skip elided elements.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL "elided"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lol I don't remember writing this

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

} else if (i < arr.length - 1) {
s += ', ';
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😮 lol

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't resist

if (badRef !== null) {
extraWarning =
` Mutable values like '${badRef}' aren't valid dependencies ` +
"because their mutation doesn't re-render the component.";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

@gaearon gaearon Feb 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I understand what you're saying.

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

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.

Array.from(set)
.sort()
.map(quote),
) +
Copy link
Contributor

@Jessidhia Jessidhia Feb 20, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually we could alphabetize if existing list is alphabetized. 😏

errors: [
"React Hook useEffect has an unnecessary dependency: 'local2'. " +
'Either exclude it or remove the dependency array.',
],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you give me a specific example?

Copy link
Contributor

@Jessidhia Jessidhia Feb 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 }} />
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@dan-lee
Copy link

dan-lee commented Feb 20, 2019

  • 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
Copy link
Collaborator Author

gaearon commented Feb 20, 2019

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 gaearon changed the title [WIP] useEffect/useCallback/useMemo dependency rule Add eslint-plugin-react-hooks/exhaustive-deps rule to check stale closure dependencies Feb 20, 2019
@gaearon
Copy link
Collaborator Author

gaearon commented Feb 20, 2019

Published eslint-plugin-react-hooks@1.1.0-rc.0 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 gaearon merged commit dab2fdb into facebook:master Feb 20, 2019
@gaearon gaearon deleted the eslint-2.1 branch February 20, 2019 18:19
@gaearon
Copy link
Collaborator Author

gaearon commented Feb 21, 2019

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.

@facebook facebook locked as resolved and limited conversation to collaborators Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.