Enable react-hooks/exhaustive-deps eslint rules #24914
Conversation
kevin940726
added
the
[Type] Code Quality
kevin940726
requested review from
ajitbohra,
gziolo,
nerrad and
ntwb
as code owners
|
Size Change: +7.61 kB (+1%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
|
It looks like Personally I'm a 👍 as I've found myself wanting this rule on several occasions. When I glance at the list generated by running @youknowriad: Do you remember what exceptions to this rule we were running into? |
|
@kevin940726: Could you please add a CHANGELOG entry to |
|
@noisysocks Ahh didn't know that! Good to know ❤️ It seems like when we're removing it we didn't really understand the usage of it since there isn't much documentation about it either. I think that's not applicable now, it does more good than harm, and enabling them only gives us warnings not errors. Sure, I'll add an entry! |
|
I think there are a lot of places where we explicitly rely on the fact that we don't want to run the "effect" when some variable changes. I don't know which ones though. So I'm not certain it's a good approach to just go ahead and add a warning for this. I'd rather we audit where the rule fails and see what impact applying the rule has. I know this just adds "warnings", but a warning on the output of eslint doesn't tell the whole story that potentially, this is not an issue. |
|
Agreed with the "just add warnings" part, it definitely adds some noises, so I'd expect us to fix them all in the future.
Have to disagree with this though. As pointed by Dan in facebook/create-react-app#6880 (comment), disabling it often leads to bugs and is usually a mistake. Furthermore, we have a big open source community with many contributors with different levels of skill, enabling the "official recommended" eslint rules not only helps us have consensus on coding style, but also helps contributors write resilient code. It also helps us reviewing the code too (#23952 (comment))! I'd suggest to enable the rules, and if we did find a proper use case where eslint rules shouldn't be enabled, we can always disable it inline and add comments above it. If we believe it's a bug, we can even submit it to facebook/react#14920 :) As per audit, I didn't do a full project-wide audit, but I have looked into some of the cases where the rule fails. IMHO, most of them are just oversights or bugs. For some of them that are technically "correct", they often have better ways to do it too. That's just my experience and preference though, I'd appreciate other's feedbacks ❤️ . |
|
Happy to be proved wrong here and I know Dan Abramov's opinion on this but IMO it's not black and white here. I'm certain that this rule is helpful in 80% of our current warnings but the audit will help make sure whether we can trust it 100% |
|
I think if we want to explicitly leave a variable out of a dependency array, we need to make it clear that we're doing so intentionally via a lint-rule disable for that line and an associated comment explaining why it's okay to not include every dependency in that particular use-case. It's also worth noting that for cases where we want to run something only once, we could simply introduce an explicitly titled For those reasons, I am completely in favor of introducing this ESLint rule. |
kevin940726
requested review from
cameronvoell,
ellatrix,
etoledom,
guarani and
youknowriad
as code owners
kevin940726
force-pushed
the
update/eslint-react-hooks-exhaustive-deps
branch
2 times, most recently
from
473e713
to
2e25f1c
Compare
|
Not sure if I agree with adding refs to the dependencies. #27080 (comment)
Also adding I don't have a strong opinion on this though. |
kevin940726
force-pushed
the
update/eslint-react-hooks-exhaustive-deps
branch
3 times, most recently
from
1877a12
to
0b3ec5d
Compare
|
Noting that it is currently enabled as Line 355 in 26d2f37 I'm happy to introduce this as a warning in the eslint plugin since we have great proof that it helps 👍 |
|
Shall we just merge this now then? Once done we can drop a note in Core Slack to advise everyone not to panic and how they can go about resolving errors. |
I don't see why not. This will only land in the next release of the eslint-plugin package and it does have a CHANGELOG entry, plus it's only a warning, so it will not be blocking any work. Of course, a note in Slack or a dev note would be helpful to inform everyone interested. |
|
Just another note - we should aim to fix any new warnings of that type in the Gutenberg codebase. The idea is not to clutter it with warnings 😉 |
|
With the changes from this PR, would the rule still error in the For the strategy towards "zero warnings", I suggest the approach that we took for the components package:
|
Yes. This is changing the default rules, but those are overridden by the project ESLint settings, where the rule is set to error for the components package. |
|
What's going on with the failure on Static Analysis here? |
|
Could it be because of the prettier dependency change? Is that even intentional or it got here during a bad rebase? |
|
I disabled 'auto-merge' since lint checks are failing, and there's still an ongoing discussion. @kevin940726, I remember you explored the idea of ignoring methods returned by |
I don't remember that 😅 . I think we can just add them to the dependencies array, it might be the simplest way. This is how |
|
If the lint still fails then it might be we need to rebuild |
|
Flaky tests detected in 8358467. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4135223140
|
Seems like that was the cause. |
| @@ -171,6 +171,10 @@ | |||
| - The bundled `eslint-plugin-react` dependency has been updated from requiring `^7.20.0` to requiring `^7.22.0` ([#27965](https://github.com/WordPress/gutenberg/pull/27965)). | |||
| - The bundled `eslint-plugin-react-hooks` dependency has been updated from requiring `^4.0.4` to requiring `^4.2.0` ([#27965](https://github.com/WordPress/gutenberg/pull/27965)). | |||
|
|
|||
| ### New Features | |||
There was a problem hiding this comment.
This changelog entry is misplaced. It should be moved to ## Unreleased section.
We will have to rethink the way changelogs are maintained, as it's too easy to run accidentaly into this type of situation when merging the latest changes from trunk.
There was a problem hiding this comment.
We will have to rethink the way changelogs are maintained,
100%. I was thinking if we could semi-automate the changelog by asking the PR author to simply state:
- which category this falls under (bug fix, enhancements, breaking change...)
- message
The rest can be performed by the automated action at merge time, including picking the right CHANGELOG file and adding the message under the right version (or unreleased) ?
There was a problem hiding this comment.
I was looking at Jetpack monorepo the other day, and they use an interesting system.
They keep a changelog folder for every package:
Every time a changelog entry is warranted (I don't know the rules), they create a new file:
Those files are read and deleted at publishing time, which completely removes the risk of merge conflicts of any type.
Example PR from Jetpack with changelog entries included: Automattic/jetpack#28710.
There was a problem hiding this comment.
That's interesting!
It could be worth chatting to whoever implemented / maintains that system to understand pros and cons, and if it could work well for our use case. For example, I see that they have a CI action that replies to the PR with a checklist, suggesting that a CHANGELOG entry is a mandatory requirement for all PRs?
Description
As suggested in the Rules of hooks guide in the official doc, it's highly recommended to enable the
react-hooks/exhaustive-depseslint rules to prevent us from writing fragile code.There are currently a lot of violations to the rules, this PR doesn't intend to fix them now, but only emit warnings to prevent breaking backward compatibility.
I have to also upgrade
eslintandeslint-plugin-react-hooksto support optional chaining.How has this been tested?
Run the
npm run lint-jscommand.Types of changes
New feature
Checklist: