-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Block lazy loading #51778
base: trunk
Are you sure you want to change the base?
Block lazy loading #51778
Conversation
Size Change: +540 B (0%) Total Size: 2.05 MB
ℹ️ View Unchanged
|
Flaky tests detected in efac9eb. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6036439195
|
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.
This is exciting to see! I'm dropping some comments as I'm passing by, but I'm looking forward to the work to make the block parser asynchronous.
It looks like this PR can serve as a primary PR and then we can land pieces from it separately. One obvious piece for example can be the part where we extract the editor entrypoints to separate modules.
Anyway, I wonder if we should think about a centralized importmap mechanism that uses the native browser import maps. It could be something that can be separately landed, but centralization will be needed, since browsers currently only support a single import map. Or do you intend to use the webpack runtime instead?
lib/init.php
Outdated
|
||
function disable_scripts() { | ||
// disable loading and enqueuing block editor scripts and styles | ||
// can't be `__return_false` because this function is already registered for this filter |
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.
Perhaps if we add it with a different priority (the third add_filter()
argument)?
lib/init.php
Outdated
if ( ! isset( $metadata['editorScript'] ) ) { | ||
if ( isset( $metadata['file'] ) && strstr( $metadata['file'], 'gutenberg') ) { |
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.
Nit: maybe these should be combined in a single if
or we could invert the top one to avoid deep nesting.
|
||
function emit_importmap() { | ||
wp_register_script( 'wp-importmap', ''); | ||
wp_add_inline_script( 'wp-importmap', 'wp.importmap = ' . wp_json_encode( get_block_importmap() ) . ';' ); |
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.
wp_localize_script()
could be an easier way to pass raw data from PHP to JS.
} | ||
|
||
function emit_importmap() { | ||
wp_register_script( 'wp-importmap', ''); |
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.
Since we're calling wp_enqueue_script()
down there, we don't need to register it at all. This line can be removed. We might only need to call the wp_add_inline_script()
or wp_localize_script()
after the wp_enqueue_script
.
|
||
function emit_importmap() { | ||
wp_register_script( 'wp-importmap', ''); | ||
wp_add_inline_script( 'wp-importmap', 'wp.importmap = ' . wp_json_encode( get_block_importmap() ) . ';' ); |
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.
Are we aiming to use webpack and not native ES modules and import maps? I'd love to hear what your thoughts are about this choice.
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.
Same here, would love to understand why we're reinventing import maps?
const src = mod.src; | ||
const newNode = document.createElement( 'script' ); | ||
newNode.src = src; | ||
newNode.onload = () => | ||
console.log( 'loaded', name, mod.handle, src ); | ||
newNode.onerror = () => | ||
console.log( 'failed to load', name, mod.handle, src ); | ||
document.body.appendChild( newNode ); |
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.
This could just be an ES import
if we use the native import maps. It could be easier to make it async, too, since we can chain additional actions import( 'mod' ).then(() => { ... } );
lib/init.php
Outdated
if ( ! empty( $metadata['name'] ) && str_starts_with( $metadata['name'], 'core/' ) ) { | ||
$block_name = str_replace( 'core/', '', $metadata['name'] ); | ||
if ( ! isset( $metadata['editorScript'] ) ) { | ||
if ( isset( $metadata['file'] ) && strstr( $metadata['file'], 'gutenberg') ) { |
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.
This may need some additional logic for when it's ported to core.
333c37a
to
80cc489
Compare
Going to report on the current status. The first step is to register all blocks, on the server, without enqueing any actual block scripts and styles. We just need to declare them in a
This server-side registration and dynamic loading now mostly works, especially for Core blocks. One thing that doesn't work well is dynamically loading styles. To register the block styles at all, I have to enable the The next step is to make the block parser (the This dynamic parsing also already mostly works. I can open post editor for an existing block, and all the blocks that it uses will be loaded, their editor UI rendered, and they can be edited. Another important place where HTML content is being parsed into blocks are block patterns. It's the The next place that is currently broken and needs work is the block inserter. Currently it displays only the blocks that have been already loaded, triggered by the post content, and other places that trigger block loading. Therefore, it displays a rather random subset of registered blocks, not all of them. Here, it would be nice if the block inserter could do all its work with just the server-sent bootstrap info about the block. What does it need? The block name, description, icon, and information needed to determine which blocks can be inserted at a particular place. I.e., the We'll see what other issues with async block loading will come up. |
0f3fa80
to
283e25a
Compare
283e25a
to
ec38016
Compare
We are now in a state where performance tests can run successfully, all the editor features that the tests use are working. The improvements in various initial load metrics are very significant and nice:
|
4053716
to
1dde417
Compare
This pull request has changed or added PHP files. Please confirm whether these changes need to be synced to WordPress Core, and therefore featured in the next release of WordPress. If so, it is recommended to create a new Trac ticket and submit a pull request to the WordPress Core Github repository soon after this pull request is merged. If you're unsure, you can always ask for help in the #core-editor channel in WordPress Slack. Thank you! ❤️ View changed files❔ lib/blocks.php ❔ lib/init.php |
f010718
to
bd140ba
Compare
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.
It's great to see this prototype. Help move things forward.
It's clear from the changes, that a lot of breaking changes are happening, most actions need to be "awaited" for now if we make parsing blocks... async. How do you see us handling this? Is there a potential path forward here without breaking changes? (To be honest, it seems very hard to me)
Is there an alternative where lazy loading blocks could happen at the block level instead. For instance, the block registration itself might be small enough (not 0) but the dependencies are the biggest part, it might be possible to extract the heavy dependencies to external lazy loaded scripts (think image editor, resizable stuff, syntax highlighting script, ...) without breaking changes.
The last thing is about the "import map" and "loader", what are your thoughts on trying import maps and I also feel that we should probably extract this from this PR and potentially try it with a new very small script like I did in #34172 before actually making the higher level abstraction (block lazy loading)
|
||
function emit_importmap() { | ||
wp_register_script( 'wp-importmap', ''); | ||
wp_add_inline_script( 'wp-importmap', 'wp.importmap = ' . wp_json_encode( get_block_importmap() ) . ';' ); |
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.
Same here, would love to understand why we're reinventing import maps?
03e3c94
to
75362cc
Compare
15c9a1a
to
0345d15
Compare
0345d15
to
efac9eb
Compare
if ( ! empty( $block_type->editor_script_handles ) ) { | ||
foreach ( $block_type->editor_script_handles as $editor_script_handle ) { | ||
if ( str_starts_with( $editor_script_handle, 'wp-block-' ) ) { | ||
wp_deregister_script( $editor_script_handle ); |
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.
To connect the efforts. I was looking in WordPress/wordpress-develop#5118 at ways to allow referencing the same file path by multiple blocks by storing a handle name in the asset file to use it as a way to prevent registering the same file under different names. As of today, WP core would register the same script file under different handles because the name is generated based on the block name.
efac9eb
to
e8680ee
Compare
One question about backward compatibility. To make the transition to async APIs, ideally, we would need to support:
That is the most extreme case, right? Something like this: // some plugin function...
echo "
<script>
const { registerBlockType } = wp.blocks;
registerBlockType( 'my-plugin/block', {
// ...
} );
</script>
"; |
e8680ee
to
e1d88a5
Compare
This is the current state of my async block loading experiment. What I try to achieve is:
On initial load of the post editor, never load any block edit scripts, or anything else that calls
registerBlockType
. The only information about blocks is the bootstrap info inwp.blocks.unstable__bootstrapServerSideBlockDefinitions
. From the bootstrap info, we know that a block exists, but everything else should be dynamically loaded.Then we setup webpack to build each Core block editor script individually, as
editor.min.js
andeditor.min.asset.php
. Until now they were all bundled in oneblock-library
package script.Instead of enqueuing the editor script, I output an "importmap". It could be a real native ES importmap, or just a plain JSON object assigned to a global
wp.importmap
variable. I'm doing the JS variable now. To use a native importmap andimport()
statements, we'll have to overcome one limitation: for each block, we can register multiple editor scripts and multiple stylesheets. The importmap can specify all of them in an array, and thenwp.importBlock( 'core/paragraph' )
can load all the registered files at once. JS, CSS, and maybe translations in the future. On the other hand, native importmap can only map 1:1, one module name to one JS script.Now, the unfinished part is to dynamically load the block scripts in the browser. The idea is to change
getBlockType
to throw an error when a block definition is not found. And replace the failing calls with a new function,async loadBlockType
, which will dynamically load the missing block. The first place to do this will be the block parser.