[WIP] Block Supports: Try stabilizing default controls to top-level property #67729
Conversation
aaronrobertshaw
added
[Type] Enhancement
|
Size Change: +141 B (+0.01%) Total Size: 1.83 MB
ℹ️ View Unchanged
|
|
@youknowriad this PoC should give some indication as to what is required to move default controls configuration to a top-level property. I got a little lost trying to get something working here, so there's a good chance I'm missing something, or there's a better way of doing this. That uncertainty and going in circles trying to understand how our block types are handled across the server and editor doesn't leave me with much confidence to foresee issues with this approach. The biggest catches I've found so far are:
As noted earlier, I'm a bit confused around what exactly is needed on the core side of things right now, so take that last point with a grain of salt. I can take a closer look at this tomorrow with a fresh set of eyes and mind. Following that though, I'll be AFK on and off for the next couple of weeks. It might take me some time to wrangle this if we decide to go ahead with the top-level property. Any thoughts? |
|
It seems some kind of hacky backwards compatibility code is needed regardless of where the feature end up. I would love a recap of the "public" APIs that we need to maintain backward compatibility for. It's a bit hard to read the changes here, but my understanding is that we're shiming in multiple places and ideally we should only do it in a single place. |
It felt that way during my explorations today. There are several things required to introduce a top-level block type property on the core side (see diff in PR description) as well as in Gutenberg (see 06a44a9). On the core side I needed to add
I don't know how easy it would be, or even if it's possible, to override all that within Gutenberg for backwards compatibility.
I'll give it a go, if I've gone down the wrong track here let me know 🙂 The current plan for stabilization of experimental supports started with #63401, which involved stabilizing the block supports :
Experimental support keys were to be replaced with stable counterparts. The following key maps reflect what is being stabilized in these overall efforts. export const EXPERIMENTAL_SUPPORTS_MAP = {
__experimentalBorder: 'border',
};
export const COMMON_EXPERIMENTAL_PROPERTIES = {
__experimentalDefaultControls: 'defaultControls',
__experimentalSkipSerialization: 'skipSerialization',
};
export const EXPERIMENTAL_SUPPORT_PROPERTIES = {
typography: {
__experimentalFontFamily: 'fontFamily',
__experimentalFontStyle: 'fontStyle',
__experimentalFontWeight: 'fontWeight',
__experimentalLetterSpacing: 'letterSpacing',
__experimentalTextDecoration: 'textDecoration',
__experimentalTextTransform: 'textTransform',
},
};The bulk of those should be relatively painless because it is only the key changing, not where they live. For extenders developing custom blocks that opt into these block supports they won't need to change anything immediately. Ideally, though, they'll migrate their custom block's block.json or block type definitions to use the stable support keys. If 3rd parties are filtering block type args or metadata, they'll continue to function for the most part. The main edge case here is where there are multiple plugins/themes filtering the same supports using a mix of experimental and stable keys to change the same support. That scenario can't be accounted for 100% so the approach taken will check the insertion order in the config to make a "best guess" as to which value was updated last and use that. If plugins or themes are checking block supports in custom code, they'll need to update that code to check the stable key. This all would apply to During my tinkering today, I also looked at monkey patching FWIW a quick search suggests that 1656 plugins are using I hope that wall of text helps paint a clearer picture 🙏 |
I think these changes are ok (the core ones) but I do believe we should be able to do that in Gutenberg, unrelated to defaultControls. it shouldn't be a pain point for us to introduce a new optional key to block.json
My main thinking here is that we should not be patching the "defaultControls" at registration time (both server and client). (I guess the same reasoning can apply to block supports and any deprecated property). Instead, we should be patching the selectors we use to retrieve these. In simpler words:
Doing this at the selector level prevents any potential hacks we need to do to support registration/filtering... These are obviously just theoretical thoughts, maybe there's a flaw in this reasoning as I know these things are complex. Also it seems the same issues apply to "defaultControls" and "block supports" stabilization regardless of whether the stable keys live, so I would say this cumbersome deprecation shouldn't be impacting the decision on where to put "defaultControls" (Correct me if I'm wrong). It seems the main difference between top-level or not, is the difficulty to add a top level property in Gutenberg (overriding Core). |
|
Appreciate the continued guidance @youknowriad, thanks 🙇
Agreed, it shouldn't be a pain point. I'd say a large part of the issue is I'm not very familiar with this area so it's tougher to know what needs doing and how. I'm hopeful I can figure it out, it might just take some time.
If I understand correctly, we'll face the same edge cases around filtered values. Wherever we fetch and merge the stable and experimental values, we need to decide on how that is done.
If the decision purely comes down to where makes the most sense for The hope with the block supports stabilization was to have this all in place early in the 6.8 release cycle. If we revisit the approach to handle all this within selectors (which I'm on board with), it will push back when this all lands until into the new year.
That's it in a nutshell. |
I've taken another run at overriding core to add the default controls property within Gutenberg. In 0f4914a I've managed to avoid all but one core update. What I haven't been able to solve yet is updating the fields to pick in core's After 0f4914a and very quick initial tests, it appears that if I define Given that, my latest plan is to remove the stabilization of experimental block supports from the Happy for nudges in the right direction if I'm heading down the wrong track. |
|
Thanks for looking into this @aaronrobertshaw and exploring the options.
That sounds like a good option if it takes the pressure off, and gives folks time to come up with robust approach. We haven't committed the core patch yet, so it can be put on ice at any time. |
|
Your proposed plan sounds good to me.
Insertion order seems as arbitrary as preferring stable keys no? What are we hoping to gain by doing this? |
It is arbitrary, true, but a little more flexible than simply preferring the stable key values. The plan to date has been to update all the core blocks to use the stable block supports as soon as this stabilization is solid. That is partly due to our block library effectively forming part of our overall documentation and setting examples for 3rd parties. If we prefer stable over experimental values, all plugins that filter these supports would see their filters fail to take effect. Using the insertion order helps alleviate that for the majority of use cases. It doesn't solve an edge case where multiple filters of the same support config use a mix of experimental and stable keys, however, this scenario is much more contrived than a plugin being slow to migrate its filters to use stable keys. In other words, by using the insertion order, we can provide some backward compatibility for the following situation:
Given that scenario, do you have a preferred approach? |
What if the order is reversed? |
Do you mean our order of preference? So preferring experimental key values over stable? We'd have a similar scenario/problem.
Using the insertion order approach with the above scenario:
Likewise, using the other scenario:
The block starts with the stable key, then gets the experimental afterwards via the filter. The experimental key is inserted last and so is preferred over the original stable one. Leaving us with the expected value, that of the filter. To illustrate the edge case where this falls over, imagine the following:
Here, the block starts with the stable key and the experimental key is added later by the plugin filter. Using the insertion order to determine/guess which value was updated last fails here as it would indicate the experimental key should be used. The expected outcome would be for the theme's filtered value to be used. Apologies I wasn't clearer before when referring to the insertion order. Hopefully, I've done a better job communicating this time around. |
|
I see. Thanks for detailing. I think it's your call if you want to keep the "insertion order" behavior or not (depending on how complex it is). Personally, I'm ok optimizing for the common case which sounds like:
|
|
@youknowriad I've been attempting to implement a selectors-based approach to stabilizing block supports and it is proving more difficult than hoped for. I'd like to propose a few options to move forward below and would love to get your thoughts. Possible options:
Keeping the existing migrate-on-registration approach Pros:
Cons:
Existing migrate-on-registration approach but skip stabilizing defaultControls Pros:
Cons:
Revert existing block support stabilization to buy time to find cleaner solution Pros:
Cons:
Selectors-based Approach Here's a bit of a TL;DR on the issues encountered with updating selectors to handle stabilization.
Summary On their own, most of the issues or cons listed above aren't major. With possibly the exception of needing more time to communicate required changes for 3rd parties. I'm genuinely at a bit of a loss as to how to stabilize these supports in a cleaner fashion than we already have in Gutenberg or in any way that won't require 3rd parties to tweak something. I'll be AFK for the next couple of weeks, into the new year, so the revert option is looking like the best course of action to me right now. I'd be more than happy to hear alternative opinions though! Given that, I can get a PR together to revert the stabilization of supports already merged in Gutenberg. If we decide to pull the trigger on that, perhaps @ramonjd might be able to hit merge on the revert for me while I'm away 🙏 |
|
Huge kudos to all the work so far @aaronrobertshaw And it's great to have our options laid out like this, I appreciate the diligence and clear communication.
Just wanted to note that WordPress has previous form with regards to this approach. See the wp_migrate_old_typography_shape callback to the
What is the basis for this requirement? It seems separate to me. We're talking about stabilization, not API enhancements. Blocks could happily live without it in the meantime. I understand it might make sense to bundle it in with stabilization changes, if it creates too much tech debt later to try to patch in (?). In that case, it's a solid case for reverting this and doing things more methodically and safely.
I think we should feel comfortable in saying we need more time. Nothing is broken, except maybe a few promises made by other folks, no API will change. Gutenberg, as every knew from day one, is an experimental plugin, and so long as it's not in Core I think we have licence to revert things that don't make sense, or that jeopardize backwards compatibility. |
|
A revert PR for the original approach is up in #68163. |
Related:
What?
This PR is a proof of concept to see what it would take to stabilize current
supports.feature.__experimentalDefaultControlsconfig to its own top-leveldefaultControlsproperty instead of being stabilized within thesupports.featureobject.Why?
There's a desire to configure non-block-support controls via the
defaultControlsproperty. This makes a touch more sense if it is a top-level block type property.How?
Requires core updates to works (at least to my current knowledge)
__experimentalDefaultsmove the config to a new top-leveldefaultControlspropertydefaultControlsexists. That already stable config indefaultControlswill be usedsupports, this approach can't determine the order the experimental or stable default controls config was defined in so is slightly more "dumb" and would be a little more prone to ignoring plugin or theme filtered default controls. It is also then an exception to how the rest of the experimental block supports are stabilized.__experimentalDefaultControlstodefaultControlswithin a currentgetBlockSupport()call. Now, the config no longer exists within block supports, they'll need to update to calling a new utilgetBlockDefaultControls.Testing Instructions
Proper testing instructions are TBA once it's confirmed we want to go in this direction.
Core diff needed (I think 🙂 )
Screenshots or screencast
There should be no visual difference if this PR hasn't broken something 😅