Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Block API: Add support for block aliases (PoC) #6523

Draft
wants to merge 8 commits into
base: trunk
Choose a base branch
from

Conversation

tjcafferkey
Copy link

@tjcafferkey tjcafferkey commented May 8, 2024

Gutenberg counterpart: WordPress/gutenberg#61481

This PR is a little more exploratory/hacky, I'm not sure quite yet how the end result will look but my thinking is that when we come across aliased blocks on the server I want them to also be treated like WP_Block_Type's and be aware of its canonical block, but also for the canonical block to know about all of its aliases.

Trac ticket:
Related to: WordPress/gutenberg#43743


This Pull Request is for code review only. Please keep all other discussion in the Trac ticket. Do not merge this Pull Request. See GitHub Pull Requests for Code Review in the Core Handbook for more details.

@tjcafferkey tjcafferkey changed the title Block API: Add support for block aliases. Block API: Add support for block aliases (PoC) May 8, 2024
Copy link

github-actions bot commented May 8, 2024

Test using WordPress Playground

The changes in this pull request can previewed and tested using a WordPress Playground instance.

WordPress Playground is an experimental project that creates a full WordPress instance entirely within the browser.

Some things to be aware of

  • The Plugin and Theme Directories cannot be accessed within Playground.
  • All changes will be lost when closing a tab with a Playground instance.
  • All changes will be lost when refreshing the page.
  • A fresh instance is created each time the link below is clicked.
  • Every time this pull request is updated, a new ZIP file containing all changes is created. If changes are not reflected in the Playground instance,
    it's possible that the most recent build failed, or has not completed. Check the list of workflow runs to be sure.

For more details about these limitations and more, check out the Limitations page in the WordPress Playground documentation.

Test this pull request with WordPress Playground.

* @param array $aliases Array of registered aliases for a block type.
* @param WP_Block_Type $block_type The full block type object.
*/
return apply_filters( 'get_block_type_aliases', $this->aliases, $this );
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know this already matches the existing pattern of using a filter similar to get_block_type_variations, but it's always felt odd to me that there are not more declarative APIs to register variations or in this case, aliases.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a method that is a bit more declarative, you can use the variation_callback when registering the block itself which you can see an example of here.

It still feels different to the client-side registration where you can register them separately using an imported function.

@ockham
Copy link
Contributor

ockham commented May 14, 2024

I'd been largely unaware that we already have some mechanics for block variations in place on the server side. (I'd thought they were almost entirely confined to the client.) This comment by @ntsekouras helped me quite a bit.

I'd love to keep the API surface as minimal as possible; if we already have variation_callback for individual blocks, and get_block_type_variations on a global level, it might be somewhat confusing for extenders if on top of that we add functions and/or filters to create an alias, i.e. a named block variation, as it's only really a block variation plus a name; not sure this small amount of extra data warrants a separate API. To make matters worse, block variations already support a name field (which is used to distinguish variations of a given block, vs globally).

I'm thus inclined to start small -- YAGNI and all. To avoid confusion of the alias and the existing name field ("Which do I use for what?"), how about we generate the alias from the canonical block type, with the variation name appended? E.g. the wordpress variation of the core/social-link block gets core/social-link/wordpress assigned as its alias (as previously discussed on a video call).

➖ This has the drawback that 3rd-party plugins cannot use their namespace for variations they add to existing blocks; e.g. WordPress/gutenberg#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. Not ideal, but might be okay. (It'd be nice to include some sort of vendor prefix/namespace in the last part to avoid collisions, i.e. something like core/query/woo-product-list. This might be a bit trickier, though.)

➕ The approach has the benefit that we wouldn't need to store an explicit reference to the underlying ("canonical") block's name (previously discussed here), as the canonical name is part of the alias -- WordPress will always know that core/query/product-list is just an alias of core/query.

0️⃣ Another aspect of this approach is that all existing variations automatically get an alias assigned. I'm not sure that's a net negative or positive; it seems somewhat convenient at first glance, as extenders can simply start referring to variation aliases instead of being limited to "actual" block types; e.g. in the Block Hooks/generic blocks example (#6228), it was considered a drawback in terms of Developer Experience that block variation authors would need to manually create an alias for their variation.
One potential downside could be that we end up with a lot of unneeded aliases (which we'll treat like "normal" block types in many instances, after all). This could lead to a bit of clutter; probably not so much in terms of UX (where these changes won't have much impact at all), but maybe for developers.

@ockham
Copy link
Contributor

ockham commented May 15, 2024

Some more notes on the namespace/blockname/variation alias idea (e.g. core/social-link/wordpress):

The parser doesn't currently allow more than one slash, so we'd need to change that (see e.g.).
Moreover, we'll have to deal with an edge case: In block markup, it's permitted to omit the core namespace (unlike any other namespace), so the following is valid block markup:

<!-- wp:social-link {"url":"wordpress.org","service":"wordpress"} /-->

But that means that if we allow appending the slash-separated variation, things can get ambiguous. Consider

<!-- wp:social-link/wordpress {"url":"wordpress.org","service":"wordpress"} /-->

This can now mean two things:

  1. The wordpress variation of the core/social-link block, or
  2. The wordpress block in the social-link namespace.

I'm inclined to think that this ambiguity is not a deal-breaker, but I think that we'll need to have the first possibility always take precedence over the second (so that Core blocks can't be spoofed by "malicious" plugins). This will have the side effect that if there are any plugins that include blocks with a namespace that happens to collide with an existing core block name, they'll stop working. Not sure how much of a problem that could be.

@ockham
Copy link
Contributor

ockham commented May 15, 2024

➕ 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.

@ockham
Copy link
Contributor

ockham commented May 15, 2024

Finally, using these "auto-generated" aliases is 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.

(It could allow true/false/string as values; string to manually set it, true for the auto-generated alias, false to suppress it. We could also default to false if we'd rather avoid clutter. But that's something to consider later IMO.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants