Babel Plugin Async Load: Lazy load react components when necessary

[youknowriad]

This PR tries to solve a blocker we have at the moment to ensure some of the packages are generic enough but at the same time allow lazy-loading some of their dependencies in the runtime code.

It does so by swapping the component with a lazy-loaded version at build time. Details on the README.

Testing instructions

• Add the HTML block
• Notice the editor component is lazy loaded.

[youknowriad]

This higher-order component has been added to the compose package for now. I was wondering if it should be its own package.

[aduth]

Should it be part of the Babel transform package? If it's specifically meant for handling asynchronous components, including the component loader doesn't seem too farfetched.

[youknowriad]

This still lacks some unit tests and style fixes but I wanted to have feedback.

[noisysocks]

Could you elaborate on the benefits of using a babel plugin? Why don't we manually wrap the imported component with wp.compose.lazyLoad when it is used?

(This is very cool, though 🙂)

[youknowriad]

@noisysocks because the exported package in npm shouldn't have lazy loading built-in. it should be just a regular npm dependency. (There's no WordPress script handles or WordPress Url in npm packages, they should be generic and can be used in any application)

[gziolo]

Seeing it and the comment from @noisysocks, I thought that an alternative would be to use withFilters HOC and wrap the original code with LazyLoading part.

I didn’t try it myself, but according to the description it might be an option.

[youknowriad]

@gziolo That's probably an option, it would make edit-post like the bottleneck of all these WP-heavy dependent code. I think I prefer the babel plugin approach because it seems easy to add lazy loading dynamically to any component without any change to the component itself but definitely an option to keep in mind if the babel plugin fails for some reason.

[noisysocks]

But we can wrap the component on the calling side, no? e.g.

// core-blocks/html/index.js
import { CodeEditor } from '@wordpress/components';
import { lazyLoad } from '@wordpress/compose';
const LazyCodeEditor = lazyLoad( ... )( CodeEditor );
...
edit() {
	...
	return <LazyCodeEditor />;
	...
}

Or is the idea that we don't want to rely on load-scripts.php in @wordpress/core-blocks, either?

[youknowriad]

Or is the idea that we don't want to rely on load-scripts.php in @wordpress/core-blocks, either?

Exactly, wrapping on the calling side means the calling side can't be exposed as a generic npm module.

[aduth]

Typo: "Scipts" -> "scripts"

[aduth]

Minor: Mismatched whitespace (missing before first closing parenthesis).

[aduth]

Initial reaction: Why does a Babel plugin care about a WordPress site URL?

Edit: Second reaction: Can't we just allow this to be defined as some generic option which is serialized and passed through to the base component via the template call?

Edit: Third reaction: Okay, duh, I understand now; the loading is satisfied by calling load-scripts.php. I still wonder if, as part of this being a generic non-WordPress-specific Babel plugin, we could change this to be satisfied by an implementation provided by the consumer. Maybe a global function which returns a promise when called, that the lazyLoad wrapper will execute (and pass serialized options for specific components, that we could use to pass the script handles?)

[aduth]

Looks like a good candidate for early return to avoid excessive indentation.

[aduth]

TIL about template. Neat 👍

[aduth]

Remove : function ? (Object function shorthand)

[aduth]

What is this predicate doing? (Add as a code comment)

[aduth]

Should it be part of the Babel transform package? If it's specifically meant for handling asynchronous components, including the component loader doesn't seem too farfetched.

[aduth]

This won't handle the case where another instance of the component is mounted while loading is still occurring.

We should just set the scope variable to the promise itself. That way it can repeatedly be .then'd, even if it's already resolved.

[aduth]

Should there be retry if a previous load attempt failed?

[aduth]

Wonder if this is something where we could just assign onto the component prototype itself, rather than having to have this be part of the transform.

i.e.

sprintf( 'wp.components.CodeEditor.siteURL = %s;', json_encode( site_url() ) )

[youknowriad]

I'm not certain about this approach anymore mainly because it works only for Core registered styles and scripts. We should probably see if we can make it work for custom scripts/styles first before pushing this forward.

[aduth]

We should probably see if we can make it work for custom scripts/styles first before pushing this forward.

Seems relevant to my third point in #8017 (comment)

[aduth]

I took another look at this one. I don't know that we should concern ourselves too much with trying to leverage or improve load-scripts.php / load-styles.php. Ultimately while improvements could be made, I think it'd be fine to load these independently via direct URL as a first pass.

Where the challenges I see with this, at least with the code editor, are in determining what it is that the CodeEditor component is depending upon. Is it an actual npm package? Or the assumed presence of a global, as we have today? If it's a package, is it the codemirror package? Or wp.codeEditor? If the latter, how do we define a dependency upon this such that it's suitable to publish CodeEditor in the components package? Or do we need to distribute code-editor.js as a proper package first? Or do we simply document that these globals need to be available?

A few naive thoughts / opinions:

• We could consider refactoring CodeEditor to depend on codemirror package directly, so that we don't need to worry about maintaining the wp.codeEditor intermediary and whether to publish it to npm (and the challenges therein)
• To the more generalized problem:
  1. I'm wondering if we're tackling this at the non-optimal point. Rather than asynchronously loading components, should we be asynchronously loading blocks? And not just specific blocks, but ideally any block, or at least any block that has a lower chance of being used (by presence in content, per-user usage, or manually pre-defined likelihoods)
  2. What we have today in the CodeEditor component as a placeholder component seems like something we could generalize to a higher-order component, similar to what we have in this pull request already, but maybe even less specific to script/style handles, and rather just a function which returns a promise:

wp_add_inline_script(
	'wp-components',
	implode(
		"\n",
		array(
			'wp.components.CodeEditor = wp.compose.withAsyncLoad( function() {',
			'	return Promise.all( [',
			'		' . implode(
				','
				array_merge(
					array_map(
						'gutenberg_get_script_load_js_promise',
						array(
							'wp-codemirror',
							'code-editor',
							'htmlhint',
							'htmlhint-kses',
							'csslint',
							'jshint',
						)
					),
					array_map(
						'gutenberg_get_style_load_js_promise',
						array( 'wp-codemirror', 'code-editor' )
					)
				)
			),
			'	] );',
			'} )( wp.components.CodeEditor );',
		)
	)
);

Note here:

• We override CodeEditor in the context of WordPress. This would only really be effective if the component itself depended on globals†, which is maybe not what we want.
  • † Optionally, those could be globals we transform to inject in the build step, similar to what we do with our Webpack externals configuration
• The server can probably do a more reliable job of determining the appropriate URLs for these script handles than what we could recreate in the client. Also allows filters to be respected (if they exist)

[aduth]

Noting above observations also made with consideration of new need for asynchronous loading of playlist dependencies (#9169, cc @antpb, @gziolo ).

[aduth]

Or do we need to distribute code-editor.js as a proper package first?

@youknowriad pointed me to #8252 😄

[gziolo]

I'm wondering if we're tackling this at the non-optimal point. Rather than asynchronously loading components, should we be asynchronously loading blocks? And not just specific blocks, but ideally any block, or at least any block that has a lower chance of being used (by presence in content, per-user usage, or manually pre-defined likelihoods)

A long time ago I opened #2768 where I was debating with myself that we could take this direction. If we would split block's registration into two parts - server and client - we could easily achieve it marking the component as async. However, it seems like the block itself would have to be an independent entry point so we could enqueue it but defer loading it until a user picks it in the inserter or it is rendered in the post's content.

[youknowriad]

Closing as not needed now, but we might want to revisit at some point.