ESLint: Stop disabling react-hooks/exhaustive-deps rule
#66324
Conversation
tyxla
added
[Type] Code Quality
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -28 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
|
So, for now, we just ignore the warnings, correct? That works for me, but I would like to hear from the components team.
Honestly, I would like this rule set to |
Yes, like we do across the rest of the repository. I don't see why
In an ideal world, yes. However, I think we'll always have a non-ideal world, meaning many cases where you want extra dependencies or fewer dependencies than React's static analysis determines. There are workarounds for many of those instances that will satisfy all involved sides, but I think we should work on them separately. |
|
I think @ciampo summarized the case for the |
Right - that was the right choice at the time. However, this was before we had a real need to stop ignoring these warnings. |
There was a problem hiding this comment.
I echo @Mamaduka 's worries and I agree that the ideal point was for the whole project to error — removing this rule feels like a step backward.
Having said that, it sounds like we have two choices: either refactor all code instances so that we don't need to disabled the ESLint rule, or change the ESLint rule to only warn. And, practically speaking, only the second option allows the react compiler work to move forward in a reasonable amount of time.
I still wish that we put together a serious effort to remove all these exceptions to the rule, so that we can enforce it again
| @@ -235,8 +232,7 @@ function useOnClickOutside( | |||
| document.removeEventListener( 'mousedown', listener ); | |||
| document.removeEventListener( 'touchstart', listener ); | |||
| }; | |||
| // Disable reason: `ref` is a ref object and should not be included in a | |||
| // `ref` is a ref object and should not be included in a | |||
There was a problem hiding this comment.
We may as well include ref here, it won't hurt and will disable the warning?
There was a problem hiding this comment.
We usually don't include refs as dependencies because it's unexpected when reactive behavior depends on them changing. This was the sole reason to have this eslint-disable here in the first place. I'm happy to add it if you're sure about it, although this should be done separately IMHO.
There was a problem hiding this comment.
My worry is that, by having the disable comment, we will prevent future breakages of the rule from being surfaced by eslint.
In other words, if the disable comment is already there, I can make changes to the hook, forgetting to add the relevant dependencies, and ESLint won't tell me because the rule is already disabled. And the disable comment may be even more deceiving when debugging a potential issue, because it will potentially hide the real cause of the bug behind an "innocent" "refs are immutable" comment.
I think there are 2 better alternatives:
- ideally, we should be able to somehow mark
refas a react ref to eslint (or, somehow, to mark is as an immutable reference). But I don't think that's an option; - that's why I think that a potentially better compromise would be to add
refto the list of dependencies (it shouldn't hurt in practical terms), which allows us to remove the disable comment. At most, we could leave a normal text comment mentioning thatrefis there to satisfy lint rules, even if not necessary.
I would apply this reasoning in every place where it could allow us to remove the disable comment.
What do you think?
There was a problem hiding this comment.
I think you have a point here. Added the ref in 8fdcc76
| // Temporarily disabling exhaustive-deps to avoid introducing unexpected side effecst. | ||
| // See https://github.com/WordPress/gutenberg/pull/41820 | ||
| // eslint-disable-next-line react-hooks/exhaustive-deps |
There was a problem hiding this comment.
In this instance (and any instances where the disable rule has a justification / link), I think it would be useful to preserve such comment — it informs the reader to why the warning was not addressed, or at the least gives some extra context.
There was a problem hiding this comment.
I'm not sure about that. The comment made sense when we were ignoring the ESLint error, but doesn't make sense anymore IMHO. If you take a look at the rest of the instances, you'll see that I've:
- Preserved comments that add extra context on top of the sole "make exhaustive deps checks pass"
- Removed comments that only relate to "making exhaustive deps checks pass" since those are no longer relevant IMHO.
Let me know what you think.
There was a problem hiding this comment.
Makes sense I thought that, in this case, linking back to #41820 could be valuable, since it shows a previous attempt at adding dependencies that ended up being reverted — I thought that providing such context could be useful to a future reader.
In general I agree with your approach.
There was a problem hiding this comment.
Sounds good. I'll work on retaining those comments that have context then.
Thanks for the feedback! Yes, ideally, we'd refactor them all, but this is a convoluted task that I'm afraid we won't be able to prioritize soon. So I agree with you here - we'll need to go with the second option, which is, setting the rule to However, I don't see this as a step backward and I tend to disagree with the general worries, for a few reasons:
|
Although I would hope that folks would be more reluctant in ignoring an error vs a warning, your point is true.
This also sounds reasonable. I guess it's a different type of compromise with respect to the previous strategy, and not necessarily a step backwards.
Agreed. The hope, when this rule was introduced, was to raise the bar for the whole repository, and not to keep it as a special rule for the |
The error will fail the CI check required to merge the PRs, so they usually get more attention. Unfortunately, ESLint warnings are often ignored in the codebase. |
Well, if you ignore with |
|
Next steps for the PR: as @ciampo suggested, I'll work on retaining some of the old comments, and ping you folks for another review. |
…66324) * ESLint: Stop disabling react-hooks/exhaustive-deps rule * CHANGELOG * Add ref to dependencies * Retain some context in comments Co-authored-by: tyxla <tyxla@git.wordpress.org> Co-authored-by: ciampo <mciampini@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org>
What?
Un-ignore all
react-hooks/exhaustive-depsin the@wordpress/componentspackage, and remove the custom rule that was setting severity ofreact-hooks/exhaustive-depsto beerror.Why?
Any
eslint-disablewould prevent the React Compiler from working on a specific file. However, we'd like to use React Compiler for as many files as possible (see #61788 where we're experimenting with it).This came up recently in #66207 (comment).
How?
eslint-disabledirectives forreact-hooks/exhaustive-depsin@wordpress/components.react-hooks/exhaustive-depsseverity toerror.Testing Instructions
Take a look at the lint check, part of the static analysis:
Testing Instructions for Keyboard
Same
Screenshots or screencast
None