Blocks: move bootstrapped block types to Redux state#53807
Conversation
jsnajdr
added
[Type] Enhancement
|
Size Change: +162 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in e6dae48. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6024898479
|
tyxla
left a comment
tyxla
left a comment
There was a problem hiding this comment.
@jsnajdr haven't done a thorough review just yet, but in the meantime, I noticed that the failing e2e tests have a consistent kind of "The block "undefined" must have a title." error and it persists after retrying. Seems to be something related to the changes suggested here.
jsnajdr
force-pushed
the
add/boostrapped-blocks-reducer
branch
from
2d9c9f9 to
bee7370
Compare
That was a bug in the |
See #34299 for reference. The experimental action had indirect e2e test coverage added, but it would be great to add more tests to increase confidence in this logic. I think the issue is that the I believe that the modified action gets raw settings now, while in the past it would standardize it with default values first. |
|
Thank you for opening this PR. It's definitely the area of the codebase that should be improved after we confirmed over time that #34299 resolved the issue with the order of applying filters for the block registration. I need to spend more time processing the changes proposed. I'd like to share some higher-level thoughts for now after I left a previous comment regarding the failing unit test.
An important note is that |
Yes, I added a unit tests that tests the "reapply block filters" logic. Before, it was tested only through e2e tests. The bug was a simple mistake, I was reading the
Yes, I decided to rearrangle the logic in such a way that |
|
By the way, from my perspective, you can promote |
This would mean that an inline script that does: wp.blocks.unstable__bootstrapServerSideBlockDefinitions({
'core/paragraph': { title: 'Paragraph' }
});would be replaced with: wp.apiFetch.use(wp.apiFetch.createPreloadingMiddleware({
'/wp/v2/block-types': {
'body': [ { name: 'core/paragraph', title: 'Paragraph' } ]
}
});and the access to the boostrapped block types array would be a select( coreStore ).getEntityRecord( 'root', 'blockTypes' );Is that right? That would work, sure, there's just one caveat:
The preloaded REST data are created in the context of the post editor page, just like the |
|
@jsnajdr, you did a great job explaining how that could work. The REST API call would definitely need to be preloaded and further optimized so only essential fields get returned, mirroring what happens today around
Excellent observation. I didn't consider it before. In that case, it should be pretty straightforward refactoring that we can tackle after this PR lands. |
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Excellent refactoring, @jsnajdr. I did a quite in-depth review and it's looking very close to ready. I noticed one or two potential minor issues, and some room for improvement in terms of how this code was documented in the past (that's on me) to better explain all the ways the block can get registered to satisfy different contexts: client only (covers unit tests and React Native, too) but also client + server.
| [ name ]: getBlockSettingsFromMetadata( blockNameOrMetadata ), | ||
| } ); | ||
| } | ||
| const metadata = isObject( blockNameOrMetadata ) |
There was a problem hiding this comment.
Previously, it wouldn't run metadata processing when the first param passed to the function is the string representing the block's name. I believe it needs to remain as it as was because the settings object can provide a different set of keys, and some of them can be already translated like title or description. In effect, only when the blockNameOrMetadata is an object it should call addBootstrappedBlock. Historically, it was always possible to register a block without block.json by providing it's name and settings object that could be used as is.
There was a problem hiding this comment.
I made this change because it's very useful for async block loading. At some places, like block inserter, my prototype looks at the server bootstrapped data, to avoid fully loading the block:
const { title, parent, supports } = select( blocksStore ).getBootstrappedBlockType( 'core/query-pagination');But this doesn't work for blocks that are registered only client-side, with no block.json metadata:
registerBlockType( 'core/query-pagination', {
title: __( 'Pagination' ),
parent: [ 'core/query' ],
supports: { inserter: false },
} );There is no metadata, nothing is added to state.bootstrappedBlockTypes for this block. That's why I'm "reconstructing" the metadata by extracting selected fields from settings.
the settings object can provide a different set of keys, and some of them can be already translated like
titleordescription.
There are two scenarios that can happen here:
- the
titleanddescriptionfields were bootstrapped from the server. Then they are localized (the server did that) and the call toaddBootstrappedBlockinregisterBlockTypewon't overwrite them. It will do at best the polyfilling of.selectors. - the fields were not bootstrapped from the server, and
addBoostrappedBlockwill add the data extracted fromsettingstostate.bootstrappedBlockType. Thetitleshould be localized, because the JS code will calltitle: __( 'Pagination' )if the block is localized at all.
So, in neither of these cases are correct localized data overwritten with incorrent unlocalized data. We're just adding "fallback" bootstrapped metadata in case they don't exist at all.
There was a problem hiding this comment.
If it's not using block.json, then it should never have textdomain in the object, so it shouldn't run custom code that applies localization helpers. We are good in that regard.
It still isn't clear to me why it's necessary to fake the block-type bootstrapping in the case where it is registered on the client with:
registerBlockType( 'core/query-pagination', { title: __( 'Pagination' ), parent: [ 'core/query' ], supports: { inserter: false }, } );There is no metadata, nothing is added to state.bootstrappedBlockTypes for this block. That's why I'm "reconstructing" the metadata by extracting selected fields from settings.
In this case, it should be just fine to bootstrap this block type with an empty object to signal it was also registered. Then, you could do the fallback the other way around and read values directly from the processed version of the block type. You essentially need to fully register the block type anyway to be able to expose it in the inserter.
The only controversial issue is whether to extract "fallback" metadata from a registerBlockType( 'name', { ...settings } ) call. It's useful for the async block loading prototype, but it's also possible to remove this change from this PR and merge it separately, carefully tested.
I guess I don't fully understand the requirements for async loading, so I would say we either pass an empty object for now, or we can skip it altogether so we can discuss it separately and unblock this PR. Otherwise, I think everything is clear and I'm happy to move forward with the refactoring.
There was a problem hiding this comment.
I reverted to the old boostrapping behavior in e6dae48. Extracting the metadata from settings can be done later, and can be discussed with better context about async block loading.
Having done this, I believe all feedback is addressed 👍
|
|
||
| const settings = applyFilters( | ||
| 'blocks.registerBlockType', | ||
| blockType, |
There was a problem hiding this comment.
Filters might modify the blockType object directly, so a copy { ...blockType } was passed in the past. If we aren't concerned with the modifications, then maybe it would be better to use settings instead of the blockType after this line.
There was a problem hiding this comment.
We don't need to make a copy because blockType is a new object (created from default, bootstrapped and unprocessed settings) on each call.
Before this PR, processBlockType was working directly on an object in Redux state, so a copy was needed.
Still, in both the old and new versions, there is a risk that a filter will do something like:
blockType.supports.inserter = false;mutating the unprocessed settings object in Redux. We'd need to make a deep copy to avoid that.
There was a problem hiding this comment.
That is a good point about deep copy. Thinking a bit more about all of these, I think the challenge here is that block deprecations have very specific needs. You essentially want to pass the orginal blockType without any modifications/mutations applied with blocks.registerBlockType filter to avoid the situation where the deprecation has its opinions on how to handle the same filter for every deprecation. Although the old implementation is still not resilient to mutation nested objects ...
Well, it doesn't fail any existing unit tests so that might be not an issue at all 🤷🏻
I'm however still skeptical about whether this REST approach is the right way to go. In addition to the fragility caused by REST being async, it also adds a new |
jsnajdr
force-pushed
the
add/boostrapped-blocks-reducer
branch
2 times, most recently
from
9e7c9b6 to
3949aa1
Compare
jsnajdr
requested review from
adamziel,
ajitbohra,
draganescu,
juanmaguitar,
kevin940726,
noisysocks,
ryanwelcher,
talldan and
tellthemachines
as code owners
|
Thanks @gziolo for a detailed review. I addressed all your feedback. The only controversial issue is whether to extract "fallback" metadata from a |
We definitely don't want to make the Anyway, it's a separate issue, and we can discuss it later. The main reason I think it would be valuable is that today, we can only consume core blocks in the mobile app, so that would help to bridge the gap as we would use a single approach to bootstrap blocks registered on the server.
See my comment at #53807 (comment). It would be useful to mention two changes in the CHANGELOG file:
|
jsnajdr
force-pushed
the
add/boostrapped-blocks-reducer
branch
from
11322b9 to
e6dae48
Compare
|
|
||
| // The `autoInsert` prop is not yet included in the server provided | ||
| // definitions and needs to be polyfilled. This can be removed when the | ||
| // minimum supported WordPress is >= 6.4. |
There was a problem hiding this comment.
Added this comment, and it implements the suggestion I shared with @ockham in https://github.com/WordPress/gutenberg/pull/52969/files#r1310187507.
gziolo
left a comment
gziolo
left a comment
There was a problem hiding this comment.
Let's go ahead and move forward now that all feedback has been taken care of. Thank you so much for walking me through all the changes proposed. I'm delighted to see all the improvements applied.
bph
added
[Type] Code Quality
4 participants
This is a spinoff from the async block loading prototype (#51778), and it moves the
serverSideBlockDefinitionsto Redux state of thecore/blocksstore, and adds some private actions and selectors to that store to work with the bootstrapped block types.The
serverSideBlockDefinitionsobject is now instate.bootstrappedBlockTypes, and almost all code fromunstable__bootstrapServerSideBlockDefinitionshas been moved to theboostrappedBlockTypesreducer. It's the code that adds some missing properties and converts property names to camel case. There is a newaddBootstrappedBlockprivate action to add boostrapped blocks, andunstable__bootstrapServerSideBlockDefinitionscalls this action in a loop.The
__experimentalRegisterBlockTypepublic action has been renamed to a private actionaddUnprocessedBlock. It's very unlikely that it's used by any plugin. A wpdirectory.net search shows two matches, but on closer inspection both of them just bundle the@wordpress/blockspackage.Similarly, the
__experimentalGetUnprocessedBlockTypespublic selector has been renamed to privategetUnprocessedBlockTypes.Now block registration works like this:
addBootstrappedBlockand stored instate.bootstrappedBlockTypes.wp.blocks.registerBlockTypesare stored instate.unprocessedBlockTypes.processBlockTypefunction (now extracted to a separate module) that takes these two pieces of information ("bootstrapped" metadata and "unprocessed" settings), merges them together, fills default values, and runs the result throughregisterBlockTypefilters. The filtered processed result is stored instate.blockTypes. That's where all the user code, likewp.blocks.getBlockTypes(), gets the block types from.__experimentalReapplyBlockTypeFiltersaction, called after editor initialization, that reruns theprocessBlockTypejob and re-createsstate.blockTypesonce again from the raw data. That accounts for filters that are registered late.A good followup for this PR would be a separate PR that renames and stabilizes the
unstable__bootstrapServerSideBlockDefinitionsand__experimentalReapplyBlockTypeFiltersAPIs. Both of them are by now established parts of any Gutenberg editor initialization.