Open gziolo opened 2 years ago
Hey @gziolo has there been any more progress around this idea/issue? I think this would be a great feature to have and may really benefit us in WooCommerce when laying out the template for the new product editor. We currently make active use of attributes and re-use blocks quite often, I think something like this would offer two big benefits for us:
*This will depend if the above proposed solution will also work with block hooks.
A current example would be:
We have a section block, which renders a unique title/description and renders nested blocks. We have several sections across the product editor page. Like a general details section, a pricing section, an attributes section.
It would currently not be possible to extend after a specific section ( like pricing ) by a 3rd party plugin, but with the above they could, as they could hook into the wc:section/pricing
for example.
I guess this would also allow setting allowedBlocks/template on innerBlocks to allow only some variations inside it? For Example we're using a variation of core-media/text to show a team member with some data - the innerblocks of the core-media/text block are locked. We then have a group block as section in which editors can insert these blocks. At the moment we've locked the group block to allow only core/media-text blocks, but of course the editors could also insert a non-team-member variation of the media-text blocks. So this would be great for such use cases as well - if I understand it correctly?
So this would be great for such use cases as well - if I understand it correctly?
Potentially. It didn't occur to me initially, but now that you said it, it's worth considering as that would be a very interesting use case. It would also play nicely with block variations that define block bindings, so potential Site Title and Post Title could be replicated with the Heading block but maybe people still would want to have a control where to insert it.
I'm still in the process of understanding the complexities in this since this code is unchartered territory for me, but having given this a little bit of thought and there are a few things I think we need to consider, unless I have over complicated it:
registerBlockVariation
function which only registers it client side for the purpose of the new variation being discoverable in the inserter (ie they're not registered as a new block type). If we want to create an alias registered on the server too we'd need to provide a PHP counterpart method or other means (block.json?) of allowing users to register their variation/alias block.Currently each variation can be registered using the registerBlockVariation function which only registers it client side for the purpose of the new variation being discoverable in the inserter (ie they're not registered as a new block type). If we want to create an alias registered on the server too we'd need to provide a PHP counterpart method or other means (block.json?) of allowing users to register their variation/alias block.
Variations can be registered client side with registerBlockVariation
API and also in a block's block.json
file either statically or dynamically(ex here).
Unless I'm misunderstanding how things currently work but I don't think the server understands the concept of variations the same way the client does.
I'm not sure if I misunderstood something here, but I don't think there is a difference in handling between client and server. In both cases we treat all variations of a block as the same single block. The difference is that with variations we have some selected attributes or inner blocks, only when we first insert them. After insertion we can update attributes and can end up not 'having' the original variation we inserted. This could be done either from UI with the block variation transforms or by simple updating the specific attributes in the markup.
I think this proposal is for adding aliases
where we could still have one single block for editing etc.., but also having some more explicit info about the variation that we could use for other purposes. This would include changes in APIs to add this new info and always point to the shared code of the single block. So in places where have actions about blocks, this new API would also support to treat variations as standalone blocks, where in fact they are not.
Some examples that we need that is global styles and theme.json to add specific styles for specific block variations or in block manager to be able to disable/enable specific variations of the block and not holistically do that for the main block(allow only youtube
variation).
It's a complex issue and I think some code explorations will start showing the way forward. I think we can do this iteratively and not update all the UI we would need it eventually. For start we should explore how we can update the registration of blocks and block variations to work with an alias and what an alias would be technically. For example what do we do with the isActive
API? Will it be removed or actually preserved and used to add some meta in the block or dynamically generate some info to help us solve the above use cases.
One of the major questions is where we would use block variation aliases instead of the underlying, „canonical“ block names, and where we would use them in addition. This has been partly answered by @gziolo who mentioned the following goals:
<!-- wp:myplugin/my-cool-block-variation {"someAttr":"someValue"} /-->
instead of <!-- myplugin/some-block {"someAttr":"someValue"} /-->
). This will make it easier for tooling to e.g. find out what are the most frequently used block variations across a WordPress installation.OTOH, we're planning to retain the canonical block name in the editor, which means that there will have to be some logic to convert the alias (as found in the persisted markup in the DB) to the canonical name upon loading in the editor, and back upon saving. I've shared some thoughts on whether we need to include the alias in the markup in the editor (e.g. in block metadata
) in order for the latter part to work (especially here and here); the tl;dr is that I think that we can avoid this, as we can infer the variation from the block's attributes.
In conclusion, we'll store the block alias in the persisted markup in the DB, but will convert it to its canonical name in the editor. However, we still need to answer the question of whether we'll use the alias or the canonical names for all of the in-between steps (e.g. in the parsed block format or in WP_Block
instances on the server), and/or if we'll introduce an additional variation field in any of those. We'll get back to this question later.
A @ntsekouras mentioned, there’s an existing API on the server side to register block variations (variation_callback
, get_block_type_variations
). For simplicity’s sake, I’d like to avoid introducing any new functions or filters to register a variation alias; instead, it should be doable through the existing API.
Furthermore, variations already have a name field to distinguish them from each other. For example, the core/social-link
block has variations named wordpress
, x
, tumblr
, etc.
IMO, there’s a risk of confusion if we introduce aliases (which would be typically prefixed — think myplugin/cool-block-variation
) as a separate concept that the user has to assign themselves. I’m thus leaning towards auto-generating alias names from the block type and the variation by concatenating them, separated by a slash.
(Note that this would require us to change the parser, and would pose another challenge described in more detail here.)
Auto-generating aliases from block type names and variation names has the drawback that 3rd-party plugins cannot use their own namespace for variations they add to existing blocks; e.g. https://github.com/WordPress/gutenberg/issues/43743 had the example of a woocommerce/product-list
alias for the product-list
variation of core/query
. This would have to become core/query/product-list
instead, which isn't ideal, but probably not a dealbreaker. (It'd be nice to include some sort of vendor prefix/namespace in the part after the slash to avoid collisions, i.e. something like core/query/woo-product-list
. This might be a bit trickier to implement, though -- especially for existing blocks.)
On the plus side, we already seem to be creating class names for block variations that closely match the pattern we're considering for the aliases. For example, the wordpress
variant of the Social Link block already gets wp-social-link wp-social-link-wordpress
assigned as its class names. This means that the second requirement mentioned at the top of this comment would be taken care of already.
Finally, note that using "auto-generated" aliases is also partly motivated by YAGNI -- it doesn't preclude allowing manual setting of aliases later. To keep the API simple, I'd add an alias
(or blockAlias
, or even createBlockAlias
) field to the variation schema.
One of the trickiest problems with using block variation aliases is the following dilemma: On the one hand, we want to expose them to extenders so that they can write code that targets them much like they would otherwise target canonical block names -- e.g. for Block Hooks to be able to inject a block variation. On the other hand, if we start exposing the variation name instead of the canonical name in certain places, this is likely going to break existing code.
I'll give an example. For Block Hooks' hooked_block_types
filter to work with variation aliases, we'd basically need the parsed block's blockName
field to contain the alias, rather than the canonical block name; i.e. <!-- core/social-link/wordpress /-->
will result in
array(
'blockName' => 'core/social-link/wordpress'
)
(rather than setting the blockName
field to core/social-link
).
IMO, this is fine: The parsed block format is a fairly low-level representation of the markup, generated by the block parser. (Note that we also likely don't want to introduce a dedicated blockVariation
field, much like there's no dedicated blockNamespace
field. Those are higher-level, semantic concepts that aren't reflected in the block parser.)
However, what about existing code that relied on the canonical block name -- e.g. a hooked_block_types
filter that used core/social-link
as its anchor block? That code would break if we started to persist aliases instead of canonical block names to the DB.
As a direct consequence, with the constraints above, we cannot automatically opt in all blocks that have variations to using auto-generated aliases in the persisted markup; instead, we'll have to make it a per-block user choice, likely through an alias
field that for starters could be a boolean, telling WP whether to generate and use the alias name.
The next layer where we need to decide how to represent aliases is WP_Block
instances. This layer is a lot more semantic that the parsed block format; among other things, it validates the block name based on what blocks are registered. For this reason (and others), I think that the WP_Block
instance's $name
field should reflect the canonical block name, and that we need to introduce additional fields and/or methods to reflect the current variation.
Anyway, that's the current state of my research and thinking. Curious to hear your thoughts, especially from the folks who've worked with variations before @gziolo @ntsekouras. LMK if you agree with my assumptions and conclusions, and if I'm missing something 😄
Thank you @ockham for documenting this so thoroughly! Something I want to pull out for discussion which I've brought up in DM with you is the following statement:
I've shared some thoughts on whether we need to include the alias in the markup in the editor (e.g. in block metadata) in order for the latter part to work (especially https://github.com/WordPress/gutenberg/pull/61481#issuecomment-2107643448 and https://github.com/WordPress/gutenberg/pull/61481#issuecomment-2110137628); the tl;dr is that I think that we can avoid this, as we can infer the variation from the block's attributes.
I'd still be interested to see how this might work, I am currently under the assumption that this means that we can compare the current blocks attributes against all registered variation blocks default attributes to determine whether it's a variation or not (reminder; one of the main motivators for variations existing is allowing a user to create multiple versions of the same block with different attribute values). With that in mind, if the user inserts a variation and changes an attribute it would fall back to the canonical block type and wouldn't get converted upon serialization and persisted to the database.
I'm imaging scenarios where we want to insert the same variation with slightly different attributes, imagine the scenario where we create a variation of WooCommerce's Product Collection block (e.g. woocommerce/product-collection/best-sellers
) and we want to use this variation across different patterns or templates but change the layout
attribute to display products in a grid/list format depending on its context. The purpose of the variation remains the same, display best sellers but only the ones that match the default attribute get saved as the 'correct' variation, the others saved as the canonical block. A consequence of this would be that we could not reliably hook blocks against this alias.
Obviously this may only be an issue if I've understood what you mean correctly, or perhaps there's a way around this 😄
@tjcafferkey So I'd definitely wouldn't want the variation to become unrecognized if the user changes only some circumstantial attribute(s). But I thought we'd be safe from that happening as we'd only evaluate "distinguishing"/"defining" attributes?
There aren't a lot of examples of server-side registered variations in the WP and/or GB codebase, but here's one: The Post Terms block registers a variation for each known taxonomy (so one for Categories, one for Tags, etc?). The defining attribute is the block's term
attribute: https://github.com/WordPress/gutenberg/blob/7674a57870ec3e512c3d0d0edb42fe24ecc53cea/packages/block-library/src/post-terms/index.php#L95-L97
This means that if we're given a Post Term block (no matter if as markup, parsed, or WP_Block
instance), we can tell what variant it is simply by looking at its term
attribute -- and that's not going to be affected by changing any of its other attributes.
So the algorithm I'm envisioning would do something like this:
foreach ( known_variations_for_a_given( $block as $block_variation ) ) {
foreach ( $block_variation->attributes as $attribute => $value ) {
if ( $block->$attribute !== $value ) {
continue 2;
}
}
return $block_variation->name;
}
Or so. Maybe I'll give that a try somewhere, e.g. over at https://github.com/WordPress/wordpress-develop/pull/6594 😬
Or so. Maybe I'll give that a try somewhere, e.g. over at WordPress/wordpress-develop#6594 😬
https://github.com/WordPress/wordpress-develop/pull/6594#issuecomment-2122412575
But I thought we'd be safe from that happening as we'd only evaluate "distinguishing"/"defining" attributes?
Ah, I see now the way in which you're thinking about this and I like the idea! One thing I just want to mention is that you could have a block variation whose attributes remain the same as the canonical block but its inner blocks are different. Just pulling out this sentence from the API documentation for reference.
The Block Variations API allows you to create additional versions of existing blocks that differ by a set of initial attributes or inner blocks.
From my understanding your approach should still work but we'd also need to run a comparison against $variation['innerBlocks']
as well as $variation['attributes']
But I thought we'd be safe from that happening as we'd only evaluate "distinguishing"/"defining" attributes?
Ah, I see now the way in which you're thinking about this and I like the idea! One thing I just want to mention is that you could have a block variation whose attributes remain the same as the canonical block but its inner blocks are different. Just pulling out this sentence from the API documentation for reference.
The Block Variations API allows you to create additional versions of existing blocks that differ by a set of initial attributes or inner blocks.
From my understanding your approach should still work but we'd also need to run a comparison against
$variation['innerBlocks']
as well as$variation['attributes']
Oh, that's a very good point, thank you!
I just had another look at the docs myself, with a focus on the isActive
part. When used on the client side, that's typically a function that helps WP determine which block variation is currently active, based on information such as attributes -- in other words, pretty much what we're doing in infer_block_variation
.
isActive: ( blockAttributes, variationAttributes ) => blockAttributes.providerNameSlug === variationAttributes.providerNameSlug,
Of course we cannot use a JS function like isActive
on the server side, but there's a second format that isActive
accepts, which is an array of (distinguishing) attributes.
isActive: [ 'providerNameSlug' ]
That's a format that we could actually use (and evaluate) on the server side! We might want to give precedence to that isActive
field, and fall back to our current behavior (which is to compare all variation-defining attributes).
That's a format that we could actually use (and evaluate) on the server side! We might want to give precedence to that isActive field, and fall back to our current behavior (which is to compare all variation-defining attributes).
Makes sense. Although it is my understand that callbacks assigned to the isActive
property can become complex which is why @gziolo would like to see this removed (see quote below from the original issue desc). I don't know a tonne about how it's used currently though but it's worth considering how we can still remove that logic and infer a variation if it's possible at all.
This could also remove the need for complex logic used with block variations when detecting the variation based on the result of the isActive callback executed.
Update:
Based on our research and experiments, we're planning to generate block aliases by concatenating block name and variation (i.e. the wordpress
variation of the core/social-link
block gets the core/social-link/wordpress
alias). The alias will then be used in the persisted markup, and at the lowest levels of block processing on the server side; it will however be converted back to the canonical block name at higher levels, and on the client side.
Block authors will need to opt in to have aliases generated for their blocks, for reasons given here.
We have a prototype that seems to be working reasonably well in https://github.com/WordPress/wordpress-develop/pull/6594 (which is based on prior art by @tjcafferkey). We'll work to polish that PR and get it ready for inclusion in 6.6.
Since this will affect the way that block markup is stored and processed, there's a risk that this will meet some opposition, or that it might impact existing code (both Core and 3rd party) in a way that we're not yet aware of. To deal with that risk, we'll break the problem into multiple parts, and will frontload the less risky parts, as they're required for the riskier parts anyway, and have a higher chance to make it into 6.6:
page
variation of the Navigation Link block will receive the wp-block-navigation-link-page
class name in addition to the existing wp-block-navigation-link
.variations.php
and/or variations.json
file.isActive
field. I propose we extend that mechanism to the client side's getActiveBlockVariation
; I also have an idea how to modify it so it might solve this problem. I believe that this might eventually allow us to deprecate isActive
. ~I'll file a separate issue for this.~ Edit: PR here.Hi @ockham — thanks so much for publishing the note on make/core about this proposal and soliciting feedback.
In our client work, we refer fairly regularly to the block markup stored in the database. Speaking for myself, I know that I've tended to assume that a block name, once decided, was stable in the same way that a core shortcode or post type name saved to the database would be stable (not to preclude evolutions to new blocks, like the shift to the core/embed
block, or updated implementations of existing blocks, like the shift to using inner blocks in core/list
).
Accordingly, some of our tooling would be affected by a change to have a block alias be stored instead of the canonical block name — for example, our Match Blocks library (the name
parameter) or our block audit command (the block name and other parameters passed to hooks).
These tools rely on blocks as constructed by the block parser. If existing blocks started appearing in the database under new names, these and other tools would certainly need to be updated in order to continue working accurately. I don't want to call it a "backwards compatibility break," since that's a bit of a loaded term and perhaps not appropriate for this instance, so let's maybe call it a "backwards compatibility disruption" 😄
Please let me also stress that I don't consider myself "in opposition" to this proposal because I'm not really familiar with the underlying problem beyond what was in the post. I share this experience in the spirit of providing information about how this change, as I understand it, would affect developers and some assumptions that might exist in the field about block data.
Thank you for your feedback @dlh01, it's much appreciated!
I've heard similar concerns from some colleagues after I'd posted that proposal, and I definitely want to avoid breaking the internet 😅
To mitigate that risk, block variations aliases would have to be opt-in (i.e. block type names in existing markup won't have their /variation
appended automatically unless the block opts into that -- probably through a flag in block.json
). This should provide some safety, as it's reasonable to expect that the block author would apply any changes necessary to the block's code if they decide to opt into using the variation alias in markup. However, it's true that other tools might not be able to do so. The way I'd try to address this is to delineate quite clearly the level of WP functions where the variation alias would be used (e.g. parse blocks level), and where the "canonical" block type name would still be used (WP_Block
and up). This will however still leave some burden on developers of tools that process blocks based on their types, as they'd need to make sure that their tools use the right function for the right purpose.
It probably needs more fleshing out (including some PoC code), but would the above manage to assuage some of your concerns?
Also, do you have qualms about the specific format (using a second slash to denote the block variation), and how it might impact parsing? Would you be less worried if we allowed arbitrary aliases instead (but kept the format, i.e. didn't allow a second slash) -- once again on an opt-in basis?
BTW one of the motivations for this proposal seems to overlap quite strongly with the functionality of the block audit command you mentioned! In short, we'd like to make it easier to determine not just which block types, but specifically which variations of a block are used how much on a given site. Is that something you'd find useful overall?
To mitigate that risk, block variations aliases would have to be opt-in (i.e. block type names in existing markup won't have their /variation appended automatically unless the block opts into that -- probably through a flag in block.json). This should provide some safety, as it's reasonable to expect that the block author would apply any changes necessary to the block's code if they decide to opt into using the variation alias in markup. However, it's true that other tools might not be able to do so. The way I'd try to address this is to delineate quite clearly the level of WP functions where the variation alias would be used (e.g. parse blocks level), and where the "canonical" block type name would still be used (WP_Block and up). This will however still leave some burden on developers of tools that process blocks based on their types, as they'd need to make sure that their tools use the right function for the right purpose.
It probably needs more fleshing out (including some PoC code), but would the above manage to assuage some of your concerns?
Sort of. I do think that it's the right move for the variation aliases would be opt in. But the key point to me is:
The way I'd try to address this is to delineate quite clearly the level of WP functions where the variation alias would be used (e.g. parse blocks level), and where the "canonical" block type name would still be used (WP_Block and up).
I think what's getting to me is that the delineation wasn't there before. It feels like it makes a "private" API out of something that wasn't private before — or at least it didn't seem private.
That's tricky to handle in any case with software that doesn't use semantic versioning, but I would be particularly sad to see it happen with this specific API of the lower-level block functions. For site migrations, WP-CLI commands, dynamic block insertion, and lots of other cases, I find the developer experience of working with parse_blocks()
and the resulting block shape is way preferable to WP_Block
, and I've valued its stability.
(Aside about WP_Block
: Some operations in WP_Block
require the block type instance, and in my experience, we might not have access to that code during a content migration, so any functionality tied to information normally stored with the block type might not work as expected. Just a use case to think about.)
do you have qualms about the specific format (using a second slash to denote the block variation), and how it might impact parsing? Would you be less worried if we allowed arbitrary aliases instead (but kept the format, i.e. didn't allow a second slash) -- once again on an opt-in basis?
If I'm following you, I think my qualm is not with the second slash in itself, and I don't think I have any opinion about whether aliases should be arbitrary, but just about the alias appearing in the spot in markup where the block name (and only the name) is today. So, for example, any of these imaginary implementations would be unobjectionable to me:
<!-- wp:social-link social-link/wordpress
<!-- wp:social-link {"variation: "wordpress"}
<!-- wp:social-link --variation-wordpress
<!-- wp:social-link wp-variation:wordpress
perhaps yielding
array(
'blockName' => 'core/social-link',
'variation' => 'wordpress',
)
In short, we'd like to make it easier to determine not just which block types, but specifically which variations of a block are used how much on a given site. Is that something you'd find useful overall?
Sure. We quite often do audits to determine which embed providers are in use on a site. In that case, the relevant information for the audit (which services are used) is 1:1 with the variation, so if there was a canonical way to determine the variation, we'd probably find a way to use it.
The way I'd try to address this is to delineate quite clearly the level of WP functions where the variation alias would be used (e.g. parse blocks level), and where the "canonical" block type name would still be used (WP_Block and up).
I think what's getting to me is that the delineation wasn't there before. It feels like it makes a "private" API out of something that wasn't private before — or at least it didn't seem private.
That's tricky to handle in any case with software that doesn't use semantic versioning, but I would be particularly sad to see it happen with this specific API of the lower-level block functions. For site migrations, WP-CLI commands, dynamic block insertion, and lots of other cases, I find the developer experience of working with
parse_blocks()
and the resulting block shape is way preferable toWP_Block
, and I've valued its stability.
Yeah, that's fair enough. I can't deny that that delineation would've been a bit arbitrary, and you're making a compelling case, so let's scrape that idea.
(Aside about
WP_Block
: Some operations inWP_Block
require the block type instance, and in my experience, we might not have access to that code during a content migration, so any functionality tied to information normally stored with the block type might not work as expected. Just a use case to think about.)
Good point, thank you for raising that 👍
If I'm following you, I think my qualm is not with the second slash in itself, and I don't think I have any opinion about whether aliases should be arbitrary, but just about the alias appearing in the spot in markup where the block name (and only the name) is today. So, for example, any of these imaginary implementations would be unobjectionable to me:
<!-- wp:social-link social-link/wordpress <!-- wp:social-link {"variation: "wordpress"} <!-- wp:social-link --variation-wordpress <!-- wp:social-link wp-variation:wordpress
perhaps yielding
array( 'blockName' => 'core/social-link', 'variation' => 'wordpress', )
Gotcha, thank you for clarifying. In particular, I agree that if we want to avoid introducing an arbitrary delineation of variation-unaware and variation-aware data structures and code, then our best course of action is adding a dedicated variation
field to the parsed block format, as you've suggested 🙂
In short, we'd like to make it easier to determine not just which block types, but specifically which variations of a block are used how much on a given site. Is that something you'd find useful overall?
Sure. We quite often do audits to determine which embed providers are in use on a site. In that case, the relevant information for the audit (which services are used) is 1:1 with the variation, so if there was a canonical way to determine the variation, we'd probably find a way to use it.
👍
Based on the feedback we've gotten on this proposal, we're not going to implement this change. (Instead, we'll only make some changes to add variation-specific class names to blocks.) I'll comment on the make/core proposal shortly. Thanks again for sharing your concerns!
What problem does this address?
Part of Block API roadmap #41236.
Let's explore whether some block variations can be read as a block type. For example, instead of
core/template-part/header
be able to just havecore/header
to still get resolved by the template part functions. We used to have a similar granular naming schema for Embed and Social Link blocks before we converted them to block variations:https://github.com/WordPress/gutenberg/blob/bdb6a41099529541ce1f35d05037fef066674150/packages/blocks/src/api/parser/convert-legacy-block.js#L23-L29
https://github.com/WordPress/gutenberg/blob/bdb6a41099529541ce1f35d05037fef066674150/packages/blocks/src/api/parser/convert-legacy-block.js#L31-L50
Benefits of using aliases
theme.json
granularly (like attach custom CSS that only is used when the block is used, etc.).multiple
block support).What is your proposed solution?
Add handling for block type name aliases used with block variations. They should fall back if necessary to the original block type name, e.g.
woocommerce/product-list
that can get resolved to theproduct-list
variation added to thecore/query
block.Some examples to consider:
becomes
This could also remove the need for complex logic used with block variations when detecting the variation based on the result of the
isActive
callback executed.