Skip to content

Commit 253126e

Browse files
committed
Allow useEffect(fn, undefined) in react-hooks/exhaustive-deps. (#27525)
## Summary There is a bug in the `react-hooks/exhaustive-deps` rule that forbids the dependencies argument from being `undefined`. It triggers the error that the dependency list is not an array literal. This makes sense in pre ES5 strict-mode environments as undefined could be redefined, but should not be a concern in today's JS environments. **Justification:** * The deps argument being undefined (for `useEffect` calls etc.) is a valid use case for hooks that should re-run on every render. * The deps argument being omitted is considered a valid use case by the `exhaustive-deps` rule already. * The TypeScript type definitions support passing `undefined` because hooks are typed as `useEffect(effect: EffectCallback, deps?: DependencyList): void;`. * Since omitting an argument and passing `undefined` are considered equivalent, this eslint rule should consider them as equivalent too. Further, I accidentally forgot passing a dependency array to `useEffect` in code that I shared on Twitter, and people started abusing me about it. I'd like to create an eslint rule for my projects that requires me to provide a dep argument in all cases (`undefined`, `[]` or the list of dependencies) so that I can avoid such problems in the future. This would also force me to always think about the dependencies instead of accidentally forgetting them and my hook running on each render. In an audit of my own codebase I had about 3% of hooks that I want to run on each render, and adding an explicit `undefined` seems reasonable in those situations. It could be argued this could be an option or part of the `exhaustive-deps` rule, but it's probably better to merge this PR, make a release and see if my custom eslint rule gains traction in the future. ## How did you test this change? * Added a test. * `yarn test ESLintRuleExhaustiveDeps-test` * Careful code inspection. DiffTrain build for [d947c2f](d947c2f)
1 parent 0d03ff9 commit 253126e

File tree

2 files changed

+3
-2
lines changed

2 files changed

+3
-2
lines changed

compiled/facebook-www/REVISION

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1 +1 @@
1-
77c4ac2ce88736bbdfe0b29008b5df931c2beb1e
1+
d947c2f1100dfd006fd91c6e9ed84d42ca7ab088

compiled/facebook-www/eslint-plugin-react-hooks.js

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1856,7 +1856,8 @@ var ExhaustiveDeps = {
18561856
var callback = node.arguments[callbackIndex];
18571857
var reactiveHook = node.callee;
18581858
var reactiveHookName = getNodeWithoutReactNamespace(reactiveHook).name;
1859-
var declaredDependenciesNode = node.arguments[callbackIndex + 1];
1859+
var maybeNode = node.arguments[callbackIndex + 1];
1860+
var declaredDependenciesNode = maybeNode && !(maybeNode.type === 'Identifier' && maybeNode.name === 'undefined') ? maybeNode : undefined;
18601861
var isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); // Check whether a callback is supplied. If there is no callback supplied
18611862
// then the hook will not work and React will throw a TypeError.
18621863
// So no need to check for dependency inclusion.

0 commit comments

Comments
 (0)