Include React Hooks rules (eslint-plugin-react-hooks)

[jahed]

The React team published a plugin to enforce the Rules of Hooks. I think it's worth including as part of standard-react. Seems pretty standard.

https://www.npmjs.com/package/eslint-plugin-react-hooks

Thoughts?

[feross]

This is a great suggestion. Can you send a PR with the rules that you suggest we add?

[jahed]

Looked into sending a PR but came across an issue.

I was considering the following:

"react-hooks/rules-of-hooks": "error", // Checks rules of Hooks
"react-hooks/exhaustive-deps": "error" // Checks effect dependencies

(Note: the default is to set exhaustive-deps to warn but I realise Standard JS avoids warn in favour of error. Which isn't a big deal.)

The exhaustive-deps rule is extremely useful for auto-populating the deps array (using eslint's autofix).

However, the recommended usage of exhaustive-deps is to disable it on a case-by-case basis, which doesn't really go in line with how StandardJS is used. That is, strict rules to enforce a standard code style and avoid divergences. exhaustive-deps is more of helper than a strict rule.

If I omit exhaustive-deps from the PR, I don't think the change will be very useful since users (like myself) will probably want to enable it on their end -- after they've read up on how to use it.

At this point, I don't think there's much benefit to adding this plugin as it may cause confusion. To me at least, it goes against the simple, success-or-failure result of Standard's existing rules and introduces a potentially problematic "disable" step to the flow.

At the least, I'd wait until React adds the rules to the create-react-app where it's likely to become more familiar among the community.

https://reactjs.org/docs/hooks-rules.html

In the future, we intend to include this plugin by default into Create React App and similar toolkits.

Thoughts?

[feross]

I think waiting until the kinks in this get worked out is a good idea. (either through improvements to the rule, or through community familiarity)

[MatanBobi]

@jahed I believe that even though exhaustive-deps is more of a helper than a rule, the benefit of it is worth the work, as this will affect react devs only.

@feross I don't mind creating a PR for this one.

[feross]

@MatanBobi Feel free to send a PR, but I'd like a few other users like @jahed and others who have contributed to this repo in the past to confirm it doesn't cause too many false positives for them before we merge it.

[orpheus]

@MatanBobi I'm not to sure exhaustive-deps is just a helper. Dan Abramov clearly states in his article about useEffect that "all values from inside your component that are used by the effect must be there"

But either way, I agree that these would be really beneficial to have as part of standard (or at least to be easily be able to add them)

[MatanBobi]

@orpheus definitely right, my bad.
Actually made a PR just have to fix something there.

[jahed]

Following on from my previous comment, Create React App has added react-hooks/rules-of-hooks as error and react-hooks/exhaustive-deps as warn.

https://github.com/facebook/create-react-app/blob/master/packages/eslint-config-react-app/index.js

Considering this seems to be the "standard", maybe adding it as warn is fine?

I looked through existing PRs.

#62 committed node_modules 😨
#63 does a version bump which is part of the release process. 🤔

I can make a new PR if we're in agreement and neither of those are resolved. This isn't urgent or anything since it's just a few lines of config.

[MatanBobi]

@jahed I saw that you opened a new PR so I closed mine.. Sorry about the node_modules 🤦🏼‍♂️ My bad.

[jahed]

@MatanBobi No worries. I didn't make a PR yet, cwonrails did (#63) though it's been waiting for over a year. I'm assuming feross has a lot on his hands with all the other projects so he hasn't had the time to review it.

[feross]

Just reviewed PR #63

[LinusU]

Merged in #75

[LinusU]

Released in v13.0.0