Blocks: Merge variations bootstrapped from a server with the client definitions #60832
Conversation
Mamaduka
added
[Type] Bug
|
Size Change: -1.58 kB (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
| variations: [ | ||
| ...( bootstrappedBlockType?.variations || [] ), | ||
| ...( blockSettings?.variations || [] ), | ||
| ], |
There was a problem hiding this comment.
I initially planned to use deepmerge here but decided to handle only this singular case for now.
There was a problem hiding this comment.
I think you should do some more sophisticated merging from the beginning. It doesn't need to be deep merge, but it should handle the following typical situation.
The server usually knows some subset of the block registration info. It can know as little as just the name and render_callback. The bootstrapped block info from server can look something like this:
{
name: 'core/group',
title: 'Group',
variations: [
{
name: 'group-row',
title: 'Row'
},
{
name: 'group-stack',
title: 'Stack'
}
]
}Then when the client registers the block in JS, it has the complete information. Including some fields that have JS functions as values, and can't be even expressed in PHP and JSON. The client-side block registration then does a shallow merge of the server and client values:
...bootstrappedBlockType, // from server PHP
...blockSettings, // from client JSThere's no merging of fields, just overwriting server values with client ones.
The individual variations have the same field structure as the top-level block. Therefore, to combine server and client information, you should match the array elements by name, and merge them. Like:
variations = bootstrappedVariations.map( b => {
return {
...b,
...clientVariations.find( c => c.name === b.name ),
}
} );The real code should also add the items that are not in bootstrappedVariations but are only in clientVariations, but you get the idea.
Note that there is no deep merging, just matching the array elements and shallow-merging them together.
There was a problem hiding this comment.
Makes sense. Thanks, @jsnajdr!
Also, just to clarify, I was considering deep merging of whole setting objects.
...deepmerge( bootstrappedBlockType, blockSettings )There was a problem hiding this comment.
I think deepmerge is overkill, and base block registration doesn't do that either. You might even end up with some unexpected buggy results. It might not always be a good idea to merge {a} and {b} into {a,b} or {a:[1]} and {a:[2]} into {a:[1,2]}.
Deep merging should be very specifically targeted at specific properties. For example, with variations, we'll be doing one-level deep merge of two arrays keyed by name.
There was a problem hiding this comment.
I think deepmerge is overkill
I thought so too, and dropped the idea 😄
We can add similar handlers as needed and eventually extract them into a custom merger method.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Mamaduka
force-pushed
the
fix/block-registration-merge-settings
branch
from
de8786a to
be32c0b
Compare
| clientVariations.forEach( ( clientVariation ) => { | ||
| const index = result.findIndex( | ||
| ( bootstrappedVariation ) => | ||
| bootstrappedVariation.name === clientVariation.name |
There was a problem hiding this comment.
Any concerns with the fact that variations are not required to have a name? Could this possibly run into the issue of wrongly assuming two completely different variations without a name are the same one?
There was a problem hiding this comment.
Odd. I always thought that a unique name was required; this seems like an oversight on our end.
There was a problem hiding this comment.
Maybe it's a very rare or legacy scenario, but still worth considering. I was also surprised when I learned about it when reading @ndiego's post linked above.
Maybe we should treat such variations as unique and never attempt to merge them?
There was a problem hiding this comment.
I believe the article is wrong about this (fyi @ndiego). A variation name is crucial for Gutenberg to be able to use it. If I look at how variations list for block inserter is constructed in getInserterItems and getItemFromVariation, how an unique id is generated from block and variation name in getItemFromVariation, how the name is used as a React list key in many UI components, I don't think a variation without a name would work.
| ); | ||
|
|
||
| if ( index !== -1 ) { | ||
| result[ index ] = { ...result[ index ], ...clientVariation }; |
There was a problem hiding this comment.
Do we intentionally want to keep the bootstrapped variation data and mingle it with the client side one? What if on the client we intentionally want to keep a variation property empty, but the server declares it?
There was a problem hiding this comment.
Can you elaborate a little more here?
The merge is intentional. Otherwise, data defined on the client side will override variations provided by the server, which causes the bug described in the original issue #60298.
I think processing logic should respect any definition of block variations. The server-side definition becomes much easier with WP 6.5, and I think we'll see more issue reports until this is resolved.
There was a problem hiding this comment.
What if on the client we intentionally want to keep a variation property empty, but the server declares it?
You mean that the server would set a variation property and then client doesn't want it to exist at all, wants to unset it? This should never happen. First, when registering blocks, the server has incomplete information and then the client fills in the missing pieces or providing more detail. The server information should not be ever wrong, the client's job is to improve it, not overwrite.
Second, the block registration already uses exactly the same logic when building the blockType object:
...select.getBootstrappedBlockType( name ),
...blockSettings,In this PR we're merely extending this logic to variations.
There was a problem hiding this comment.
The server information should not be ever wrong, the client's job is to improve it, not overwrite.
This is what I wanted to confirm. If that's the intention, then we're good here.
| variations: mergeBlockVariations( | ||
| bootstrappedBlockType?.variations, | ||
| blockSettings?.variations | ||
| ), |
There was a problem hiding this comment.
Here, if neither source has a variation field, we shouldn't set it on the resulting blockType at all.
There was a problem hiding this comment.
That case would result in empty array, just like before.
There was a problem hiding this comment.
I see, I didn't notice there is always an empty array by default.
| clientVariations.forEach( ( clientVariation ) => { | ||
| const index = result.findIndex( | ||
| ( bootstrappedVariation ) => | ||
| bootstrappedVariation.name === clientVariation.name |
There was a problem hiding this comment.
I believe the article is wrong about this (fyi @ndiego). A variation name is crucial for Gutenberg to be able to use it. If I look at how variations list for block inserter is constructed in getInserterItems and getItemFromVariation, how an unique id is generated from block and variation name in getItemFromVariation, how the name is used as a React list key in many UI components, I don't think a variation without a name would work.
| variations: mergeBlockVariations( | ||
| bootstrappedBlockType?.variations, | ||
| blockSettings?.variations | ||
| ), |
There was a problem hiding this comment.
I see, I didn't notice there is always an empty array by default.
|
Thanks for the reviews and feedback! 🙇 I'll try to follow up on variations requiring the |
|
Here is the type definition for block variations: Name and title are mandatory. It's also reflected in block.json schema: |
What?
Resolves #60298.
PR updates the block type processing logic (
processBlockType) to merge thevariationssettings provided by the server and client.Why?
Some block settings like
variationsshould be merged.Testing Instructions
Testing variation
Testing Instructions for Keyboard
Same.
Screenshots or screencast