Framework: Extract the block API to a generic block-api module without global registration

[youknowriad]

In our road to modularize the editor this PR is huge step but I think it can't be broken into smaller PRs. In this PR I create a new module blockscore that contain what we had before in blocks/api except the registration: Parsing, Serializing, Factory, PastHandler...

But all these methods don't rely on the globally registered blocks anymore, they take as an argument a config with the following shape:

const config = {
  blockTypes: [],
  fallbackBlockName: 'core/freeform',
};

Registering the blocks has only one purpose: declare that a block has been registered in WP but to make the blockTypes available in the editor, we need to provide the settings object above to the root element of the Editor (EditorSettingsProvider).

This resolves previous concerns about the global usage expressed here #336 and #1132 (comment)

This PR has a side effect of reducing the role of the blocks module to registering the default blocks essentially. There’s no real reason to keep this module separate from the editor module. As a follow-up I propose to merge them (before extracting a generic block Editor Component maybe)

Caveats:

• I had to reintroduce the editorSettings to the redux store because of their usage in the effects. Extracting a generic block editor will help us remove this (see Framework: Extract the block editor from the editor chrome to make it reusable #2065 as an example)
• The module name is blockscore because our current setup don't work well with dashes in the module name (should be easy to rename when necessary). I'm hoping to rename it just blocks once we merge blocks and editor

Please help review this quickly since this will be hard to keep updated without an enormous amount of conflicts.

Testing instructions
Verify that everything is working as expected :)

[nylen]

"block score"? "blocks core"? Let's call it block-api instead.

[nylen]

There’s no real reason to keep this module [the library of blocks] separate from the editor module.

I'd propose calling this block-library instead. But I agree that is a task for a separate PR :)

[aduth]

Thinking out loud here and continuing conversation from #2119 (comment), as this was on my mind yesterday. I started to question: What is a block without an editor? One cannot exist without the other. But indeed we made a decision early on to split out blocks into its own module because we wanted them to be agnostic to the specific editor, in our case the post editor we're building. But if we're now discussing extracting the core editor bits (#2065), should we not reevaluate whether it makes sense that the block core be independent from the editor core, or should instead seek to converge them?

[youknowriad]

should we not reevaluate whether it makes sense that the block core be independent from the editor core, or should instead seek to converge them?

Not against merging them but at the moment the editor is not the editor-core, it's everything. So this PR extracts blocks-core. Once we extract editor-core we can decide if these two belong together or not.

[youknowriad]

"block score"? "blocks core"? Let's call it block-api instead.

I'm ok with that, blockapi then :)

[aduth]

If we're splitting a blocks "core" from blocks with the intent that what would remain of blocks is to be merged into an editor core anyways, why would we even take the step of creating a separate core instead of just moving out the bits that aren't relevant as "core" from blocks. i.e. it sounds like you're suggesting we'll have block-api, and then eliminate blocks. Why can't we just keep the core as blocks?

I think we're getting a little ahead of ourselves...

[youknowriad]

I'm suggesting we create blocks-api in this PR and rename it later to blocks once we merge what's currently remaining in blocks into editor. I just don't want this PR to be bigger.

[youknowriad]

Anyway, I think I'm missing something obvious but can anyone tell me why the JS tests are failing?

[youknowriad]

Ok so renamed the module blockapi and fixed the JS unit tests. Should be better now

[nylen]

Let's go with block-api with a hyphen separator for readability.

I haven't reviewed this in detail, but at a high level the changes are reasonable, and removing the global state from the block registration API makes sense.

I'm suggesting we create blocks-api in this PR and rename it later to blocks once we merge what's currently remaining in blocks into editor. I just don't want this PR to be bigger.

I am not so convinced about this part.

I think we should merge this PR and continue to discuss the next steps.

[youknowriad]

@nylen Can't you hyphen, right now. I haven't taken a deep look to this, but for some reason, our build fails. Will see if I can fix it.

Edit: I don't know what happened the first time I tried, but block-api worked like a charm. Updated.

[youknowriad]

I think I'll merge this right after today's release.

[codecov]

Codecov Report

Merging #2182 into master will increase coverage by 0.08%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master    #2182      +/-   ##
==========================================
+ Coverage   26.44%   26.53%   +0.08%     
==========================================
  Files         157      158       +1     
  Lines        4851     4892      +41     
  Branches      819      820       +1     
==========================================
+ Hits         1283     1298      +15     
- Misses       3015     3041      +26     
  Partials      553      553

Impacted Files	Coverage Δ
block-api/paste/strip-attributes.js	100% <ø> (ø)
blocks/library/button/index.js	26.66% <ø> (ø)	⬆️
blocks/library/text-columns/index.js	33.33% <ø> (ø)	⬆️
blocks/library/pullquote/index.js	33.33% <ø> (ø)	⬆️
blocks/library/table/index.js	36.36% <ø> (ø)	⬆️
blocks/library/freeform/index.js	100% <ø> (ø)	⬆️
blocks/library/audio/index.js	36.36% <ø> (ø)	⬆️
block-api/validation.js	94.33% <ø> (ø)
block-api/source.js	95.45% <ø> (ø)
block-api/paste/remove-spans.js	77.77% <ø> (ø)
... and 38 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c8c8078...1f23e41. Read the comment docs.

[mtias]

I'd like to give this a look, but won't be able until next week.

[nylen]

I thought of a reason not to do this: prohibiting plugins from registering blocks in the core namespaces.

This is much easier to achieve if our registered blocks live in the same ES6 module as the block API, because then we can use an internal flag that is not exported out of that module. See #2241 - the approach taken there would no longer work after this PR.

I think we can probably achieve the main goal of this PR ("don't rely on the globally registered blocks anymore") without creating the new block-api module.

[youknowriad]

@nylen I don't see why the approach in #2241 won't work with this PR, The global variable and the registration API are still in the same module.

[youknowriad]

@mtias do you mind taking a look at this, I'd like to merge it soon in the week :)

[nylen]

@nylen I don't see why the approach in #2241 won't work with this PR, The global variable and the registration API are still in the same module.

Because this flag is completely internal to the blocks module, it is not accessible by third-party code.

In this PR the flag would be internal to the block-api module. So the blocks module - which registers the blocks for the editor - cannot use it.

In order to achieve "don't rely on the globally registered blocks anymore", why do we need to split the blocks module in two? Why should the block API be separate from the blocks that will use it most often?

[youknowriad]

@nylen Registration is not part of the block-api module so why should we declare this variable in this module?

Why should the block API be separate from the blocks that will use it most often?

Because the block API is used by a generic Block Editor while the registration and the blocks are WP specific. A potential generic editor module, let's call it "editor-core" will be dependent on the "block-api" module but do not depend on the "blocks" module, and the final "blocks" module will depend on both "block-api" and "editor-core", so this PR resolves the inter-dependency issue.

"block-api" and "editor-core" could be merged later (like suggested by @aduth) since they'll be both low-level modules but if we merge "blocks" and "block-api", this means we'll have to merge everything in a unique module.

[nylen]

Registration is not part of the block-api module so why should we declare this variable in this module?

I assumed that registerBlockType would be part of block-api since it's the main function of our block API today.

What role does registerBlockType play after this PR? I expected that the changes here would make it accept and modify a config object rather than a global config.

[youknowriad]

What role does registerBlockType play after this PR? I expected that the changes here would make it accept and modify a config object rather than a global config.

It has the same role as today which is: "declare that a block type in WordPress"
but For an editor to actually use this block type, it has to be included in its config when initializing the editor (see EditorSettingsProvider). For now, the init function just retrieves all the registered blocks to build this config object, but later a PostEditor, a FieldEditor, a CustomizationEditor, a CPT Editor could pick different block types.

[youknowriad]

The rebase today was very difficult to do, I'd really appreciate some help to move this forward.

[nylen]

but later a PostEditor, a FieldEditor, a CustomizationEditor, a CPT Editor could pick different block types

Won't all of these things want basically the same block types, with some minor variation that could be easily captured in the block settings? e.g. { showInContent: false, showInCustomizer: true }

Because the block API is used by a generic Block Editor while the registration and the blocks are WP specific. A potential generic editor module, let's call it "editor-core" will be dependent on the "block-api" module but do not depend on the "blocks" module, and the final "blocks" module will depend on both "block-api" and "editor-core", so this PR resolves the inter-dependency issue.

I'm not convinced that this distinction will actually be so valuable in practice: we're not building a generic block editor, we're building a WP block editor.

[nylen]

I think this should be wp-block-api under the current structure of the PR

[nylen]

I am not sure what this should be but blocksapi isn't used anywhere else in this PR

[youknowriad]

Won't all of these things want basically the same block types, with some minor variation that could be easily captured in the block settings? e.g. { showInContent: false, showInCustomizer: true }

That's exactly how I see it too, but to avoid filtering the blocks in all the places we use the block types, we only filter them once, once initializing the editor.

I'm not convinced that this distinction will actually be so valuable in practice: we're not building a generic block editor, we're building a WP block editor.

Good point, but despite the architectural benefits genericity brings, there are practical benefits too. I'd imagine us needing to load multiple block editors in the same page (say one for the editor, one for a meta-box) or multiple ones in the customizer for different areas. It's not possible right now because all the editor's state is mangled with the Application state (posts, ....). This PR is the first step to bring this genericity.

[BE-Webdesign]

it sounds like you're suggesting we'll have block-api, and then eliminate blocks. Why can't we just keep the core as blocks?

I think this is relevant, and from a quick glance it looks like this is not what is happening.

Overall this looks good, but would need more time to review this.

[youknowriad]

closing this one, it needs more thinking