Autocomplete: Remove getAutoCompleterUI factory pattern#77048
Merged
Autocomplete: Remove getAutoCompleterUI factory pattern#77048
Conversation
Mamaduka
added
[Type] Enhancement
Mamaduka
commented
Apr 6, 2026
| selectedIndex={ selectedIndex } | ||
| onChangeOptions={ onChangeOptions } | ||
| onSelect={ select } | ||
| value={ record } |
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: -31 B (0%) Total Size: 7.74 MB 📦 View Changed
ℹ️ View Unchanged
|
|
Flaky tests detected in ff9c2f2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/24141871817
|
mirka
reviewed
Apr 6, 2026
| } | ||
|
|
||
| export function getAutoCompleterUI( autocompleter: WPCompleter ) { | ||
| export function AutocompleterUI( { |
Member
There was a problem hiding this comment.
Since there is no index.native.js, it seems like changing the exports at this level would break the React Native version 🤔 Do you know?
Member
Author
There was a problem hiding this comment.
I'll double-check and avoid errors.
Member
Author
There was a problem hiding this comment.
Applied the same refactoring to the native file - 868344b.
Member
There was a problem hiding this comment.
Ideally, we would be able to pass the autocompleter object as a key, it has exactly the kind of uniqueness that we need. And it was also the previous behavior, the setAutoCompleterUI call checked oldAutocompleter !== newAutocompleter before updating.
But React.Key must be a string | number. One way to simulate this is:
let completerId = 0;
function useAutocompleterKey( autocompleter ) {
return useMemo( () => ++completerId, [ autocompleter ] );
}Another thing that would be nice if the "reset completely on autocompleter change" behavior was internal inside AutocompleterUI. It shouldn't be up to the user to pass the expected key value. Can be done with a simple wrapper around the inner component.
Member
Author
There was a problem hiding this comment.
Ideally, yes. But that also assumes that autocompleter will always have a stable (enough) reference, which isn't reliable; we don't know what the autocompleter filter returns. Here's one example.
I think the current key combo should remount only when needed.
Another thing that would be nice if the "reset completely on autocompleter change" behavior was internal inside AutocompleterUI. It shouldn't be up to the user to pass the expected key value. Can be done with a simple wrapper around the inner component.
Can you elaborate a bit more on this? The AutocompleterUI is an internal component, so I don't see a problem with core handling the key logic.
Member
There was a problem hiding this comment.
But that also assumes that
autocompleterwill always have a stable (enough) reference, which isn't reliable
But existing code always assumed this. When I look at #77020, both the old code (useState) and the new useMemo calls getAutocompleterUI every time autocompleter object identity changes. Was there a bug that we are fixing now, that the UI was re-created too often?
Can you elaborate a bit more on this?
It's a minor nit: the user needs to pass the right key so that the component internals work correctly. Ideally this would be encapsulated. Not a big problem with a private internal component, but we probably both agree that for a public component this would be worth improving.
Member
Author
There was a problem hiding this comment.
I couldn't find any existing issue at the moment, but I think the fix from #61877 was one of the side effects. Avoiding recreating components or unmounting when it's not needed is a good practice, and the current key shouldn't cause any issues, IMO.
But existing code always assumed this.
Unfortunately, it's an incorrect and unreliable assumption for the time being.
Not a big problem with a private internal component, but we probably both agree that for a public component this would be worth improving.
Sure. For iternals I think it's just fine. React docs also suggest/recommend similar uses.
| onKeyDown: withIgnoreIMEEvents( handleKeyDown ), | ||
| popover: showPopover && ( | ||
| <AutocompleterUI | ||
| key={ autocompleter.name } |
Member
There was a problem hiding this comment.
The fact that a name property would be a unique remount key is not obvious from reading the docs, and it's unclear to me whether there are parts of the autocompleter config that could be changed in place while maintaining the same name.
In Gutenberg, this only seems to be used in one specific place in @wordpress/block-editor (almost like this component should just live in that package), so not sure how much thought needs to go into arbitrary third-party usage.
In any case, there should probably be some documentation about this new behavior, at the very least in the prop description for name.
Member
Author
There was a problem hiding this comment.
I think the assumption was always to refer to name as a unique ID, though it's not enforced. Hard to do that via a filter vs. a proper registry API. I can update the docs to highlight this more.
To avoid duplicate keys, we could use the following combo autocompleter.name + autocompleter.triggerPrefix. They should get us as close to a unique value as possible.
Mamaduka
force-pushed
the
refactor/autocompleter-ui-component-factory
branch
2 times, most recently
from
868344b to
ea20056
Compare
jsnajdr
approved these changes
Apr 8, 2026
Member
jsnajdr
left a comment
jsnajdr
left a comment
There was a problem hiding this comment.
Looks good, including the value and onReset prop cleanups 👍
Converts AutocompleterUI from a factory-returned closure component into a standalone component that receives autocompleter as a prop. The factory pattern meant every call created a new component definition — if useMemo stabilization broke, React would unmount/remount the entire subtree. Now AutocompleterUI is rendered with key={autocompleter.name} to handle completer changes cleanly.
Mamaduka
force-pushed
the
refactor/autocompleter-ui-component-factory
branch
from
ea20056 to
ff9c2f2
Compare
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?
Continues work from #61877.
Converts
AutocompleterUIfrom a factory-returned closure component into a standalone component that receives autocompleter as a prop.The factory pattern meant every call created a new component definition - React would unmount/remount the entire subtree. Now
AutocompleterUIis rendered withkey={ autocompleter.name }to handle completer changes cleanly.Testing Instructions
Testing Instructions for Keyboard
Same.
Use of AI Tools
Used Claude to execute my idea of refactoring.