Framework: Extract the block API to a generic block-api module without global registration#2182
Framework: Extract the block API to a generic block-api module without global registration#2182youknowriad wants to merge 1 commit intomasterfrom
Conversation
|
"block score"? "blocks core"? Let's call it |
I'd propose calling this |
|
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 |
a7a5843 to
0b6c0fd
Compare
Not against merging them but at the moment the |
I'm ok with that, |
|
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 I think we're getting a little ahead of ourselves... |
|
I'm suggesting we create |
|
Anyway, I think I'm missing something obvious but can anyone tell me why the JS tests are failing? |
0b6c0fd to
95fbdf8
Compare
|
Ok so renamed the module |
95fbdf8 to
5e5d246
Compare
5e5d246 to
f3cfa33
Compare
|
Let's go with 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 am not so convinced about this part. I think we should merge this PR and continue to discuss the next steps. |
|
@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 |
282b086 to
38131e3
Compare
|
I think I'll merge this right after today's release. |
11d6c69 to
836df0e
Compare
Codecov Report
@@ 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
Continue to review full report at Codecov.
|
|
I'd like to give this a look, but won't be able until next week. |
|
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 |
|
@mtias do you mind taking a look at this, I'd like to merge it soon in the week :) |
In this PR the flag would be internal to the In order to achieve "don't rely on the globally registered blocks anymore", why do we need to split the |
|
@nylen Registration is not part of the
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. |
I assumed that What role does |
It has the same role as today which is: "declare that a block type in WordPress" |
836df0e to
79e7c6e
Compare
|
The rebase today was very difficult to do, I'd really appreciate some help to move this forward. |
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.
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. |
lib/client-assets.php
Outdated
There was a problem hiding this comment.
I think this should be wp-block-api under the current structure of the PR
test/setup-wp-aliases.js
Outdated
There was a problem hiding this comment.
I am not sure what this should be but blocksapi isn't used anywhere else in this PR
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.
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. |
879c298 to
4ba533e
Compare
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. |
4ba533e to
c5f68e8
Compare
c5f68e8 to
40ce176
Compare
…out global registration
40ce176 to
1f23e41
Compare
|
closing this one, it needs more thinking |
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
blockscorethat contain what we had before inblocks/apiexcept 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
configwith the following shape: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
blocksmodule to registering the default blocks essentially. There’s no real reason to keep this module separate from theeditormodule. As a follow-up I propose to merge them (before extracting a generic block Editor Component maybe)Caveats:
editorSettingsto 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)blockscorebecause 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 justblocksonce we mergeblocksandeditorPlease 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 :)