Components: refactor ComboboxControl to pass exhaustive-deps#41417
Merged
Components: refactor ComboboxControl to pass exhaustive-deps#41417
ComboboxControl to pass exhaustive-deps#41417Conversation
44 tasks
|
Size Change: -2 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
approved these changes
May 30, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
Thank you @chad1008 for working on this!
This component, in its current "hook" form, was first added in #25442 — including the useMemo hook in question and value in the list of dependencies. Skimming through the PR, I couldn't find any particular mention justifying value as an extra dependency.
The lack of unit tests for this component give us less confidence when making this sort of changes, but the code changes LGTM at a first glance. I also had a look at Storybook and I the component seems to work as expected.
I'm going to approve this PR, but in the meantime let's see if @youknowriad has anything to add to clarify your doubts.
Contributor
Author
|
I've just done another pass of manual testing in Storybook and everything appears to be behaving as expected with this change. @ciampo what are your thoughts at this point? Should we merge, or wait for feedback from @youknowriad? |
ciampo
reviewed
Jun 29, 2022
Contributor
ciampo
left a comment
There was a problem hiding this comment.
@ciampo what are your thoughts at this point? Should we merge, or wait for feedback from @youknowriad?
@youknowriad won't be around for the next couple of months.
I would probably give a final round of testing in the editor, before merging, and then I'd keep an eye open for any potential bug reports on UIs using this component.
Alternatively, depending on how you feel about it and your availability, we could also open a PR to add a few unit tests to this component, in order to gain more confidence for the changes in this PR — up to you, both options are ok to me.
stokesman
reviewed
Jul 8, 2022
f8cd1c5 to
521250f
Compare
Contributor
Author
|
First pass at some unit tests in this draft. It doesn't have everything I'd like, and I'm open to input on what more I should add! For now though, this PR is passing those proposed tests :) |
Contributor
Awesome. Once #42403 is merged, we can rebase this PR and merge it too in case all unit tests are ✅ |
521250f to
61adc8e
Compare
Contributor
Author
|
The new unit tests are merged, and passing locally after a rebase. Letting the tests run here in GH and then we can merge this as well! |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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
ComboboxControlcomponent to satisfy theexhaustive-depseslint ruleWhy?
Part of the effort in #41166 to apply
exhuastive-depsto the Components packageHow?
I think we can safely remove
valuefrom thematchingSuggestionsuseMemodependency array.valueis a prop that, as far as I can tell, doesn't impact that matches based on current input.The main risk here felt like the impact of the parent component updating
valueon us mid-search, so I've tested this in Storybook by adding an input that firessetValueon a five second delay. During that delay I began typing into the ComboBox and waiting for thevalueto change causing a rerender. When it did, the current matches didn't appear to be disrupted, and there was no impact in selecting an option based on the current input.Just in case, @youknowriad, do you have any recollection of the details on adding
valueto this dependency array? It was a long time ago, but I wanted to double check in case I've overlooked something :)Testing Instructions
npx eslint --rule 'react-hooks/exhaustive-deps: warn' packages/components/src/combobox-controlComboboxControl's stories/docs to ensure they still work as expected with no console errors