Avoid fetching ALL reusable blocks (user patterns) on post/site editor load #58239
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
ellatrix
commented
Jan 25, 2024
| @@ -258,7 +258,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| } | |||
| ); | |||
| }, | |||
| __experimentalReusableBlocks: reusableBlocks, | |||
| __experimentalReusableBlocksSelect, | |||
There was a problem hiding this comment.
I'll make this a private key. This seems something very WP specific right now and seems to require some more thought before exposing an editor setting for user created patterns.
ellatrix
commented
Jan 25, 2024
| @@ -137,11 +142,6 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
| hiddenBlockTypes: get( 'core', 'hiddenBlockTypes' ), | |||
| isDistractionFree: get( 'core', 'distractionFree' ), | |||
| keepCaretInsideBlock: get( 'core', 'keepCaretInsideBlock' ), | |||
| reusableBlocks: isWeb | |||
There was a problem hiding this comment.
I'm not sure why there's an isWeb condition here, because there's a native version of this file, so this will never be called for mobile.
|
Size Change: +110 B (0%) Total Size: 1.72 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in fef6b2a. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/7743726475
|
jsnajdr
reviewed
Jan 25, 2024
| ? reusableBlocksSelect( select ) | ||
| : __experimentalReusableBlocks ?? [] | ||
| ).map( ( userPattern ) => { | ||
| return { |
There was a problem hiding this comment.
For clarity, could this function be extracted as function reusableBlockToPattern( block )? The const userPatterns = ... expression is becoming quite big 🙂
| const userPatterns = ( | ||
| reusableBlocksSelect | ||
| ? reusableBlocksSelect( select ) | ||
| : __experimentalReusableBlocks ?? [] |
There was a problem hiding this comment.
reusableBlockSelect( select ) can return null if the underlying REST endpoint is still fetching. The .map that follows will crash on it.
The ?? [] bit defaults only the __experimentalReusableBlocks value. a ? b : c ?? d groups like a ? b : ( c ?? d ).
Also, it's unfortunate that the setting for patterns is a "fetch" function and the setting for reusable blocks is a "select" function. Is there a way to make the API consistent?
There was a problem hiding this comment.
Also, it's unfortunate that the setting for patterns is a "fetch" function and the setting for reusable blocks is a "select" function. Is there a way to make the API consistent?
The problem is that reusable blocks is a post type that fits with entity records, while patterns is something provided by the theme. I'm not sure what's best here. I guess we could directly fetch the reusable blocks without the entity records API. Or we move patterns back to the core-data store and leave it as a select function. The latter is a bit more restrictive, since you can't access the raw state to use in memoized selector dependencies (you have to call the selector which will resolve it).
There was a problem hiding this comment.
Btw, this is why I made it a private setting, so we have room to change it later. Maybe we should make the fetch patterns function also private.
jsnajdr
reviewed
Jan 25, 2024
| return ( | ||
| select( coreStore ).getEntityRecords( 'postType', 'wp_block', { | ||
| per_page: -1, | ||
| } ) ?? EMPTY_BLOCKS_LIST |
There was a problem hiding this comment.
Oh nevermind, now I see it's defaulted here. It suprised me because it doesn't match how a REST-bound selector with resolver usually works.
jsnajdr
approved these changes
Jan 31, 2024
ellatrix
force-pushed
the
try/avoid-fetching-all-reusable-blocks
branch
from
f753ec0
to
fef6b2a
Compare
ellatrix
commented
Feb 1, 2024
| @@ -2299,12 +2321,12 @@ export const __experimentalGetParsedPattern = createRegistrySelector( | |||
| __unstableSkipMigrationLogs: true, | |||
| } ), | |||
| }; | |||
| }, getAllPatternsDependants ) | |||
There was a problem hiding this comment.
At some point we should change __experimentalGetParsedPattern so it doesn't depend on all patterns...
ellatrix
force-pushed
the
try/avoid-fetching-all-reusable-blocks
branch
from
fef6b2a
to
a96464e
Compare
|
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. |
ellatrix
force-pushed
the
try/avoid-fetching-all-reusable-blocks
branch
from
a96464e
to
349a252
Compare
|
Let's go for it. I have more confidence in this approach now that the other setting works the same way. |
|
Something in this PR regressed preview performance, I'll have to investigate. But that's strange because we should be using the same store instance. 🤔 |
Mamaduka
reviewed
Mar 29, 2024
| if ( ! reusableBlock ) { | ||
| return null; | ||
| } | ||
| export const __experimentalGetReusableBlockTitle = createRegistrySelector( |
There was a problem hiding this comment.
FYI, I create PR to deprecate this selector and remove its usage from the core - #60278.
cbravobernal
pushed a commit
to garridinsi/gutenberg
that referenced
this pull request
Apr 9, 2024
…r load (WordPress#58239) Co-authored-by: ellatrix <ellatrix@git.wordpress.org> Co-authored-by: jsnajdr <jsnajdr@git.wordpress.org>
2 tasks
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?
Wraps the select call in a function to be passed as a block editor setting and called when needed instead of triggering the selector/resolver on page load.
Why?
This could be quite an expensive request on page load if you have a lot of reusable blocks, especially since it's unbounded (per page: -1). At some point we should probably figure out pagination.
Triggering this resolved currently in block editor settings will also cause some dispatching to the core store, recalling a bunch of selectors.
How?
Testing Instructions
Convert a block to a pattern. Save and reload white logging network requests. There should be no request for blocks.
Open the inserter, then click the patterns tab. There should be a section "My patterns". Click on it and check if your pattern is there.
Testing Instructions for Keyboard
Screenshots or screencast