add ignoreThisDependency option to exhaustive-deps

[seiyab]

Summary

I added ignoreThisDependency option to the ESLint rule exhaustive-deps. It's like my suggestion in https://github.com/facebook/react/issues/16265#issuecomment-685723348 .

This helps people who doesn't prefer destructuring. You can find such people at #16265 .

In my case, our project uses React Redux connect like below.

import { doSomething } from 'foobar';

const mapDispatchToProps = { doSomething };

// ...

export default connect(null, mapDispatchToProps)(MyComponent)

Destructuring props conflicts with eslint no-shadow. And I don't want to establish naming convention for auxiliary variables to pass the linter.

Test Plan

• Run tests (I added some new valid cases and a invalid case)
• Installed in my local projects

Screen shots

ignoreThisDependency = 'never'

ignoreThisDependency = 'props'

[codesandbox-ci[bot]]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 17e909e0b96a83e2c85a82ae819261bdeaff4961:

Sandbox	Source
React	Configuration

[sizebot]

Details of bundled changes.

Comparing: 50393dc3a0c59cfefd349d31992256efd6f8c261...17e909e0b96a83e2c85a82ae819261bdeaff4961

eslint-plugin-react-hooks

File	Filesize Diff	Gzip Diff	Prev Size	Current Size	Prev Gzip	Current Gzip	ENV
eslint-plugin-react-hooks.development.js	+1.0%	+0.7%	84.66 KB	85.52 KB	20.17 KB	20.31 KB	NODE_DEV
eslint-plugin-react-hooks.production.min.js	:small_red_triangle:+0.9%	:small_red_triangle:+0.9%	24.75 KB	24.96 KB	8.51 KB	8.59 KB	NODE_PROD

Size changes (experimental)

Generated by :no_entry_sign: dangerJS against 17e909e0b96a83e2c85a82ae819261bdeaff4961

[sizebot]

Comparing: a0d991fe6587ad1cd1a97230f62f82c7cb6b9a40...614657da35694e3c073bb18060f5d133344fa35a

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	=	128.86 kB	128.86 kB	=	41.01 kB	41.01 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js	=	133.83 kB	133.83 kB	=	42.52 kB	42.52 kB
facebook-www/ReactDOM-prod.classic.js	=	419.90 kB	419.90 kB	=	77.34 kB	77.34 kB
facebook-www/ReactDOM-prod.modern.js	=	408.48 kB	408.48 kB	=	75.59 kB	75.59 kB
facebook-www/ReactDOMForked-prod.classic.js	=	419.90 kB	419.90 kB	=	77.34 kB	77.34 kB

Significant size changes

Includes any change greater than 0.2%:

Expand to show

Name	+/-	Base	Current	+/- gzip	Base gzip	Current gzip
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.01%	87.38 kB	88.26 kB	+0.69%	20.74 kB	20.89 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.01%	87.38 kB	88.26 kB	+0.69%	20.74 kB	20.89 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js	+1.01%	87.38 kB	88.26 kB	+0.69%	20.74 kB	20.89 kB
oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.85%	25.54 kB	25.76 kB	+0.33%	8.76 kB	8.79 kB
oss-stable-semver/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.85%	25.54 kB	25.76 kB	+0.33%	8.76 kB	8.79 kB
oss-stable/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.production.min.js	+0.85%	25.54 kB	25.76 kB	+0.33%	8.76 kB	8.79 kB

Generated by :no_entry_sign: dangerJS against 614657da35694e3c073bb18060f5d133344fa35a

[stale[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[seiyab]

I rebased and changed base branch.

[seiyab]

New related Issue: https://github.com/facebook/react/issues/22305

[seiyab]

Need help: Restore node_modules cache fails with following message.

Found a cache from build 374353 at v2-node-arch1-linux-amd64-6_85-pull/20521-WlwS1rWKpwv8jelaStXkEooKEFNEwNHl9rb9vtTRZUQ=-e1MwvNU5aTEEXZDm8y40_6bzz1qRuD6FXv7OHN7oZEU=-node-modules
Size: 212 MiB
Cached paths:
  * /home/circleci/project/node_modules

Downloading cache archive...

file size mismatch in downloaded cache archive, downloaded: 222122050, stored size: 222120848

Running yarn test --prod locally, it passes.

[seiyab]

bump

[pistonsky]

We need this customization option. Because react-navigation's navigate should never be listed in deps, as it causes "maximum depth exceeded" and infinite loops, due to a bad coding of that lib. Same goes to redux's dispatch - why should I list it in deps when it just never changes? Makes no sense. And I don't want to wrap it into useRef. So thumbs up on this 👍

[seiyab]

bump

[laurent22]

Just so you know the doc for your custom fork here is wrong. It should be:

{
  "plugins": [
    // ...
    "@seiyab/eslint-plugin-react-hooks"
  ],
  "rules": {
    // ...
    "@seiyab/react-hooks/rules-of-hooks": "error",
    "@seiyab/react-hooks/exhaustive-deps": "warn"
  }
}

[seiyab]

I updated README. https://www.npmjs.com/package/@seiyab/eslint-plugin-react-hooks Thank you for your feedback. Sorry for late reply.

[laurent22]

Thanks @seiyab, your fork has made a big difference for us, allowing us to keep our own programming patterns while keeping things reliable. We haven't noticed any issue with it so far.

[xeinebiu]

The issue I've identified with this feature is interaction with native JavaScript APIs. When working with a collection of numbers and utilizing any function of the array, this rule would suggest correcting the error as follows:

const [numbers, setNumbers] = useState<number[]>([]);

useEffect(() => console.log(numbers.filter(number => number > 0)), [numbers.filter]); <--- Note that the filter won't change. The useEffect will never run if you update the numbers state.

[laurent22]

Why would you want to do that @xeinebiu? Why not just pass the numbers array as a dependency? Not sure about JS internals but it's very possible that this filter function never changes if it's attached to a prototype

[xeinebiu]

Why would you want to do that @xeinebiu? Why not just pass the numbers array as a dependency? Not sure about JS internals but it's very possible that this filter function never changes if it's attached to a prototype

What I meant is, when the ignoreThisDependency is enabled, ESLint will start suggesting fixes, as I mentioned. Usually, I let ESLint handle dependencies. I just gave it a try and found it to be potentially dangerous.

[seiyab]

You're right. ignoreThisDependency: always is dangerous for common cases. Doesn't ignoreThisDependency: props work for you?

Adding information, I know ignoreThisDependency: props still have false-positive. However, it's too difficult (or impossible) to determine whether this is used or not precisely. Perhaps TSESLint feature can help somewhat for TS projects. I don't intend to add TS feature into it, though. This have been ignored long enough to lose my eagerness.

[xeinebiu]

You're right. ignoreThisDependency: always is dangerous for common cases. Doesn't ignoreThisDependency: props work for you?

Adding information, I know ignoreThisDependency: props still have false-positive. However, it's too difficult (or impossible) to determine whether this is used or not precisely. Perhaps TSESLint feature can help somewhat for TS projects. I don't intend to add TS feature into it, though. This have been ignored long enough to lose my eagerness.

I did not got the difference when using ignoreThisDependency: props and the regular behavior of react-hooks rules. When I used it with always, it started suggesting to pass only the functions ( something.save ) instead of ( something ) passed entirely. So far, it was working fine, but then I realised it is dangerous and it requires a lot of attention when auto fixing ...

[seiyab]

always is dangerous and I don't recommend it but extreme case (e.g. strictly forbids depending on this). The dangerousness is the reason why the original rule warns it. https://github.com/facebook/react/issues/16265#issuecomment-517518539

The difference between original one and ignoreThisDependency: props is whether function call like props.onClick() requires entire props or not (but requires the function props.onClick). This PR suggests not reporting lack of props when the identifier before . is named props. Ignoring component props itself for member function of props rarely be dangerous. This suggestion utilize this heuristics. (Perhaps depending name of identifier might not be good idea.) It looks hard to find better (safe and more convenient) heuristic to me. It's not complete solution yet, tough.

[xeinebiu]

always is dangerous and I don't recommend it but extreme case (e.g. strictly forbids depending on this). The dangerousness is the reason why the original rule warns it. #16265 (comment)

The difference between original one and ignoreThisDependency: props is whether function call like props.onClick() requires entire props or not (but requires the function props.onClick). This PR suggests not reporting lack of props when the identifier before . is named props. Ignoring component props itself for member function of props rarely be dangerous. This suggestion utilize this heuristics. (Perhaps depending name of identifier might not be good idea.) It looks hard to find better (safe and more convenient) heuristic to me. It's not complete solution yet, tough.

So it only works if the object is called props right? I did not knew this one, therefore enabled the always. Thanks for the clearification.

[seiyab]

Right. I'm sorry for the unclear doc.

[laurent22]

I only use ignoreThisDependency: props as well. I'm curious why the always option was added? It seems indeed dangerous to allow this, I certainly wouldn't want it in code I maintain.

[seiyab]

I forgot the reason. Probably I had thought it might make sense for this-less projects and had missed built-in classes like Array. Additionally, I have considered props might not be enough for some projects.

It might be better to remove always.

I noticed following might be better by chance.

// If original one works well:
{ ignoreThisDependency: 'never' }

// If you feel requiring entire "props" is annoying:
{ ignoreThisDependency: ['props'] }

// If you know other "safe" patterns:
{ ignoreThisDependency: ['props', 'revalidator', 'actions'] }

Just a note:

• ravalidator: https://reactrouter.com/en/main/hooks/use-revalidator
• actions: https://react-redux.js.org/api/hooks#recipe-useactions

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

[laurent22]

Bump

[github-actions[bot]]

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.