[ESLint] Add more cases to exhaustive-deps rule #14930
Conversation
Details of bundled changes.Comparing: f99fca3...e9ea642 eslint-plugin-react-hooks
Generated by 🚫 dangerJS |
| { | ||
| // Valid because we assign ref.current | ||
| // ourselves. Therefore it's likely not | ||
| // a ref managed by React. |
There was a problem hiding this comment.
This comment and the code below seem out of sync. Maybe bad copy-pasta? Maybe should mirror the comment above?
Valid because the ref is captured.
| node: dependencyNode.parent.property, | ||
| message: | ||
| `Accessing '${dependency}.current' during ` + | ||
| `the effect cleanup is likely a mistake because by this time React ` + |
There was a problem hiding this comment.
A cultural suggestion: Explain what is happening and why rather than telling someone directly they made a mistake. I suspect people might get overly defensive if the computer tells them they made a mistake.
| `the effect cleanup is likely a mistake because by this time React ` + | |
| `the effect cleanup will likely cause a TypeError because by this time React ` + |
No opinion what should we call it e.g. "null pointer exception", "reading a property from null" etc.
There was a problem hiding this comment.
TypeError isn't the main problem (easy to work around). A bigger problem is that you'll always keep getting the previous ref value since it's being read too early. The challenge is to explain this without freaking people out about how "complex" it is.
There was a problem hiding this comment.
Oops, I misread. This is a different warning from the one I was thinking about. In this case the problem is that you're reading the next ref value when you meant to access the previous one.
There was a problem hiding this comment.
Right, maybe
| `the effect cleanup is likely a mistake because by this time React ` + | |
| `the effect cleanup will likely use a different ref value because by this time React ` + |
My point was more about not actually saying that this "is a mistake". That's already implied by reporting that a lint rule has been violated. As far as I know the other messages don't add this is likely a mistake either.
There was a problem hiding this comment.
Sounds reasonable.
ref.currentin effect cleanup (for example, [ESLint] Feedback for 'exhaustive-deps' lint rule #14920 suffers from this bug). The heuristic is that we only warn when you see you don't assignref.current =yourself. Then we assume it’s managed by React.