-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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.
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
Avoid fetching ALL reusable blocks (user patterns) on post/site editor load #58239
Conversation
@@ -258,7 +258,7 @@ function useBlockEditorSettings( settings, postType, postId ) { | |||
} | |||
); | |||
}, | |||
__experimentalReusableBlocks: reusableBlocks, | |||
__experimentalReusableBlocksSelect, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
? reusableBlocksSelect( select ) | ||
: __experimentalReusableBlocks ?? [] | ||
).map( ( userPattern ) => { | ||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
return ( | ||
select( coreStore ).getEntityRecords( 'postType', 'wp_block', { | ||
per_page: -1, | ||
} ) ?? EMPTY_BLOCKS_LIST |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's merge this, we can tidy up the private APIs after all the settings for fetching stuff are migrated to functions or selectors.
f753ec0
to
fef6b2a
Compare
@@ -2299,12 +2321,12 @@ export const __experimentalGetParsedPattern = createRegistrySelector( | |||
__unstableSkipMigrationLogs: true, | |||
} ), | |||
}; | |||
}, getAllPatternsDependants ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At some point we should change __experimentalGetParsedPattern
so it doesn't depend on all patterns...
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. |
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. 🤔 |
if ( ! reusableBlock ) { | ||
return null; | ||
} | ||
export const __experimentalGetReusableBlockTitle = createRegistrySelector( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI, I create PR to deprecate this selector and remove its usage from the core - #60278.
…r load (WordPress#58239) Co-authored-by: ellatrix <[email protected]> Co-authored-by: jsnajdr <[email protected]>
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