-
Notifications
You must be signed in to change notification settings - Fork 4.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ESLint: Stop disabling react-hooks/exhaustive-deps
rule
#66324
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -165,9 +165,6 @@ export function getAutoCompleterUI( autocompleter: WPCompleter ) { | |
useLayoutEffect( () => { | ||
onChangeOptions( items ); | ||
announce( items ); | ||
// 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 | ||
}, [ items ] ); | ||
|
||
if ( items.length === 0 ) { | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We may as well include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We usually don't include There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think you have a point here. Added the ref in 8fdcc76 |
||
// hook's dependency list. | ||
// eslint-disable-next-line react-hooks/exhaustive-deps | ||
}, [ handler ] ); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
Let me know what you think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good. I'll work on retaining those comments that have context then.