User autocomplete - use debounced search request #4406

adamsilverstein · 2018-01-11T14:01:16Z

Description

Looking into user autocomplete (typing @ and then starting to type a username, see #2896), specifically the code for userAutocompleter: currently, it attempts to load all users with a single fetch via new wp.api.collections.Users() ).fetch().then( ( users ) => {.... While this works for testing with a few users, it is not scalable. By default the REST API users endpoint returns 10 users and the default maximum for REST requests is 100 records. Some sites might have hundreds or even thousands of users, so the filtering applied as you type won't work.

This should instead use a debounced search. Every time the user types and pauses briefly, fire off a request to the REST API with the typed search string, this will work: new wp.api.collections.Users() ).fetch( { data: { search: searchString } } ).then( ( users ) => {....

Addresses #2793 (comment)

How Has This Been Tested/How to test

Load a post and type @ and start typing a username.
Check the network console, you will see requests being sent with a search string
Note the requests are sent at most every 250ms which should feel immediate while preventing over requesting

Screenshots (jpeg or gifs if applicable):

Types of changes

debounce search callback is autocompleters/index.js
in search callback, in autocompleter popover is open and selected autocompleter exposes a setSearch function, calls this function with the current search string on every (debounced) input event

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows has proper inline documentation.

adamsilverstein · 2018-01-11T14:06:55Z

This PR breaks some tests that need fixing if/when we add this. Also, I'm wondering if calling new wp.api.collections.Users over and over is a good idea (is it getting garbage collected?), seems like it would be more efficient to instantiate this object once, then re-use it.

BoardJames

A good idea but needs some changes.

BoardJames · 2018-01-11T23:57:50Z

components/autocomplete/index.js

+		this.doSearch = ( event ) => {
+			// Ensure the synthetic event can be handled asynchronously.
+			event.persist();
+			this.debouncedSearch( event );


search only uses event.target so you should be able to avoid using persist if you extract it.

aha! I struggled to figure this out, I thought because of React's synthetic events I had to persist when debouncing... So if I extract search hereI won't need the event later. I'll change that.

BoardJames · 2018-01-11T23:59:08Z

components/autocomplete/index.js

@@ -131,6 +131,14 @@ export class Autocomplete extends Component {
 		this.search = this.search.bind( this );
 		this.handleKeyDown = this.handleKeyDown.bind( this );
 		this.getWordRect = this.getWordRect.bind( this );
+		this.debouncedSearch = debounce( ( e ) => {
+			this.search( e );
+		}, 250 );


I wonder if all these debounce's are really needed, the search code is already pretty efficient and tends to exit early when it can.

yes, when typing a search term with an asynchronous source, debouncing is appropriate. before adding the debounce I was getting a search fired for every character I typed. When I quickly type 'cat' I would expect only one search for 'cat' not three searches for 'c', 'ca', and 'cat'

Yes, but in that case the debounce should be in user search code in autocompleters, not in the general code in autocomplete. The general code may still be used to search things that have a fixed list (ie the forward-slash block completer).

Yea, good point. I'll move it there.

BoardJames · 2018-01-12T00:11:06Z

components/autocomplete/index.js

 		// check if we should still suppress the popover
 		const suppress = ( open && wasSuppress === open.idx ) ? wasSuppress : undefined;
 		// update the state
 		if ( wasOpen || open ) {
+			if ( open ) {
+				if ( this.props.completers[ open.idx ].setSearch ) {


To make this clearer to read in the multiple places it is used I would extract this.props.completers[ open.idx ] into a constant, ie:

const completer = this.props.completers[ open.idx ];

BoardJames · 2018-01-12T00:13:37Z

components/autocomplete/index.js

+				if ( this.props.completers[ open.idx ].setSearch ) {
+					this.props.completers[ open.idx ].setSearch( query, this.props.completers[ open.idx ].getOptions ).then( ( options ) => {
+						const keyedOptions = map( options, ( option, i ) => ( { ...option, key: open.idx + '-' + i } ) );
+						filteredOptions = filterOptions( this.state.search, keyedOptions );


As this code is asynchronous I would not reuse the filteredOptions variable (call it asyncFilteredOptions or something) because the code that comes after this async block on the page will not get access to the updated value.

Right, good point, I'll rework this part.

BoardJames · 2018-01-12T00:17:30Z

components/autocomplete/index.js

+					this.props.completers[ open.idx ].setSearch( query, this.props.completers[ open.idx ].getOptions ).then( ( options ) => {
+						const keyedOptions = map( options, ( option, i ) => ( { ...option, key: open.idx + '-' + i } ) );
+						filteredOptions = filterOptions( this.state.search, keyedOptions );
+						const selectedIndex = filteredOptions.length === this.state.filteredOptions.length ? this.state.selectedIndex : 0;


I know my code probably made a similar bad assumption but just because the lengths are the same the item at that position may not be the same - something that will probably be more noticeable with the new search code. It might be desirable to search the filtered options?

yea, good point, the slug should be unique. i'll work on this some more.

I removed my copy of this code block, the length check seems to work, I guess in some cases it might show the incorrect selection?

BoardJames · 2018-01-12T00:21:17Z

components/autocomplete/index.js

+			if ( open ) {
+				if ( this.props.completers[ open.idx ].setSearch ) {
+					this.props.completers[ open.idx ].setSearch( query, this.props.completers[ open.idx ].getOptions ).then( ( options ) => {
+						const keyedOptions = map( options, ( option, i ) => ( { ...option, key: open.idx + '-' + i } ) );


The original intent of adding the key here was that each unique option would have a unique key that was not effected by the filtering - this effects the ARIA the most because it used the key to determine if the selection had changed and needed to be re-announced. Unfortunately the dynamic server-side search breaks this assumption and I am not sure how you will fix it.

Hmm, not sure i understand. the issue is the tool will not re announce the selection, or that it will over announce when the selection hasn't changed?

could we use the item slug here which should be unique?

I reworked the keying to use slugs if available, seems to work but needs testing

Ok, make sure it works in the general case (ie for the slash completer and the proposed hash-tag completer).

BoardJames · 2018-01-12T00:24:58Z

components/autocomplete/index.js

@@ -330,11 +338,25 @@ export class Autocomplete extends Component {
 		// create a regular expression to filter the options
 		const search = open ? new RegExp( '(?:\\b|\\s|^)' + escapeRegExp( query ), 'i' ) : /./;
 		// filter the options we already have
-		const filteredOptions = open ? filterOptions( search, this.state[ 'options_' + open.idx ] ) : [];
+		let filteredOptions = open ? filterOptions( search, this.state[ 'options_' + open.idx ] ) : [];


If you rename the asynchronously calculated output further down this doesn't need to be a let.

BoardJames · 2018-01-12T00:29:59Z

blocks/autocompleters/index.js

@@ -122,6 +122,10 @@ export function userAutocompleter() {
 		return textNode.parentElement.closest( 'a' ) === null;
 	};

+	const setSearch = ( search, getOptions ) => {


Why is getOptions a parameter? Why not just pass the search?

I am calling getOptions in the inner function, I couldn't seem to call it without passing, I'll double check this

removed setSearch entirely so this no longer applies

BoardJames · 2018-01-12T00:31:08Z

components/autocomplete/index.js

 		// check if we should still suppress the popover
 		const suppress = ( open && wasSuppress === open.idx ) ? wasSuppress : undefined;
 		// update the state
 		if ( wasOpen || open ) {
+			if ( open ) {
+				if ( this.props.completers[ open.idx ].setSearch ) {
+					this.props.completers[ open.idx ].setSearch( query, this.props.completers[ open.idx ].getOptions ).then( ( options ) => {


As before, why are you passing getOptions?

gziolo · 2018-01-12T13:38:23Z

blocks/autocompleters/index.js

-	const getOptions = () => {
-		return ( new wp.api.collections.Users() ).fetch().then( ( users ) => {
+	const getOptions = ( search = '' ) => {
+		return ( new wp.api.collections.Users() ).fetch( { data: { search: search } } ).then( ( users ) => {


You can use ES6 shorthand for search field:

{ data: { search } }

Good point, changed

adamsilverstein · 2018-01-12T15:05:00Z

@EphoxJames thanks for all the feedback. I gave this a little tree shaking and simplified the changes. Can you please give it another review?

One think I'd like to figure out is how to make this extensible. In particular, for the original use case that brought about this PR, we want the autocomplete to insert the user slug instead of the user name. Would the correct approach be to make the entire component filterable with the HOC?

# Conflicts: # blocks/autocompleters/index.js

adamsilverstein · 2018-03-13T03:06:26Z

@aduth This should be ready for a merge.

aduth · 2018-03-14T01:10:17Z

blocks/autocompleters/index.js

+	const getOptions = ( search ) => {
+		let payload;
+		if ( search ) {
+			payload = { data: { search } };


This is never used? Cause of your lint error.

Ah, that was lost in a merge, fixed and verified its working again.

adamsilverstein · 2018-03-27T02:28:11Z

addressed your latest feedback, can you give this a test please @aduth?

aduth · 2018-03-28T20:55:47Z

components/autocomplete/index.js

+		this.props.completers[ index ]
+			.getOptions( query )
+			.then( ( options ) => {
+				const keyedOptions = map( options, ( option, i ) => ( { ...option, key: index + '-' + ( option.value.slug ? option.value.slug : i ) } ) );


What is slug here? Are we assuming a particularly-shaped REST API entity where the autocomplete options are otherwise generic?

Hmmm, I'm not sure we need this maybe a remnant of a previous approach, I'll revert this change

aduth · 2018-03-28T20:59:50Z

components/autocomplete/index.js

+		this.props.completers[ index ]
+			.getOptions( query )
+			.then( ( options ) => {
+				const keyedOptions = map( options, ( option, i ) => ( { ...option, key: index + '-' + ( option.value.slug ? option.value.slug : i ) } ) );


This line is excessively long. Implicit object arrow returns are hard to distinguish from arrow function body. Dense single lines ought be undesirable.

With some breathing room, and clear labeling of what we're constructing via variable assignment of the suffix.

const keyedOptions = map( options, ( option, i ) => { const keySuffix = option.value.slug || i; return { ...option, key: [ index, keySuffix ].join( '-' ), }; } );

Aside: Existed previously, but as a reader, I'm not sure what the difference between index and i are here at a glance. Maybe we ought to name each autocompleter instead of using its index as an identifier.

I renamed this variable autocompleterIndex to make it clearer.

also, unfurled that long line as you suggested to make it easier to read

aduth · 2018-03-28T21:04:17Z

blocks/autocompleters/index.js

+	const getOptions = ( search ) => {
+		let payload = '';
+		if ( search ) {
+			payload = '?search=' + search;


Does this need to be encoded?

I assume you mean just the search part of the payload, for security? otherwise I wouldn’t think its needed, as we are searching usernames?

Not necessarily for security. If someone entered @foo&bar, I assume it would (have previously) caused issue for the payload request.

ah, right '&' could cause unexpected results, good catch

adamsilverstein · 2018-04-04T14:59:18Z

Any blockers for merging this? I'd like to leverage this for #5921

mtias · 2018-04-05T08:40:58Z

@adamsilverstein thanks for working and finishing this one!

adamsilverstein · 2018-04-09T14:03:19Z

@mtias happy to help, and especially to see the effort improve Gutenberg.

Adam Silverstein added 12 commits January 8, 2018 09:29

Merge branch 'master' of github.com:WordPress/gutenberg

52dc331

Merge branch 'master' of github.com:WordPress/gutenberg

f357fce

Autocomplete search while typing, first pass

6875efa

remove unrelated changes

7afff73

remove some unused experimental code

badca63

minimize changes

6eb4432

whitespace, eslint

8f57e6e

replace a missing closing tag

9dc23b9

undo package-lock changes

ab425af

debounce the search handler

c67ef0d

debounce at 250 ms, cleanup for eslint

00f9f0f

only re-search if popover open

0edc3ff

adamsilverstein mentioned this pull request Jan 11, 2018

Autocompletion of #hash-tags or @users #2793

Closed

adamsilverstein requested review from gziolo, aduth and BoardJames January 11, 2018 14:07

BoardJames suggested changes Jan 12, 2018

View reviewed changes

gziolo reviewed Jan 12, 2018

View reviewed changes

Adam Silverstein added 10 commits January 12, 2018 08:48

Merge branch 'master' of github.com:WordPress/gutenberg

c1e831c

hoist search container to this.searchContainer; avoid event.persist()

bf356c0

Don’t pass getOptions to search

1a6c124

rename openFilteredOptions, switch to const

874dce5

remove setSearch, handle in getOptions

5f4206e

pass the query to loadOptions

62ca388

call getOptions again if query not blank

e0d288f

remove unused currentCompleter.setSearch

510f642

use slug for option key if available

135dc77

use ES6 notation for search

0806d95

Adam Silverstein added 7 commits February 20, 2018 16:46

Merge branch 'master' of github.com:WordPress/gutenberg

2151b13

Merge branch 'master' of github.com:WordPress/gutenberg

a0eedce

Merge branch 'master' of github.com:WordPress/gutenberg

ea50097

Merge branch 'master' of github.com:WordPress/gutenberg

8124f42

Merge branch 'master' of github.com:WordPress/gutenberg

22bec2f

Merge branch 'master' of github.com:WordPress/gutenberg

4be41af

Merge branch 'master' into feature/autocomplete-user-searching

d4f7a4c

# Conflicts: # blocks/autocompleters/index.js

aduth requested changes Mar 14, 2018

View reviewed changes

Adam Silverstein added 5 commits March 21, 2018 15:28

Merge branch 'master' of github.com:WordPress/gutenberg

ba2dcd3

Merge branch 'master' of github.com:WordPress/gutenberg

2ecb819

Merge branch 'master' into feature/autocomplete-user-searching

ec7e31c

restore use of search payload

8d50225

clean up search string build

4771c3e

aduth reviewed Mar 28, 2018

View reviewed changes

Adam Silverstein added 6 commits March 29, 2018 13:21

Merge branch 'master' of github.com:WordPress/gutenberg

a60ad7b

Merge branch 'master' into feature/autocomplete-user-searching

0023c9a

revert unrelated changes in loadOptions

d4e063e

rename index autocompleterIndex

7b6904b

last renaming index -> autocompleterIndex

4f98c79

wrap search in encodeURIComponent

cdba191

aduth approved these changes Apr 3, 2018

View reviewed changes

adamsilverstein merged commit e02ea64 into master Apr 4, 2018

adamsilverstein deleted the feature/autocomplete-user-searching branch April 4, 2018 15:15

adamsilverstein added this to the 2.6 milestone Apr 4, 2018

adamsilverstein changed the title ~~User autocomplete should used debounced search request~~ User autocomplete - use debounced search request Apr 4, 2018

User autocomplete - use debounced search request #4406

User autocomplete - use debounced search request #4406

Conversation

adamsilverstein commented Jan 11, 2018 • edited Loading

Description

How Has This Been Tested/How to test

Screenshots (jpeg or gifs if applicable):

Types of changes

Checklist:

adamsilverstein commented Jan 11, 2018

BoardJames left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gziolo Jan 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Jan 12, 2018

adamsilverstein commented Mar 13, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Mar 27, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamsilverstein commented Apr 4, 2018 • edited Loading

mtias commented Apr 5, 2018

adamsilverstein commented Apr 9, 2018

adamsilverstein commented Jan 11, 2018 •

edited

Loading

gziolo Jan 12, 2018 •

edited

Loading

adamsilverstein commented Apr 4, 2018 •

edited

Loading