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

Performance: Avoid string-allocation on keypress with inputRule #47094

Merged
merged 1 commit into from
Feb 10, 2023
Merged

Performance: Avoid string-allocation on keypress with inputRule #47094

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
8 changes: 5 additions & 3 deletions packages/format-library/src/code/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,16 +20,18 @@ export const code = {
__unstableInputRule( value ) {
const BACKTICK = '`';
const { start, text } = value;
const characterBefore = text.slice( start - 1, start );
const characterBefore = text[ start - 1 ];
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should add characterBefore as one of the parameters of __unstableInputRule because almost all rules will need this.

Maybe even better yet, associate the callback with a trigger character so we can built this in? There's a reason why this API is not stable 😅

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe, but do we know this is the only use-case? apart from inline code I don't know what the purpose is here. would we ever want to run this on triple-backtick code fences?

I guess at some point most replacements would have a stable trigger character, unless we wanted to re-use code for something like mentions.

in any case text[ start - 1 ] is essentially free so I don't see it causing harm to leave in before we have a good feeling for what this API should end up looking like.


// Quick check the text for the necessary character.
if ( characterBefore !== BACKTICK ) {
return value;
}

const textBefore = text.slice( 0, start - 1 );
const indexBefore = textBefore.lastIndexOf( BACKTICK );
if ( start - 2 < 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

When start is 0 then characterBefore will always be undefined. When start is 1 then this will also bail out so we don't care so much about characterBefore. Maybe this check should run before line 23?

Copy link
Member

Choose a reason for hiding this comment

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

I don't understand what is happening here.

Copy link
Member Author

Choose a reason for hiding this comment

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

my purpose here is to minimize the change to leave it as small and focused as possible. what's happening is exactly what @gziolo noted. the check ensures that we don't introduce a regression by moving to lastIndexOf, which treats its position parameter as 0 if it's negative. without the - 2 check we would conflate backticks at 0 with no previous backticks.

Copy link
Member

Choose a reason for hiding this comment

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

Would be good to comment inline

return value;
}

const indexBefore = text.lastIndexOf( BACKTICK, start - 2 );
if ( indexBefore === -1 ) {
return value;
}
Expand Down