Fix performance regression for isEditedPostEmpty selector #52759
Conversation
This file contains 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
|
Size Change: +59 B (0%) Total Size: 1.44 MB
ℹ️ View Unchanged
|
|
This definitely improves the typing performance. Screenshot
|
Mamaduka
approved these changes
Jul 19, 2023
|
|
||
| return ! getEditedPostContent( state ); | ||
| } | ||
| return ! getEditedPostContent( state ); |
There was a problem hiding this comment.
This last return value probably can be derived from record, but we can also leave the selector for simplicity.
| @@ -1660,7 +1665,7 @@ describe( 'selectors', () => { | |||
| expect( isEditedPostEmpty( state ) ).toBe( true ); | |||
| } ); | |||
|
|
|||
| it( 'should return false if blocks, but empty content edit', () => { | |||
| it( 'should return true if blocks, but empty content edit', () => { | |||
There was a problem hiding this comment.
I'm guessing the original test assumption was incorrect.
There was a problem hiding this comment.
Yes, IMO, if there's an edit to "content", it should take priority.
| @@ -1709,7 +1714,7 @@ describe( 'selectors', () => { | |||
| expect( isEditedPostEmpty( state ) ).toBe( true ); | |||
| } ); | |||
|
|
|||
| it( 'should return true if edits include a non-empty content property, but blocks are empty', () => { | |||
| it( 'should return false if edits include a non-empty content property', () => { | |||
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fix performance regression introduced in #52417
What?
The
isEditedPostEmptyis a super optimized selector and since it relied on thegetEditorBlocksselector that was updated in #52417 to deal with the fact that the "blocks" edit might not always be present, the performance of the former degraded. This PR applies a small trick to theisEditedPostEmptyselector to restore the previous performance.How?
The small trick is that when there are no edits, the "content" property of a post is a string and checking if the string is empty is actually faster than checking the blocks (which would require parsing).
Testing Instructions
Check that the "typing" metric in the results is actually giving the right results here.
Aside: To be honest the selector is only used to check whether to show the "save button" or not on empty posts. But I personally feel that the button should always be available if there are "edits" regardless of whether the content is empty or not. In other words, I think we can probably "deprecate"/remove/not use that selector entirely, but I'm leaving that separate.