feat(eslint): add react hooks plugin configuration #363
Conversation
config/js/eslint-react.config.js
Outdated
| 'react-hooks/rules-of-hooks': 'error', | ||
| 'react-hooks/exhaustive-deps': 'warn', |
There was a problem hiding this comment.
Perhaps the opposite approach makes more sense: we start strict and relax rules once people start complaining?
In a way what you've done here is more user-friendly, but in practice I worry we won't ever get requested to enforce stricter rules. On the other hand if we start of with rules that are too strict we'll see failing checks on GitHub, so we are 100% guaranteed to get feedback on this.
There was a problem hiding this comment.
My reasoning was: if the rule is known to have false positives we might break CI for users that aren't doing anything wrong. If we're going to break CI then I feel this update should be a major update.
So I'd go with the minor update first, and then the major version once we're sure it works as expected (basically deprecating first, and then shipping the breaking change).
The rules of hooks error seemed allowed to me in a minor since if that breaks, you're already doing something wrong that has broken your app anyway. Though I guess you could also make the argument that that should be a breaking change.
|
Hi! Due to a lack of activity on this issue over time (180 days) it seems to be stale. If still relevant, please provide information that moves it forward, e.g. additional information, a pull request with suggested changes, or a reason to keep it open. Any activity will keep it open, otherwise it will be closed automatically in 30 days. Thanks! 🤖 |
There was a problem hiding this comment.
Let's get this in :-)
In practice there are legitimate reasons to ignore the "exhaustive deps" check. In app-runtime we use an error as @HendrikThePendric suggested which requires us to manually disable the check where they know they're breaking the rule. I think this is fine for our tightly-controlled libraries, but for apps (and in particular third party apps) I think we can go either way and starting less-strict will cause fewer adoption pains.
ismay
force-pushed
the
add-react-hooks-plugin
branch
from
1837e46
to
be0616e
Compare
|
🎉 This PR is included in version 10.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This adds config for the react-hooks eslint plugin. The plugin already ships with CRA, so all I've done is enable it:
I've set exhaustive-deps to warn. Initially there were some false positives for this rule that meant an error wasn't always correct: facebook/react#14920. Though that issue has since been closed, so maybe it's safe to set to error. There are some related issues open: https://github.com/facebook/react/issues?q=is%3Aissue+is%3Aopen+exhaustive, but maybe setting it to error and just expecting the user to disable when necessary is good enough.