We need to standardize the way we name filters to have one pattern that is easy to follow. So far we were prefixing the hooks name with the package name followed by the component's name. So, in this case it would be something like: components.Autocomplete. Another thing here is that we can combine those 3 filters into one as follows and call it once only when the component gets initialized:

this.props.completers = applyFilters( `components.Autocomplete.completers', this.props.defaultCompleters );

If you want to make sure this is called only once you can use lodash.once

const getOptions = once( () => Promise.resolve... );

Thanks for the tip! That's cleaner.
I'll make the update.

I don't know this code so just wanted to double check why to remove onReplace here.

It started with a dependency issue when moving completers to block registration. We didn't yet have an onReplace.

Then I thought completers should be more declarative anyway. The presence of an onSomething name made me think about that. Why should a completer know that it is replacing something in the editor? The idea is that the editor has a better idea of what it wants to do with the result of a completer selection.

I think it ensures we remove the existing paragraph block.

The Autocomplete component now calls onReplace instead when a block is returned.
ae62091#diff-8d7b3f0b32214bb1340578e3be07ed96R176

The idea is that a completer should not be involved in making actual changes to the content.

I like this part, it makes it nicely composable out of smaller pieces.

Is there maybe an existing API method which validated if the input is a block? There is a similar logic in here:
https://github.com/WordPress/gutenberg/blob/master/blocks/block-edit/index.js#L22

We might want to extract this code and put in the method that would be used it in both places.

Agreed. I'll look at that.

I took a look and am wondering whether it is worth it to a function for the check. The only thing in common is that both make sure a truthy value is returned from getBlockType. Defining an isBlockType function in blocks/api/registration may allow us to be more direct though.

Currently, I simply moved the conditions from components/Autocomplete to blocks/contextual-autocomplete in the form of the isReplacement prop.

It looks like you can perform all assertions inside this loop.

Updated.

Or can we make it more explicit what is what? At the moment it might work, but it's hard to decipher what we are checking here.

Updated.

We might want to iterate on getBlockType passed as prop. Maybe it should be a part of the autocompleter object. Not sure.

This will go away when I make the update for BlockEdit to provide autocompleters via context. There will be no need to find the containing block type to provide autocompletion context because that will be provided naturally via React context.

It probably makes the most sense to have it here 👍 Let's see how it goes :)

Is there a particular use case we have in mind for having a block type define what autocompleters it wants? (I like to err on the side of you aren't gonna need it, especially since registerBlockType is a public API.)

You might want different autocompleters per block. A concrete example is that a block autocompleter makes sense in a paragraph but not in the caption field of a cover image. If you offer block completion there, it is more likely an opportunity for surprising replacement than a feature IMO.

What If you want different autocompleters per RichText? is this a valid use case? could this be a prop of RichText instead?

I'm not sure, but it seems reasonable to want different autocompleters per RichText. I'm not sure what overriding should look like in that case though.

I'm wondering whether this PR is trying to do too much and, in retrospect, would like to have created an autocompletion extensibility issue for discussion and smaller PRs. But if we can adjust in the future, maybe it would be good to continue as-is and get more feedback once merged.

Sounds good to me @brandonpayton

Yes, we might want to inject this, too. This is really interesting :)

There are 3 additional divs in here, at least 2 of them look obsolete. It should be investigated further. Hopefully, it can be fixed.

I noticed this as well and was initially puzzled, but they are divs added by the Autocomplete component now used within RichText:

• Autocomplete adds a div to wrap TinyMCE, adding event listeners and a sibling popup.
• withFocusOutside wraps Autocomplete and adds a parent div because it adds event listeners.
• The autocompleters hook adds another parent div in order to add its own event listeners.

I dislike this but do not currently see a better solution. I would love to be able to add event listeners to a Fragment (mentioned here as a possible future capability) to avoid rendering the elements. I'll give it some thought. Do you have any suggestions?

completers is not used inside the function, should be removed.

completers is not used inside the function, should be removed.

It can be also simplified to:

updateFilteredCompleters( nextCompleters = defaultCompleters ) {

by using a default param value.

Thanks, I'm not sure how I missed that :). Based on one of your other comments, I've updated the PR to provide defaults via a filter instead, so this no longer applies.

There is also hasFilter method which would be useful here to avoid state updates when there are no filters registered.

In this case, the state must always be updated after new completer props are received because the filtered completers are maintained in the state. This is done on demand when the component first contains focus after receiving new completer props.

We can still use hasFilter to avoid copying the completers list when there is no filter added, and I'm making that change now.

There is a convention to prefix all HOCs with with prefix. In this case, it might be better to name it withFilteredAutocompleters.

It'd be less code to put wrapper.unmount() inside afterEach call instead of using try / finally in all tests.

I was actually doing it that way initially but switched away because I thought I would be able to use shallow for most of the tests. I was wrong. :)

I started to prefer the explicit try/finally because it is easy to accidentally declare a local const wrapper that shadows the outer variable, but I just noticed we have a no-shadow lint rule that covers this case.

I'll switch it back.

As far as I understand what happens here, it seems like it would be enough to have only one filter registered for autocompleters: components.Autocomplete.completers.

Most of the logic that is inside this file is directly related to the Autocomplete component. Instead of adding filter on the component.Autocomplete, it is also possible to set default autocompleters using:

const setDefaultAutocompleters = () =>  [ ...defaultCompleters ];
addFilter(
    'components.Autocomplete.completers',
    'core/autocompleters/set-default-autocompleters',
    setDefaultAutocompleters
);

I'm not sure I correctly understand your comment, but my reading is that this component wrapper may not be necessary. Is that correct?

The purpose of this module is to filter autocompleters passed to Autocomplete components and to provide default completers when none are provided. The filtering is done on-demand per Autocomplete component when focus first enters the component after receiving new completers via props.

Your comment made me realize it would be more idiomatic to provide default completers via a components.Autocomplete.completers filter, and I've made that update.

There were some major updates to the extensibility docs. I think this should now land into its own file.

OK, I will move them in an autocompletion.md doc. Does that work for you?

It all depends if you decide to have it as block specific or component specific :)

Here is the document for Extending Blocks.

It might be also its own Extending Components document. Autocompletion might be too granular.

wp.components.Autocomplete was exposed for some time. Is there any way to make it backward compatible for the next 2 release cycles? We should also add a deprecation message to give a warning about the changes.

We should be able to detect a difference in the completer objects and log a deprecation message then. Do we want to limit such messages or print for every completer object seen with the old interface?

In other cases, it was done per function/component occurrence. See #5833 for reference.

I have a feeling that this should be documented next to the original Autocomplete component. It is also breaking change as far as I understand. We should provide some notes how to update code when someone was using wp.components.Autocomplete.

I tend to agree with you. I don't know why the completer interface should be documented within @wordpress/blocks and not with the wp.components.Autocomplete component. I plan to move the completer interface JSDoc into components/autocomplete/index.js. Sound reasonable?

The blocks completer probably belongs within @wordpress/blocks.

I feel better keeping the user completer within @wordpress/blocks as well because @wordpress/components seems to be more generic than the users module which actually hits the WP REST API.

We should provide some notes how to update code when someone was using wp.components.Autocomplete.

Where is a good place to provide such notes?

I plan to move the completer interface JSDoc into components/autocomplete/index.js. Sound reasonable?

Yes, this was my exactly my point. It's a general interface that should work with every custom Autocomplete component.

Where is a good place to provide such notes?

I think it is enough to include deprecated function in the code and leave a link to the new interface since it is now very well documented.

See also: https://github.com/WordPress/gutenberg/pull/5398/files#diff-cf74d2aaa31578636c008cace4de69f2L13.

I'm not convinced that we should have the default setup exposed in here. We should rather set it in the hook.

Agreed. I think that is a better idea. I'll make a note to make the update once we decide what to do instead of the components.Autocomplete filter.

I still don't understand why we need components.Autocomplete filter. It is applied globally so it wraps all autocompleters. Could it be applied directly in components/Autocomplete? Or should we alternatively create BlockAutocomplete component and use it inside RichText?

I don't believe there is anything here that requires the components.Autocomplete filter. I am trying to remember my frame of mind when I wrote this. In this comment, I may have thought you were suggesting we add this functionality separately via such a filter, so it is added when blocks are in use. Now that you mention it though, I suppose block and non-block UI may exist simultaneously so that we don't want to do this for all instances.

Should autocompletion extensibility be a feature limited to @wordpress/blocks or also supported for @wordpress/components?

My mind has been set on blocks, and I have not given much thought to WordPress UI outside of blocks. I don't yet know what I think about that question, but an answer will help us know what to do instead of this unnecessary filter.

If it is a blocks-only feature, I think creating a BlockAutocomplete component makes sense to provide the on-focus behavior currently added via the components.Autocomplete filter, and we should also use a blocks-related filter name instead of the generic components.Autocomplete.completers.

I'm thinking that the opinion of a default completers list should also be limited to blocks, but if we want autocompleters to be extensible for @wordpress/components as well, I am not sure how to filter and provide defaults only within a block context because the filter could be applied in both block and non-block contexts.