Previews: avoid unneeded block selectors #60543
Merged
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
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
|
Size Change: +100 B (0%) Total Size: 1.73 MB
ℹ️ View Unchanged
|
ellatrix
commented
Apr 7, 2024
| const movingClientId = hasBlockMovingClientId(); | ||
| const blockEditingMode = getBlockEditingMode( clientId ); | ||
|
|
||
| return { | ||
| ...previewContext, |
There was a problem hiding this comment.
Btw, this makes me wonder what exactly it is below that is making the editable block slower. The pure amount of selectors, or are the particularly slow ones? One of the suspicious ones in getBlockEditingMode because it's recursive, and some of the other selectors make their own calls to getBlockEditingMode.
youknowriad
reviewed
Apr 8, 2024
| : undefined, | ||
| defaultClassName: hasLightBlockWrapper | ||
| ? getBlockDefaultClassName( blockName ) | ||
| : undefined, |
There was a problem hiding this comment.
I'm surprised this PR doesn't cause more issues. I feel like defensively, we should have default values for all of these keys in the previewContext object no?
There was a problem hiding this comment.
Why all? Only some end up as a public API. The rest is all internal context that we have control over. Fixed the public API ones in 04c36b8.
There was a problem hiding this comment.
It was more about avoiding breakage. Like code base assuming some keys are defined...
There was a problem hiding this comment.
I can check all keys in a bit. Most of them are just used once in the block props hook, so it should be easy to double check.
There was a problem hiding this comment.
Added one more (mode) since it's a string and also public API. But the rest all end up in a classnames call or falsy checks.
youknowriad
reviewed
Apr 8, 2024
| return { | ||
| order: _order, | ||
| selectedBlocks: EMPTY_ARRAY, | ||
| visibleBlocks: EMPTY_SET, |
There was a problem hiding this comment.
this one is interesting because it forces all blocks to be async right? This might have a good impact in terms of performance for the different interactions when previews are rendered.
There was a problem hiding this comment.
(Idea for later)
I actually wonder if we can't introduce a "priority" number in priority-queue at some point where we could say:
- First render the sync stuff
- then render async stuff with priority 1
- and only then (maybe wait a couple async callback before running this list), render async stuff with priority 2
There was a problem hiding this comment.
They are already async because there is never any selection in previews. What would be priority 2?
There was a problem hiding this comment.
The early return here is just to prevent the selectors from running, especially getBlockEditingMode below.
There was a problem hiding this comment.
They are already async because there is never any selection in previews. What would be priority 2?
I mean a way to choose that some components even if async are higher priority than other async components.
youknowriad
approved these changes
Apr 8, 2024
|
Last result. I've noticed in other runs on other PRs that navigate is very unstable. |
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: youknowriad <youknowriad@git.wordpress.org>
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.
What?
Not sure if this will do much, but we can avoid lots of selectors in the block component if we're in preview mode.
Why?
load patterns (site editor, swap template): -9.39%, -16.91%, -14.03% = -13.44% average
navigate: -7.98%, -14.04%, -16.25% = -12.76% average
How?
Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast