Block API: Add role for 'internal' attributes not copied on duplication #34750
Conversation
stacimc
added
[Feature] Block API
|
Size Change: +82 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
|
This works as expected for me when using the block |
|
Nice approach @stacimc, I like the addition of the |
packages/blocks/src/api/factory.js
Outdated
| @@ -32,6 +32,7 @@ import { | |||
| } from './registration'; | |||
| import { | |||
| normalizeBlockType, | |||
| __experimentalStripInternalBlockAttributes, | |||
| __experimentalSanitizeBlockAttributes, | |||
There was a problem hiding this comment.
@kevin940726 @noisysocks It'd be good to look into whether this PR makes it possible to remove __experimentalSanitizeBlockAttributes, and update the widget editor to use an internal attribute instead.
There was a problem hiding this comment.
Maybe! __internalWidgetId can be set on any block type, so we'd have to hook into registerBlockType and add widgetId: { type: 'string', __experimentalRole: 'internal' } to the schema of every single registered block.
If we can do that then that will make __experimentalCloneSanitizedBlock unnecessary as this was only added so that duplicating blocks would not copy any attributes that don't exist in the schema e.g. __internalWidgetId. See #29111.
It might be nice to try and do all of that along with this change so that there is a usage of __experimentalRole: 'internal' within core. We're less likely to break APIs that we ourselves use 🙂
There was a problem hiding this comment.
When discussing it in #29693 I envisioned that the API proposed in this PR would be sufficient to refactor the __internalWidgetId use case. It'd be great to confirm that.
There was a problem hiding this comment.
I can try implementing what I described above tomorrow unless @stacimc wants to give it a try 🙂
There was a problem hiding this comment.
Unfortunately I think this may require a little more tinkering -- as is, I'm running into the same issue in #29111 where internal attributes get dropped when the user makes changes in the widget editor 🤔
In fact at the moment I'm seeing the issue even if I keep the separated cloneBlock and __experimentalCloneSanitizedBlock, because in order to fix the issue mentioned by @glendaviesnz where internal attributes aren't reset when you Copy + paste a block, I need to strip the internal attributes within createBlock as well. My somewhat tenuous understanding of the widget editor is that it fetches widgets and transforms them into blocks using createBlock -- so the internal attributes get reset every time you refresh the page. This ends up causing block invalidation errors.
Edit: I missed @ntsekouras' suggestion below which looks like a better alternative 😄 I'll see what I can do with this and take a shot at removing __experimentalCloneSanitizedBlock 👍
There was a problem hiding this comment.
Thanks for working on this @stacimc!
IMO this PR could be the place to remove the __experimentalCloneSanitizedBlock as mentioned in comments.
What we need to handle this properly is something like the cloneBlock without the new clientId (that means recursively applying this to innerBlocks). IMO a good place to add this functionality for copy action is here: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-editor/src/components/copy-handler/index.js#L124, before the blocks' serialization with something like this:
let blocks = getBlocksByClientId( selectedBlockClientIds );
if ( event.type === 'copy' ) {
blocks = blocks.map( ( block ) =>
__experimentalRemoveAttributesByRole( block, 'internal' )
);
}
|
Some updates:
Unfortunately I don’t think it’s going to be so easy to get rid of
Making That being said, I think it might still be worth making
In my latest commit, I've hooked into |
Yep, that all makes sense 🙂 |
packages/widgets/src/utils.js
Outdated
| 'blocks.registerBlockType', | ||
| 'core/widget/addAttributes', | ||
| addAttributes | ||
| ); |
There was a problem hiding this comment.
Probably it doesn't affect anything in practice but it might be better to have this addFilter() call inside a function which initialize() in edit-widgets and customize-widgets calls. That way merely importing @wordpress/widgets doesn't cause something to happen.
packages/widgets/src/utils.js
Outdated
| * | ||
| * @return {Object} Updated block settings. | ||
| */ | ||
| function addAttributes( settings ) { |
There was a problem hiding this comment.
Can we give this a more specific name? Maybe addInternalWidgetIdAttribute?
I haven't checked the changes yet, but in which case would we need to have a default value of an internal property? Wouldn't the absence of this prop be enough for a block's edit function to handle the generation of that prop? |
stacimc
requested review from
adamziel,
draganescu and
tellthemachines
as code owners
@ntsekouras In the case of Top of my head I can't think of any great use cases where someone might want an |
stacimc
force-pushed
the
try/internal-block-attributes
branch
from
d3509cc
to
5e28faa
Compare
|
Added a couple of changes: Removed use of __experimentalSanitizeBlockAttributes in __experimentalCloneSanitizedBlock Fixed a bug when moving inner blocks between sidebars in the Widgets section of the Customizer This caused problems because we were using the absence of the I'm calling this out because it does mean that we can have dangling |
It doesn't sound like a compelling reason to keep an experimental public API. If possible let's test the same functionality through the public method which is what we care about the most. |
|
Thanks for connecting those dots @gziolo! What @youknowriad says in #38191 (comment) is very relevant and sways me away from a
I think I'm leaning towards renaming We would then have two roles:
Then, to fix the problem identified in #34750 (comment):
Sorry to derail the conversation @stacimc—you've put your foot into some interesting problems here 😀 |
@noisysocks Not derailed at all, I think it's important to get this right!
My concern with this is that ultimately the goal of this PR is to provide a way to make an attribute that isn't copied on block duplication -- and I'm not sure it's obvious or desirable that an attribute with the I don't think it's perfect but I'd still lean towards
Would you prefer this approach to bringing back |
Apologies if I'm missing context, but I quite like the idea of That's clearly well outside the scope of this PR, but just thought I'd share the potential future use case in case it helps with deciding on naming! |
It seems like maybe we have two concepts: role (content, identifier, styling, etc.) which gives semantic meaning to an attribute, and access (public, internal, private) which controls where an attribute can be used (client, server, serialised HTML, etc.) I don't really think public, internal, private, etc. are intuitive, though. Do you think it would be clearer to specify precisely what behaviours are desired? For example,
Yeah we can bring back |
|
Even with the brief discussion here it's clear that singular roles are an over-simplification of an attributes relevance in various contexts. At least having a set of roles that an attribute participates in could go a long way to helping us build a reliable interface for attribute selection. Granted, it's clear that these roles can be dependent upon the values of the attributes, but at least as far as describing an attribute's role goes specifying a set of roles or context in which that attribute carries weight goes pretty far. Maybe it could resolve some of the inherent tension behind trying to figure out what exactly and attribute is by asking instead "do you do this too?" These are contexts in which the attributes are relevant or possibly capabilities each attribute has. "attributes": {
"myInternalAttribute": {
"type": "string",
"default": "my-default-value",
"__experimentalRoles": {} // an empty set means we participate in no roles
},
"myNormalAttribute": {
"type": "string", // no property set means we participate in everything
},
"produceId": {
"type": "number",
"__experimentalRoles": { "copy": false } // everything except false values
},
"styleThing": {
"type": "string",
"__experimentalRoles": { "style": true, "copy": true } // only these things
}
}, |
My initial idea for this work was along those lines -- I actually opened an earlier PR (#32604) that added a I like
I think this would help considerably. It could possibly get a bit tricky if an attribute attempts to configure multiple roles that cannot be simultaneously held, but it's definitely clear that limiting an attribute to a single role is insufficient if we're going to use |
Agreed. It's difficult to draw neat boxes around different behaviours, and, even when you do, naming them in a way that makes it clear to extenders what those behaviours are is very difficult.
Agreed. This feels more flexible and future proof to me. It's analogous to what we already do with block definitions with
I think "attributes": {
"content": {
"type": "string",
"role": "content",
},
"productId": {
"type": "string",
"supports": {
"copy": false,
},
},
}const contentAttributes = filterBlockAttributes( block.name, block.attributes, { role: 'content' } );
const copyableAttributes = filterBlockAttributes( block.name, block.attributes, { supports: { copy: true } } ); |
|
Since it's a different approach to the API, I've opened up #38643 as an alternative to this PR following suggestions by @noisysocks 😄 That PR removes the use of |
|
Sorry for catching up late with this one and thanks for all the work here @stacimc!
For me the above feels what's the case here. I don't think we should try to mix two different things in one API. For me the use case we need here is to have a way to mark an attribute as unique and handle everything that this entails, like Why do we need to create a new I think that many discussions here have been triggered by the I'll catch up with the code now, but that were my first thoughts after reading all the discussions. |
This is definitely a concern I had which led me to propose a function-based approach. For one, I imagine many blocks want to look at the values to determine these things; two, it seems like something where the potential space of roles and context these things play can kind of explode in complexity if defining that with a DSL. The idea of having both seems workable to me: either you have a list of relevant roles for an attribute or you supply a function to work out the attribute sets. Even with a DSL here we're not going to have the problem solved on the server since attributes still need to be sourced from the block's HTML. It would only be able to be relied upon in the server if coupled to a |
|
Thanks @ntsekouras, @dmsnell.
The
The problem I've been having is finding a semantic 'role' that strictly describes the behavior (attribute isn't copied on block dupe/copy).
We can support multiple |
for what it's worth I find I wouldn't expect
This reads to me more like attributes that identify the block, which I think would normally imply that they copy when duplicated instead of the inverse. Could these be other things that aren't ids that serve the same purpose which could help us figure out a good name for them?
Maybe these are some kind of The concept of |
This is not flexible as functions aren't serialisable and we need to have access to things like that server side and sometimes we need to register things server side. The same problem occurred for
I can see your points and maybe it's okay to try with For the record the use case that made me look into this problem more is the So I guess I'm going to check your other PR 😄 |
|
Will this take in account also the removal of unique attributes when we export content? |
I'm leaving this open for the moment as discussion is ongoing, but it's likely this PR will be closed in favor of #38643. That PR removes the use of So to answer your question @Tropicalista I think that approach would only handle removing those attributes on block copy/duplication -- but an |
I actually think it's a point in favor of the current PR because with the "supports", we'd have to think and add a flag for everyone case where the current PR adds meaning/semantics and not be dependent on specific use-cases. We can make informed decisions for each use-case depending on the role of the attribute. |
|
@youknowriad: What "role" do you think something like |
|
@noisysocks I see the point, I think it's not clear when you say this blocks is identified by this
To clarify, I'm not totally against |
@youknowriad The earliest implementation of this work (#32604) was actually exactly that! The concern was that |
Shouldn't you be able to duplicate and keep the same productId if that's possible? |
I agree with Riad. I think in the above use case we don't need any semantic marking. It's just the default behavior for block attributes. Am I missing something? |
I was just pointing out why the name Other suggestions were |
|
Yes, to me, I still like {
attributeName: {
type: 'number',
supports: {
copy: true, // whether attribute remains when block is copied or duplicated
serialize: true, // whether attribute remains when block is serialised to HTML
restApi: true, // whether attribute is shown in the REST API
undo: true, // whether changes to the attribute are undoable / redoable
},
}
}At any rate, I feel the discussion is going in circles a bit and that we ought to lean on |
Closes #29693
Alternative to #32604
Description
This PR:
internalattribute experimentalRole. Attributes registered with this role will not be copied on block duplication.__internalWidgetIDto use the new experimental roleExample fragment of block.json:
For just one example of how this might be used: the Jetpack
Pay with Paypalblock uses aproductIdattribute to identify the actual product record referenced by the block. This data is meant to be internal to the block and should not be exposed to the user. When the user duplicates the block, we want to copy the other attributes used to store price info and other details, but create a brand new product record/productId.This is similar conceptually to #32604 with a few changes:
internalterminology rather thanduplicableorunique, which implies additional constraints__experimentalRoleand associated utilities (thanks @ntsekouras for the suggestion)Notes
It respects any configured default valuesDefault values are not filled in on block duplication either; support for this could be easily added if a viable use case becomes apparent, thoughsetAttributescontentattribute of the Code block would not make sense, because although the attribute would not be copied on duplication, it will populate with the same value from the html.Internal attributes are not removed when you convert a block into a Reusable block. I don’t know a ton about Reusable blocks, so calling this out so that someone more familiar can correct me; my understanding is that multiple instances of a Reusable block are not independent of each other (ie editing one copy affects them all), so internal attributes should also be shared.
How has this been tested?
__experimentalRole: 'internal'to any block attribute.Test with attributes that have adefaultvalue and ensure that the default is respected in the duplicated blockCopymenu item, and by keyboard commandsScreenshots
Types of changes
Checklist:
*.native.jsfiles for terms that need renaming or removal).