Components: refactor ContextSystemProvider and the useUpdateEffect util to ignore exhaustive-deps
#45044
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
chad1008
added
[Feature] UI Components
chad1008
changed the title
Tooltip component to temproarily ignore the exhaustive-deps eslint rule ## Why? There are several warnings in this component. One, in index.js will likely need a more comprehensive refactor to avoid unexpected changes in behavior. The rest are missing dependencies in native files. Because the Components squad generally lacks the React Native expertise and tools/environments to effectively refactor these components, we'll [defer to the native team to handle when they're ready](https://github.com/WordPress/gutenberg/issues/44226). ## How? Added inline ignore comments to all exhaustive-deps warnings in the Tooltip component directoryContext to ignore exhaustive-deps
44 tasks
|
Size Change: -16 B (0%) Total Size: 1.28 MB
ℹ️ View Unchanged
|
chad1008
changed the title
Context to ignore exhaustive-deps ContextSystemProvider and the useUpdateEffect util to ignore exhaustive-deps
I rather suspect this was a mistake done in haste, rather than intentional. Alternatively, it may have been some workaround for the somewhat limited scope of the Gutenberg TypeScript configuration in early 2021. If the hooks are the same and there is no issue with TypeScript then I see no reason not to consolidate them and delete some code 😈 |
mirka
approved these changes
Oct 18, 2022
There was a problem hiding this comment.
Yeah, seems like the disable is inevitable.
- Once we activate the rule for the Components package, would it make sense to configure it to validate the dependencies of this custom hook?
I think so!
chad1008
force-pushed
the
refactor/Context-exhaustive-deps
branch
from
42ccabf to
8af7a03
Compare
|
Thanks @sarayourfriend and @mirka! I've gone ahead and removed the local I'll update #41166 shortly to include @mirka, your approval was before the update to the |
ciampo
reviewed
Oct 20, 2022
|
Yep, still good! 🚢 |
ciampo
reviewed
Oct 20, 2022
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.
What?
Updates the two
useUpdateEffectutility hooks withinContextSystemProviderandutils/hooksto ignore theexhaustive-depseslint ruleWhy?
The hook passes a non-array literal dependency list out of necessity (it's accepting the dependencies from the consumer and doesn't know what they are in advance). Further, the
effectvalue, which is also passed in from the consumer is an additional missing dependency.Adding
effectto the deps array would require either a spread operator (which would trigger a differentexhaustive-depswarning) or some more involved logic to handle the addition... but none of this would solve the original warning described above.Thoughts/questions for the future:
ContextSystemProviderto import theuseUpdateEffecthook from utils? cc @sarayourfriend in case you have any insight 🙂How?
Added inline ignore comments to all
exhaustive-depswarning in both hooks