Lazy load block edit functions #55585
Conversation
|
Size Change: -115 kB (-6%) ✅ Total Size: 1.78 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in b007e3c. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/8170009404
|
jsnajdr
force-pushed
the
try/block-edit-lazy-loading
branch
3 times, most recently
from
5578c20 to
83feb93
Compare
gziolo
added
[Type] Performance
There was a problem hiding this comment.
I wasn't sure this was ready for review, but leaving some early comments.
This is a neat compromise between reaping lazy block loading benefits and preserving backward compatibility 👍
I've left some questions, let me know what you think.
Localizations of the edit function probably won't work. At best they'll be all bundled together as localizations for block-library. To resolve this, we'd need something like Jetpack's i18n-loader-webpack-plugin in Core. And it's a good idea to have something like this in Core. Jetpack needs this, and I think Woocommerce also has its own solution for this.
I'm curious if @yuliyan has any feedback on this part since he worked on chunking localizations (including the extra string extraction work) for lazy-loaded modules in another project.
What do you think about all the e2e flakiness and failures introduced by this PR? Have you been able to dig into the cause? Seems like many of them could be solved by making the tests aware of waiting for the block to load.
Finally, are you planning any other changes before marking this as ready to review?
| @@ -17,7 +17,9 @@ export { metadata, name }; | |||
| export const settings = { | |||
| icon, | |||
| example: {}, | |||
| edit, | |||
| edit: lazyLoad( () => | |||
| import( /* webpackChunkName: "archives/editor" */ './edit' ) | |||
There was a problem hiding this comment.
Nit about the naming choice: is there a good reason to have the webpack chunk name with a different name than the original module name? The convention is to name them edit.[jt]sx? so why not name the chunk names archives/edit for example?
There was a problem hiding this comment.
Here I followed the existing convention for editor styles, which are in editor.css files, one for each individual block.
There was a problem hiding this comment.
The subtle difference is that editor.css generated might contain CSS that gets applied not only to the block's edit, but also to inspector controls, toolbars, or anything necessary to make the block editing experience coherent. This CSS might also be tied to the deprecated version of the block, or save function.
Anyway, I don't have strong feelings about the name. I'm curious whether this code comment is essential to make the feature work, though. The other question is how all that would work with custom implementations of the editor that don't use webpack. Example: https://github.com/WordPress/gutenberg/blob/trunk/platform-docs/docs/intro.md which is powered by Vite.
| import { Suspense, lazy } from '@wordpress/element'; | ||
|
|
||
| export default function lazyEdit( cb ) { | ||
| // eslint-disable-next-line @wordpress/no-unused-vars-before-return |
There was a problem hiding this comment.
It feels off that we need to do that. Does it signal that we might need to improve the rule itself?
There was a problem hiding this comment.
Yes, this is indeed a bug in the rule, I reported it in #55552.
|
|
||
| export default function lazyEdit( cb ) { | ||
| // eslint-disable-next-line @wordpress/no-unused-vars-before-return | ||
| const Load = lazy( cb ); |
There was a problem hiding this comment.
Alternatively, what's the tradeoff of moving this line inside the Edit function?
There was a problem hiding this comment.
Load is a React component, it needs to be a stable reference to a function, otherwise it would remount on every re-render of the Edit component.
| const Load = lazy( cb ); | ||
| return function Edit( props ) { | ||
| return ( | ||
| <Suspense fallback={ null }> |
There was a problem hiding this comment.
Adding an optional prop to support a fallback UI would be a nice idea for follow-up.
There was a problem hiding this comment.
What do you think about the idea to hook the fallback UI into the global loading indicator? Can it be done?
There was a problem hiding this comment.
I wonder about this, too. Currently on a slow connection the experience looks like
test-edit-blocks.mov
Given my naive understanding of how all of this works to me a local loading indicator makes more sense and could prevent jumps? I guess there's a difference between opening a post with dozens of blocks and inserting a block on a slow connection.
There was a problem hiding this comment.
Yes, from your example it seems we'll need to design a good loading experience. The global loading indicator is good when loading an existing page. But for individual blocks, we'll need to show something local.
There was a problem hiding this comment.
Another thing we need to consider are blocks which are created while typing and not loaded yet. For example if I type *<space> and continue typing the flow is disruptive
test-list-item.mov
My intention was to type * Hello World (I was continuously typing as I would in any other app).
Edit: Discovered while looking at a failing e2e test in editor/blocks/list.spec.js.
There was a problem hiding this comment.
Feel free to experiment with the simplest spinner or any other means of inline loading experience - that shouldn't be a blocker for this particular experiment. We can iterate on improving that in a separate step where we also involve design folks.
| @@ -156,6 +162,14 @@ module.exports = { | |||
| return `webpack://${ info.namespace }/${ info.resourcePath }`; | |||
| }, | |||
| }, | |||
| optimization: { | |||
| splitChunks: { | |||
| cacheGroups: { | |||
There was a problem hiding this comment.
Could you elaborate on why we're disabling all the default cache groups? What's the tradeoff that we're deciding on here?
There was a problem hiding this comment.
If this splitChunks wasn't there, the editor chunk for the Cover block wouldn't be just one file (block-library/blocks/cover/editor.min.js), but it would be split into two. Because the Cover block uses the colord library, which lives in node_modules. By default modules in node_modules that are big enough are split into separate, "vendor" chunks. The rationale is better caching, because the vendor libraries don't change so often.
I wanted to avoid this, I wanted to keep the 1:1 relationship between blocks and files. And prevent generating chunks with weird names like 7001.min.js.
There was a problem hiding this comment.
The main question is: does this make the block-library script significantly smaller? The answer is that lazy loading edit functions removes exactly 50% of the code.
It might be one of the reasons why you only noticed 50% of the code moved to chunk as shared code still needs to be bundled in the main entry point. Do you know the number with the shared vendor chunk? Well I still think that 50% saving for the initial load is good enough to roll out this prototype as it will allow us to be less strict about adding more complex functionality to individual blocks 😄
| @@ -146,6 +146,12 @@ module.exports = { | |||
| output: { | |||
| devtoolNamespace: 'wp', | |||
| filename: './build/[name]/index.min.js', | |||
| chunkFilename: ( { chunk } ) => { | |||
There was a problem hiding this comment.
Should these changes be made in the tools/webpack/block.js?
There was a problem hiding this comment.
The block webpack config only builds the view scripts, and copies the styles. All of them are already lazy-loaded, in the sense that the frontend enqueues only the view scripts of the blocks that are actually rendered. The full scale lazy loading in #51778 use the same approach for the editor scripts: they are build and enqueued separately. That PR adds its webpack bits to the block config: https://github.com/WordPress/gutenberg/pull/51778/files#diff-777759e98c224019a699e8294c23bccc34453c91b873a12cb78a828487cd5f15
But this PR is different. All the blocks are still shipped and registered at once in a big block-library script, they just dynamically load some pieces. That's why the webpack config changes are in the packages config, which builds the block-library package (script) and all others.
There was a problem hiding this comment.
Yes, I confirm it's correct. So far, wp-block-libary was a monolithic entry point with JS code for registering all core blocks in the editor. tools/webpack/block.js handles everything else: JS for the front end, CSS for the editor and front end, copying PHP files and block.json files.
Not much to add, but I think the suggested approach from Jetpack seems like a good solution for this use case as well. Also, I agree that having this in Core makes a lot of sense as many plugins are already implementing something on their own. As for the string extraction, the current language pack generation process in translate.wordpress.org should work fine as long as the function names of the gettext calls are preserved (i.e. I'm very interested to see how this topic develops and will be following the progress. Thanks for the ping! |
I think they are all caused by the slight delay between rendering a block and actually rendering a block. We'll need to tediously update the tests |
|
@jsnajdr, thank you so much for working on this prototype. It looks very promising as the changes required are minimal and fully backward compatible from the WordPress core perspective. There might be some challenges with custom editors, so we should look into integrations with other bundlers than webpack like Vite which I flagged in #55585 (comment). There are two big topics we need to explore further:
Anyway, it's great to have this problem to solve. I'm very excited to see this proposal in place that seems like a viable approach! |
|
Thanks @gziolo and @tyxla for your feedback. Until now I treated this work as an experiment, but I guess now it's the time to do the work needed to merge it into production. This is what I think needs to be done:
|
No, I don't think the 50% reduction has anything to do with shared vendor chunks and 3rd party libraries. When I look at what remains in the 
It's the registration code ( The only
This is a good point! If a dependency is no longer used by the entrypoint |
Can't we mark these as "entry points" (looping through the folders) in the webpack config maybe and maybe also have some config in place to replace the import with the chunk name (I'm guessing that's how we'll do it once it's ESmodules) |
You can automate it by switching to named chunk ids. It works fine, but extracting the block name from the named chunk id also needs to rely on some naming convention. Example: |
That's an interesting thing to try out! If we do this, the connection between the |
It is also how we handle the |
I verified this now and it works. The dependency extraction plugin has a unit test for exactly this, look up the |
Wouldn't this cause the JS file to be loaded anyway, canceling the benefits of the lazy loading? |
jsnajdr
force-pushed
the
try/block-edit-lazy-loading
branch
from
83feb93 to
40fd362
Compare
What I wanted to say that the lazy loading of edit functions doesn't break with dependency extraction plugin, i.e., the dependencies used only inside a dynamic chunk, and not used in the entrypoint, are still included in the entrypoint's But this is an interesting question anyway. The benefit is mainly that the First, we are probably not able to reliably lazy load them at this moment. To lazy load them from browser JS, the browser JS would need to know everything that the server-side Second, on closer inspection it turns out that this lazy loading would have zero impact. All the dependencies of Let's look at the 29 dependencies of
To summarize, the only library where loading dependencies only with the dynamic chunk that uses them would have a material effect is |
|
Thanks for the details @jsnajdr that makes sense for the moment. I wonder how this would be impacted when we introduce import maps. As in this case, ES modules used in the edit functions shouldn't be loaded from the start but should instead be lazy loaded as well. |
|
Excellent summary, @jsnajdr, of your investigation for the dependency extraction aspect. That's very helpful as it's reasonable to leave that aspect aside for now as it looks like lazy loading |
Before introducing import maps, the scripts like Then they can be loaded with a simple We are not there yet: the existing scripts are using all the legacy The import maps work that @luisherranz is doing is starting from scratch, from a blank slate where the legacy features don't exist. On the other hand, my block lazy loading work starts with an existing legacy codebase which uses all the |
|
There is one very interesting issue with how |
flootr
force-pushed
the
try/block-edit-lazy-loading
branch
from
40fd362 to
87e6536
Compare
jsnajdr
force-pushed
the
try/block-edit-lazy-loading
branch
from
2f1d6b8 to
b007e3c
Compare
|
A summary of where we are: the problem where we got stuck is what to do with keyboard input when a block to which the input belongs is still loading and doesn't have rendered UI. For example, consider that you type this: This is supposed to create a Quote block with a List inside, and a List Item that has a "hello" content. But, at the time when you're typing "hello", the Quote block The typing needs to go into some placeholder element which knows what to do with the input -- that it belongs to the List Item's And it gets harder: when you hit Enter or Backspace, then we're merging and splitting blocks. A lot of the knowledge on how to do that resides in the I think the best course of action is to pause the lazy block loading effort now, and return to it after some time working on other projects. If we continue developing Gutenberg being aware of certain principles, like:
Then we can gradually improve the architecture and one day it may turn out that implementing block lazy loading is much easier on the second attempt. |
This is an experiment to wrap block
editfunctions withReact.lazy, offloading theeditcode into lazily loaded modules. Theblock-libraryscript now contains only the block registration code without theeditfunctions. Block attribute schema, support info, transforms, variations, migration code, icons.The main question is: does this make the
block-libraryscript significantly smaller? The answer is that lazy loadingeditfunctions removes exactly 50% of the code. The rest, the non-editregistration code, remains statically loaded. For me this result is a bit disappointing, I expected more. The "full scale" block lazy loading in #51778 removes 100% of the static code, theblock-libraryscript completely disappears there. But on the other hand the 50% approach is much easier to implement and has no backward-compatibility issues.How does it work? It uses webpack native dynamic chunks in combination with React's
SuspenseandReact.lazy. I needed to amend the webpack configuration to setup writing the dynamic chunks into the right subdirectories of./build/block-library, with the right file names, and to prevent writing the 3rd party packages fromnode_modules(likecolord) into separate "vendor" chunks. No WordPress-specific script code likewp_enqueue_scriptor*.asset.phpfile are not needed. Theblock-libraryscript starts shipping the webpack chunk loading runtime, and loads individual chunks from URLs relative to the mainblock-libraryscript's URL.The same approach can be used to dynamically load specific libraries, like the
compressorjslibrary in the Image block, as mentioned by @youknowriad recently.ES modules and import maps
The next step would be to eliminate the webpack runtime and use native ES modules. The
import( './edit' )statement would remain untouched, except updating the./editmodule name to something unique and symbolic. The import would be performed natively by the browser, according to a generated import map. This should be achievable with a handful of webpack plugins.But would it really work in practice? I have some doubts. The ES modules are all-or-nothing, are hard to combine with non-modules. What if the
./editmodule wants to be a bundle (chunk) of multiple modules? Webpack can bundle multiple modules into a single chunk, but how would that work with native modules? Can we really "resolve" all the internal imports and exports, and expose only the exports of the top-level module of the chunk?Also, the importing module needs to be a module. We can't use
importin a non-module. Makes it hard to incorporate ES modules into an existing non-module system.There are many nice opportunities to experiment, that's for sure, but the result will almost certainly be not shippable.
Other open questions
Localizations of the
editfunction probably won't work. At best they'll be all bundled together as localizations forblock-library. To resolve this, we'd need something like Jetpack's i18n-loader-webpack-plugin in Core. And it's a good idea to have something like this in Core. Jetpack needs this, and I think Woocommerce also has its own solution for this.Similarly, we need dynamic loading of styles.
mini-css-extract-pluginor something functionally equivalent.When loading the
editchunks for individual blocks, we currently use suspense with empty placeholder:<Suspense fallback={ null }>. What we probably really want is to plug this in into the "global loading indicator", created by @tyxla some time ago.