Blocks: Defer registration of all core blocks until editor loads

[gziolo]

Description

My first attempt to resolve one of the issues raised in #3800 by @andreiglingeanu:

let settings = wp.blocks.getBlockType('core/button')
wp.blocks.unregisterBlockType('core/button')
wp.blocks.registerBlockType('core/button', {
	...oldSettings,
	attributes: {
		...settings.attributes,
		heya: {
			type: 'string',
			default: '#cf2e2e',
		},
	},
})

The reason for that hack was this: wp-blocks script does two things:

1. Registers the hooks (in wp.hooks.*) to be received
2. Calls each wp.blocks.registerBlockType method of core's blocks immediately, in a sync way

This in itself is not a problem. The problem arise when third-party plugins try to hook into registerBlockType. They literally have no time to do their transformations because action 1) and 2) happen one after another, with no way to interfere between them.

This PR refactors the library of blocks to expose the function registerCoreBlocks that registers all core block rather than doing it as a side effect at the time of declaration. This simple change allows us to have a much better control over when we want to register blocks. My initial proposal is to register all core blocks at the same time when Store instance for the editor is created. I'm open for discussion here and happy to hear how we can tackle it differently.

I like this refactoring because it revealed that some code assumes that blocks are globally available. I had to modify a few failing tests where importing from @wordpress/blocks magically bootstraps core blocks.

Sidenote: It's much easier to review this code using diff without unnecessary whitespace changes:

• https://github.com/WordPress/gutenberg/pull/4514/files?w=1

How Has This Been Tested?

Manually and npm run test-unit. There should be no visual changes introduced.

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows has proper inline documentation.

[mtias]

Is this all blocks or just the core library of blocks?

[gziolo]

Core blocks. I guess registerCoreBlocks() is the way to go then. I had it typed once, but reverted to all keyword :)

[gziolo]

Updated.

[mtias]

This seems very verbose.

[gziolo]

export default () => registerBlockType( 'core/audio', {

is another way to go.

[mtias]

Either that or

export const Audio = () => registerBlockType( 'core/audio', {} );

[youknowriad]

What about exporting the name and the settings and loop over the blocks and call registerBlockType when needed? I think we explored this possibility at some point can't recall why we dismissed it

[gziolo]

@youknowriad can you give an example? How would it work with embeds file which contains dozen definitions?

[youknowriad]

import * as audio from 'audio';
import embeds from 'embed';

const blockConfigs = [
 audio,
 ...embeds
];

blockConfigs.forEach( ( { name, settings } ) => registerBlockType( name, settings ) );

The embeds woud export an array [{ name, settings }], while the others would export { name, settings }. I don't know if this is a good option though.

I guess the main advantage of this approach is the possibility to test the settings without registering the blocks.

[gziolo]

Interesting, @mtias & @aduth what do you think? I'm fine with that, too.

[gziolo]

2c00ffc - I like name & settings pair. It gives us more flexibility when testing code.

[aduth]

Makes me wonder if name and settings need to be separate at all, e.g.

export default {
	name: 'core/foo',

	// ...settings
};

We could overload registerBlockType to support both this and the current implementation.

Thinking back to some of the original reasons for the API as it stands:

• Compatibility with similar APIs like register_post_type and register_rest_route†
• More easy to identify name at a glance (also enforcing as first, which is not true as an object property)

To such API design generally, the string, object argument signature seems best-suited in cases where the object contains properties none of which are necessarily required. An ordered set of arguments is good when all arguments are required and the set is limited in size (since otherwise the ordering loses its obviousness on what each value represents).

Here, it's a bit of an awkward mismatch. The name is required, but so are some of the options (e.g. save). Conversely, not all block properties are required (e.g. supports). To me, other APIs notwithstanding, the single object argument seems the most sensible.

</braindump>

† But this could serve as a counterpoint, since unlike in register_rest_route, we don't separate the namespace and block name arguments.

[gziolo]

@youknowriad I'm refactoring towards your proposal. I will continue tomorrow. I think it looks nice. The diff will remain big because of the embeds file.

[gziolo]

This is ready for another review. I think it's solid now.

[youknowriad]

Maybe we should move this inside the "initialize" function since we do not support multiple instances anyway.

[gziolo]

Happy to move it there. I wasn't sure what is the best place to put it :)

In the long run, we could mirror what WP core uses and introduce some global hooks like init to take advantage of the priority param and be sure that registerCoreBlocks is always called last after all other hooks coming from the plugins are registered.

[youknowriad]

I like this personally, I think it clarifies the config of the editor's block by centralizing them.
I'm not certain this solves all the "deferring issues" but it makes it clear when this initialization is happening (where this function is being called).

I'd also be open to refactor registerBlockType to move the name as part of the settings object like suggested by @aduth . But this could be a separate PR.

[gziolo]

Yes, I also like what @aduth proposed, but this might be too much for this PR. It's already big enough and we would have to update all docs to reflect this relatively small change. We would also have to promote it over the current API which can be found in a few example repositories, blog posts, online courses, etc :)

[mcsf]

This looks good to me. Seems like a nice starting point for a number of subsequent improvements, as already mentioned: combining blocktype name and settings, tweaking consumable hooks, etc.

[andreiglingeanu]

@mcsf righto, I see a lot of possibilities for improvements & customization that can happen in user land if Gutenberg gets all of this stuff right.

Also, not requiring plugin JavaScript to be loaded at a certain time in editor lifecycle is a big win. Plugins will be able to do conditional code splitting, lazy loading of their customizations & other awesome stuff.

[gziolo]

I think it's in good shape to merge it as it is. Let's iterate further on this in the follow-up PRs. Thanks for all reviews everyone 🙇

[ellatrix]

This broke shortcode paste (e.g. caption). The order of block registration is important for that. See #3610 (comment). Cc @mcsf @aduth @jorgefilipecosta.

[gziolo]

It's easy to reorder this manually, but is there any way we could make the registration of blocks independent from the paste functionality?

[ellatrix]

IMHO we should remove the block. It seems odd to just match any shortcode. Falling back to text seems fine if there is no handling in my opinion... It also breaks inline shortcodes.

[mcsf]

Falling back to text seems fine if there is no handling in my opinion...

The problem with falling back to text is that we break parameter handling in shortcodes. Per #3610:

The main benefit of this is that, within the Shortcode block, the code will be properly handled and never mishandled. Notably, this fixes the case where a shortcode sitting in a Paragraph block will be picked up with its double quotes rendered as ", thus breaking parsing of a shortcode's attributes.

Inline shortcodes are also affected by this, and obviously requiring usage of the Shortcode block is not a long-term solution; #4049, on the other hand, is.

I'm fine with simply reverting #3610, but it'd be interesting to look at priority-based registration of blocks.

[aduth]

Correct me if I'm wrong, but could we solve the original issue simply by extracting core block registration separate from the block API bundle? As part of #2751, I'd like to create individual bundles for each core block (related: #2756, #4500 (comment)), meaning we could revert to the original behavior while still addressing the challenge noted in #3800. This would also avoid those individual bundles needing to expose anything to the global namespace (and having a registerCoreBlocks function altogether), rather registering themselves directly upon being loaded.

As noted in #4097, there's an open question as to whether the relationship between registration and the editor is:

• ...register all blocks, but define as an option to the editor the subset of "active" blocks available
• ...only register blocks intended to be used in the editor

The latter is "easier" in that we can always call getBlockTypes and assume it's the canonical set of blocks available, but something feels off about relying on the fact that a block type is inactive merely incidental to the fact that it's not registered (would we ever care to know that the block exists, even if not active?).

[gziolo]

Correct me if I'm wrong, but could we solve the original issue simply by extracting core block registration separate from the block API bundle?

This would be still a bit complicated to achieve because you would always have to explicitly enforce loading order of the modules. In this particular case, it would have to load hooks and blocks modules first to make API methods available. Then you would have to load the plugin code to register all hooks that would modify core blocks. Finally, those core blocks would need to get registered before the editor code is initialized.

Another use case is for plugin blocks and their hookable parts. How do we want to enable all the modifications for the block registered by the plugin. Should the moment of registration of all blocks happen when the editor is ready to launch to make everything easier? Should we convert registerBlockType itself to a Promise that is resolved inside the editor? There are a few ways we can hide the complexity of the order how all modules are loaded.

As noted in #4097, there's an open question as to whether the relationship between registration and the editor is:

• ...register all blocks, but define as an option to the editor the subset of "active" blocks available

• ...only register blocks intended to be used in the editor

This is the most important question we need to answer first :) It's hard to predict without having a real-life use cases. We can only speculate at the moment, so maybe we should start simple and register only those blocks that are whitelisted? This will help us to discover if for instance reusable blocks will work as expected.

[aduth]

This would be still a bit complicated to achieve because you would always have to explicitly enforce loading order of the modules. In this particular case, it would have to load hooks and blocks modules first to make API methods available. Then you would have to load the plugin code to register all hooks that would modify core blocks. Finally, those core blocks would need to get registered before the editor code is initialized.

Isn't this not as complex as it sounds? Merely a combination of wp_register_script $deps and priority on wp_enqueue_scripts. The first defines dependencies to ensure hooks and blocks are loaded first, and in the second an extending plugin defines a lower priority number if they need to be loaded before core blocks are registered.

[gziolo]

Isn't this not as complex as it sounds? Merely a combination of wp_register_script $deps and priority on wp_enqueue_scripts. The first defines dependencies to ensure hooks and blocks are loaded first, and in the second an extending plugin defines a lower priority number if they need to be loaded before core blocks are registered.

So wp_register_script and priorities. I see what you want to do here. Yes, it makes sense and can be implemented very easy with the simple assumption that by default all blocks namespaced with core/ get lower priority than plugins that want to modify them. I will review #4841 tomorrow and leave my comments there.