Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve autocompleter performance #41197

Merged
merged 1 commit into from
May 20, 2022
Merged

Improve autocompleter performance #41197

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 10 additions & 8 deletions packages/components/src/autocomplete/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import {
useEffect,
useState,
useRef,
useMemo,
} from '@wordpress/element';
import {
ENTER,
Expand Down Expand Up @@ -132,7 +133,7 @@ function useAutocomplete( {
const [ filterValue, setFilterValue ] = useState( '' );
const [ autocompleter, setAutocompleter ] = useState( null );
const [ AutocompleterUI, setAutocompleterUI ] = useState( null );
const [ backspacing, setBackspacing ] = useState( false );
const backspacing = useRef( false );

function insertCompletion( replacement ) {
const end = record.start;
Expand Down Expand Up @@ -218,7 +219,7 @@ function useAutocomplete( {
}

function handleKeyDown( event ) {
setBackspacing( event.keyCode === BACKSPACE );
backspacing.current = event.keyCode === BACKSPACE;

if ( ! autocompleter ) {
return;
Expand Down Expand Up @@ -268,11 +269,11 @@ function useAutocomplete( {
event.preventDefault();
}

let textContent;

if ( isCollapsed( record ) ) {
textContent = getTextContent( slice( record, 0 ) );
}
const textContent = useMemo( () => {
if ( isCollapsed( record ) ) {
return getTextContent( slice( record, 0 ) );
}
}, [ record ] );
Comment on lines +272 to +276
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I'm probably missing the context here, but I'm curious. What's the reason for using memo for the string value? The getTextContent doesn't look like an expensive method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To be honest, there's no particular reason for this one but my idea was that this component is kind of a bottleneck to the less we do on rendering, the better.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thank you, Riad 🙇

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we add some inline comment on these preemptive hot-path optimizations in case the opposite becomes true (and running useMemo becomes more expensive)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can do that 👍


useEffect( () => {
if ( ! textContent ) {
Expand Down Expand Up @@ -325,7 +326,8 @@ function useAutocomplete( {
// Ex: "Some text @marcelo sekkkk" <--- "kkkk" caused a mismatch, but
// if the user presses backspace here, it will show the completion popup again.
const matchingWhileBackspacing =
backspacing && textWithoutTrigger.split( /\s/ ).length <= 3;
backspacing.current &&
textWithoutTrigger.split( /\s/ ).length <= 3;

if (
mismatch &&
Expand Down