Allow useEffect(fn, undefined) in react-hooks/exhaustive-deps.

[cpojer]

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.

[react-sizebot]

Comparing: 309c8ad...329f4d2

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js	=	174.68 kB	174.68 kB	=	54.32 kB	54.32 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	176.77 kB	176.77 kB	=	54.99 kB	54.99 kB
facebook-www/ReactDOM-prod.classic.js	=	565.55 kB	565.55 kB	=	99.53 kB	99.53 kB
facebook-www/ReactDOM-prod.modern.js	=	549.40 kB	549.40 kB	=	96.60 kB	96.60 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.24%	26.81 kB	26.87 kB	+0.27%	9.24 kB	9.26 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.24%	26.81 kB	26.87 kB	+0.27%	9.24 kB	9.26 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.23%	27.51 kB	27.57 kB	+0.21%	9.44 kB	9.46 kB

Generated by 🚫 dangerJS against 329f4d2

[cpojer]

@davidaurelio pointed out to me that undefined can still be overwritten in local scope post ES5 strict-mode which I was honestly unaware of. Only globalThis.undefined is not writable.

However, no-shadow-restricted-names rule is a default in ESLint and it restricts shadowing global "variables" like undefined:

(() => {
  // Shadowing of global property 'undefined'. eslint(no-shadow-restricted-names)
  const undefined = 'banana';
})

The majority of people are using the recommended rules so I believe it is reasonable to assume that people who use a linter do not name their local dependency array variables undefined.

[rahulmehta1999]

Sorry, but Undefined as dependency sounds wrong on so many levels.
Why even use an Effect if something needs to execute on every render ? Unless it needs to wait for component mount. Still doesn't make much sense to add undefined as a dependency.

[cpojer]

Whether it sounds wrong to you or not is irrelevant to this discussion.

The facts are that React supports this pattern, there are valid use cases, it is encoded in the TypeScript/Flow types, and this eslint plugin already considers omitting the dependency argument altogether as correct.

[kassens]

This makes sense to me to allow in this linter. You can always have an additional linter that disallows or forces undefined if you have strong feelings about that either way.

[Romick2005]

What is the point of using useEffect with undefined then?

        function MyComponent() {
          useEffect(() => {
            console.log('banana banana banana');
          }, undefined);
        }

It's looks weird. In such cases developers need to have lint warning and think about it. And finally rewrite it as:

        function MyComponent() {
            console.log('banana banana banana'); // It is inner code from previous example
        }

Do you know other samples that you need undefined? Would be great to see it!

[GabenGar]

These run in entirely different contexts during hybrid render, the former runs only in browser while the latter will log both on the server and the browser.

[niksauer]

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.

@cpojer Where can I find your custom rule? 🙂

[cpojer]

Ah, I never shared it. Here you go: https://gist.github.com/cpojer/92a9cb355553011394ff4bc975b93bad