Components: Add exhaustive-deps eslint rule
#41166
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: 0 B Total Size: 1.29 MB ℹ️ View Unchanged
|
gziolo
added
[Type] Automated Testing
|
If possible it would be great to maintain all ESLint rules from a central location using overrides: Line 179 in 59f42b3 Otherwise it’s harder to reason how rules resolve because the information is chunked in a few places. It’d be important to reference the previous attempt to introduce this rule in #24914. There was a long discussion so I guess it isn’t that simple to work with this rule 😅 |
|
@gziolo 's advice makes sense to me — we could do something like diff --git a/.eslintrc.js b/.eslintrc.js
index 22c354394d..e70d083496 100644
--- a/.eslintrc.js
+++ b/.eslintrc.js
@@ -224,6 +224,13 @@ module.exports = {
],
},
},
+ {
+ files: [ 'packages/components/src/**/*.js' ],
+ excludedFiles: [ ...developmentFiles ],
+ rules: {
+ 'react-hooks/exhaustive-deps': 'warn',
+ },
+ },
{
files: [ 'packages/jest*/**/*.js', '**/test/**/*.js' ],
excludedFiles: [ 'test/e2e/**/*.js' ],This way, we could also widen this rule to cover more packages in the future. |
|
Awesome, I was going to ask if using overrides this way made sense, thank you! |
ciampo
added
[Status] In Progress
|
The rule has been moved to the plugin's central |
ciampo
reviewed
May 23, 2022
This was referenced May 26, 2022
This was referenced May 31, 2022
This was referenced Jun 8, 2022
This was referenced Nov 8, 2022
This was referenced Nov 9, 2022
|
All currently known One thing: currently in this PR the rule will generate warnings, should we consider erroring instead? Update: I rebased and changed the lint rule from It looks like |
ciampo
force-pushed
the
components/add-exhaustive-deps-eslint
branch
from
051e569
to
3527ad8
Compare
|
Thank you @flootr and @ciampo for knocking out those last few PRs. The list (including the recent additions) is now complete! Running |
Co-authored-by: Marco Ciampini <marco.ciampo@gmail.com>
chad1008
force-pushed
the
components/add-exhaustive-deps-eslint
branch
from
3527ad8
to
dfca6be
Compare
|
I guess the last thing to decided is whether we should |
ciampo
approved these changes
Nov 15, 2022
|
Thanks @ciampo |
|
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Note: This PR shouldn't be merged until after the list of updates below is completed to ensure everything is ready to pass!
What?
Activate the
exhaustive-depsrule to trigger warnings for missing deps in dependency arrays foruseEffect,useLayoutEffect,useMemo,useCallback, oruseImperativeHandleWhy?
Missing dependencies can lead to unexpected results like stale state values. This rule helps us guard against that.
How?
Below is a list of the components/files that currently throw
exhuastive-depswarnings. I'll be going through as many as possible (help from other contributors is of course welcome!) to analyze and udpate as needed. Once all components are passing linting,this PR can be merged, activating the rule for the entire Components package.Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/srcComponents and files to be updated:
AlignmentMatrixControlto passexhaustive-deps#41167Autocompleteto passexhaustive-deps#41382BorderControlto passexhaustive-deps#41259ColorPaletteto disableexhaustive-depscheck for now #41253ColorPickerto passexhaustive-deps#41294ComboboxControlto passexhaustive-deps#41417CustomGradientBarto passexhaustive-deps#41463DateDayPickerto passexhaustive-deps#41470Draggableto passexhaustive-deps#41499Dropdownto passexhaustive-deps#41505Flexto passexhaustive-deps#41507FocalPointPickerto pass exhaustive-deps #41520FontSizePickerto passexhaustive-deps#41600InputControlto passexhaustive-deps#41601Mobileto ignoreexhuastive-deps#44207Modalto passexhaustive-deps#41610Navigationto passexhaustive-deps#41612NavigationItemto passexhaustive-deps#41639NavigationMenuto ignoreexhaustive-deps#44090NavigatorButtonto passexhaustive-deps#42051NavigatorScreento passexhaustive-deps#43876Noticeto passexhaustive-deps#44157PaletteEditListViewto ignoreexhaustive-deps#45467 as a follow up to @flootr's work inPaletteEditListView: add missing deps touseEffectdep array #43911RangeControlto passexhaustive-deps#44271ResizableBoxto passexhaustive-deps#44370Sandboxto passexhaustive-deps#44378SearchControlnative files to ignoreexhaustive-deps#44381SlotFillto passexhaustive-deps#44403Snackbarto passexhaustive-deps#44934TabPanelto passexhaustive-deps#44935ToolsPanelto passexhaustive-deps#45028Tooltipto ignoreexhaustive-deps#45043ContextSystemProviderand theuseUpdateEffectutil to ignoreexhaustive-deps#45044After completing the tasks above, I checked for any new warnings and found the following, which I'll tackle before proceeding:
useFlexto passexhaustive-deps#45528withNoticesto passexhaustive-deps#45530ItemGroupto passexhaustive-deps#45531NavigatorScreen: satisfyexhaustive-depseslint rule #45648exhaustive-depswarning #45660useCxstory to satisfyexhaustive-depseslint rule #45614cc @mirka @ciampo