-
-
Notifications
You must be signed in to change notification settings - Fork 94
Add react exhaustive dependencies linter rule #657
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
base: main
Are you sure you want to change the base?
Add react exhaustive dependencies linter rule #657
Conversation
chriscerie
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Need changelog. It would be great to run this against the tests from #548 to gain visibility into the drift between these impls
| @@ -0,0 +1,32 @@ | |||
| error[roblox_react_exhaustive_deps]: React Hook useEffect has unnecessary dependency: props.value | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super nit: we use lowercase in these messages
| 11 │ end, { props.value }) | ||
| │ ^^^^^^^^^^^^^^^ | ||
| │ | ||
| = Remove 'props.value' from the dependency array |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same thing and we prepend help: here
Co-authored-by: Chris Chang <[email protected]>
…om/BitwiseAndrea/selene into feature/react-missing-dependencies
https://react.dev/reference/eslint-plugin-react-hooks/lints/exhaustive-deps
Upstream
https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/src/rules/ExhaustiveDeps.ts
https://github.com/facebook/react/blob/main/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js