Autocomplete: refactor to TypeScript #47751
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
chad1008
force-pushed
the
refactor/Autocomplete-typescript
branch
2 times, most recently
from
d448ded
to
849c494
Compare
|
Size Change: +43 B (0%) Total Size: 1.33 MB
ℹ️ View Unchanged
|
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
commented
Feb 4, 2023
chad1008
added
[Package] Components
chad1008
force-pushed
the
refactor/Autocomplete-typescript
branch
from
849c494
to
dd7f354
Compare
|
Flaky tests detected in 0dc0b0f. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4306226891
|
chad1008
force-pushed
the
refactor/Autocomplete-typescript
branch
from
dd7f354
to
bf7aa7a
Compare
mirka
reviewed
Feb 8, 2023
There was a problem hiding this comment.
Wow, major props for tackling this one! 🔥
I couldn't do a thorough review in one session so there might be some more minor things to be found in the next round, but overall things are looking good. Most of the things I noticed were minor stylistic things rather than right/wrong things, so don't feel obligated to incorporate all of those 👍
| * Autocomplete UI. These items have uniform shape and have been filtered by | ||
| * `AutocompleterUIProps.filterValue`. | ||
| */ | ||
| useItems?: ( filterValue: string ) => [ Array< KeyedOption > ]; |
There was a problem hiding this comment.
Just checking: Is this intentional? An array that always contains a single array, instead of a variable number of arrays like Array< KeyedOption >[]?
There was a problem hiding this comment.
It is intentional... this one was a bit of a rabbit hole for me actually. I probably should have left a note on it.
filteredOptions returns an array of KeyedOptions.
getDefaultUseItems uses filteredOptions to update its list of items in a layout effect. From there, getDefaultUseItems wraps that array in an outer array as its return value.
The really fun part is that when getAutoCompleterUI ultimately calls getDefaultUseItems (here being called under a different variable name), it destructures the original array back out again.
So basically, an array is cast as a tuple to serve as a return value, and then immediately destructured back to an array when the function executes. I'm not sure why array is tupled then immediately de-tupled (I don't think that's a word but I'm rolling with it 😆 ) but it's only ever one array of KeyedOptions and even then it never exists long enough to be acted upon.
What all of this boils down to (unless I'm mistaken, which is always possible) is that if a consumer did end up providing a useItems function prop that returned more than one Array< KeyedOption> the first one would get destructured to the items variable, and everything else would be ignored.
There was a problem hiding this comment.
Weird, but makes sense to me 😅
There was a problem hiding this comment.
Expanding on this one: it would be interesting to git blame and research the original reasons for why this is expected to be a single element tuple, and see if it's possible to remove what feels like an unnecessary impediment... but given the nature of a TS migration, it's definitely not part of this PR's scope, and in general definitely low priority
chad1008
force-pushed
the
refactor/Autocomplete-typescript
branch
from
bf7aa7a
to
1435169
Compare
ciampo
reviewed
Feb 13, 2023
There was a problem hiding this comment.
Taking over the PR review while Lena is AFK.
As I get myself acquainted with the work, I started leaving a few replies and comments. It's definitely not an easy job!
I also had one thought — would it worth reviewing the Autocompleter e2e tests that you had previously written, checking if the new types that we assigned to the component are respected in those tests too?
Making sure that those tests don't break any assumptions that we made in this PR is a great way to validate the correctness of the work being done here :)
| * Autocomplete UI. These items have uniform shape and have been filtered by | ||
| * `AutocompleterUIProps.filterValue`. | ||
| */ | ||
| useItems?: ( filterValue: string ) => [ Array< KeyedOption > ]; |
There was a problem hiding this comment.
Weird, but makes sense to me 😅
ciampo
reviewed
Feb 15, 2023
ciampo
reviewed
Feb 15, 2023
ciampo
reviewed
Feb 15, 2023
draganescu
reviewed
Feb 21, 2023
chad1008
force-pushed
the
refactor/Autocomplete-typescript
branch
from
c151208
to
74fd80a
Compare
This reverts commit 4bb27ee.
ciampo
force-pushed
the
refactor/Autocomplete-typescript
branch
from
74fd80a
to
7348279
Compare
ciampo
approved these changes
Mar 1, 2023
There was a problem hiding this comment.
I rebased and pushed a few last commits to unblock this PR, since Chad is AFK.
I still think that we should find a better way to type the options prop on the WPCompleter type, but that can be definitely done as a low-priority follow-up.
I gave this PR one more look, and changes LGTM 🚀
Thanks to Chad and everyone else involved in the review of this PR 🙏
|
Thank you again @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.
What?
Refactor
Autocompletecomponent to TypeScriptPart of #35744
Why?
The refactor to TypeScript has many benefits (auto-generated docs, static linting and error prevention, better IDE experience). See #35744 for more details
How?
Followed the steps in the TypeScript migration guide
Testing Instructions
@mentions, confirm they work as expectedNotes: