[Block Attributes]: Add support for non-duplicable block attributes

[stacimc]

Description

Closes #29693

This PR adds the ability to mark block attributes as unique, such that they will not be copied on block duplication. The goal is to allow blocks to use attributes to store unique properties that persist across page loads (unlike clientId).

For just one example, the Jetpack Pay with Paypal block uses a productId attribute 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.

The behavior is opt-in, so blocks do not have to explicitly set unique: false. By default all attributes will continue to be copied on duplication.

Notes

• Is unique the best name for this attribute field?
  • An option is duplicable, but the conditional logic is simpler/clearer if the field is false by default (ie, it's confusing that an attribute is considered 'duplicable' if the field is explicitly set to true or if it not defined).
  • Another option is something along the lines of private, to suggest that it should not be copied. My concern is this, like unique, may be an overloaded term.

How has this been tested?

You can test by adding unique: true to any block attribute, but the Jetpack Pay with Paypal block is a particularly illustrative example. Update its block registration in index.js:

{
    productId: {
        type: 'number',
        unique: true,
    }
}

1. Add a Pay with Paypal block to a new post and fill out its fields.
2. Duplicate the block.
3. Inspect the blocks in the code editor and verify that while all other attributes have been copied, they have different productIds.
4. Edit the fields of the duplicated block.
5. Save the post.
6. Reload the page.
7. The blocks should appear as expected -- edits only affected the second block. Without this changeset applied, the edits would affect both blocks, since they would be applied to the same underlying product resource.

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• I've tested my changes with keyboard and screen readers.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.
• I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

[glendaviesnz]

This tested as advertised for me, but just a couple of questions:

1. It doesn't seem to work for attributes that are extracted from the block content with a selector, eg. the image block alt tag. I don't know that there would be a use case for this, so it may be just a case of making it clear in the docs that it won't work for these instance.
2. I wonder if santize is the correct prefix. I think unique is reasonably clear, but sanitize seems to indicate it is removing dangerous content, but not sure what a good alternative is, dedupe maybe?, but not a big issue

[ntsekouras]

Thanks for working on this @stacimc !

I've commented on the issue about a possible approach I'd been thinking about and wanted to share here as well: #29693 (comment)

It involves __experimentalRole and __experimentalGetBlockAttributesNamesByRole util.

[gziolo]

Bringing the comment from @ntsekouras for better readability:

I've been thinking about this and a possible solution I had in mind to try out involves these:

1. Set the unique attribute value on load only if it doesn't exist and mark this change as persistent or not depending on your needs (add undo level). Something like this: https://github.com/WordPress/gutenberg/blob/trunk/packages/block-library/src/query/edit/index.js#L94
2. Use the __experimentalRole attribute to something like id or unique etc.., and then use the existing __experimentalGetBlockAttributesNamesByRole util to get them.
3. Handle them in the appropriate places, like in the duplicate function and where is needed.

All the above are using experimental APIs though which were introduced here: #30469. I guess this could be okay though as the new functionality should also be experimental for start.

There is also a relevant comment from @talldan:

This could also be relevant for the widget editors, which store an internal widgetId as an attribute.

Just wanted to bring one consideration. The name unique can have some implications driven by the expectations it creates for developers - all values for the attribute stored in blocks of the same block type should be unique. It means that not only duplications action but other actions like inserting, replacing blocks should ensure that the value of the attribute marked as unique is never duplicated.

[stacimc]

Just wanted to bring one consideration. The name unique can have some implications driven by the expectations it creates for developers

Definitely agree. I considered something like 'private' or 'internal' but those have technical implications of their own. duplicable is more specific and not such a loaded term, but I'd prefer the simplicity of an opt-in field that's false by default (ie, it's confusing that an attribute would be considered 'duplicable' if that field is explicitly set to true or if it's not defined at all).

[kevin940726]

Instead of passing a boolean value as an argument, maybe passing an optional object with explicit name would be better?

__experimentalSanitizeBlockAttributes(
	name,
	attributes,
	// What does it mean?
	true
)

__experimentalSanitizeBlockAttributes(
	name,
	attributes,
	{ sanitizeUniqueAttributes: true } // clearer
)

__experimentalSanitizeBlockAttributes(
	name,
	attributes,
	{ shouldSanitizeUniqueAttributes: true } // Even better with explicit boolean-like prefix
)

However, I wonder if instead of passing an additional option here, maybe we can just create another function for this?

[stacimc]

Agreed -- the named option is a big improvement. I initially thought to create another function for this (since as @glendaviesnz pointed out, de-duping is also a bit out of place as part of 'sanitize' functionality), but it's a bit rough on code duplication. I can take another stab at refactoring here if we go with this approach.

[kevin940726]

I'm wondering if we can just create a new type called ID (Borrowed from graphql). By definition, it should be a serializable, unique, non-human-readable string. Users can transform it as a number if they wish, but I think string is a good default.

I don't understand how default value could be useful in this case though 🤔. Ideally, unique should mean that the value should be unique across the app, and default values break that rule. I understand sometimes it's necessary in the current architecture though, like when pressing Enter in a block to create two new blocks while destroying the original one.

[talldan]

I think the current types are taken from JSON schema.

• https://developer.wordpress.org/block-editor/reference-guides/block-api/block-attributes/#type-validation
• https://json-schema.org/draft/2020-12/json-schema-validation.html#rfc.section.6.1.1

It might be ok to deviate from that, I don't know whether there are any definite type restrictions in other parts of the codebase or the REST API. Certainly what's in the blocks package is pretty easy to change.

(edit: this is where I read that -

gutenberg/packages/blocks/src/api/parser.js

Lines 65 to 74 in 7428d7e

	/**
	* Returns true if value is of the given JSON schema type, or false otherwise.
	*
	* @see http://json-schema.org/latest/json-schema-validation.html#rfc.section.6.25
	*
	* @param {*} value Value to test.
	* @param {string} type Type to test.
	*
	* @return {boolean} Whether value is of type.
	*/

)

[stacimc]

My initial idea was to do something like this. I abandoned the approach for the same reason @talldan pointed out, and because making an id type creates much stronger expectations of enforcing uniqueness.

When I raised the issue, for my purposes it was sufficient to make attributes non-duplicable rather than strictly unique, an easier bar to clear. It's pretty clear that the naming in the current implementation conflates these ideas, so I've renamed the attribute key to duplicable.

[Tropicalista]

When will be this merged?

[ntsekouras]

Use the __experimentalRole attribute to something like id or unique etc.., and then use the existing __experimentalGetBlockAttributesNamesByRole util to get them.

Hasn't this approach been considered at all? It involves similar logic with this one (attribute approach) with the difference that we already have that attribute (__experimentalRole) and some utils 😄

[gziolo]

Let's close this one in favor of the alternative approach @stacimc proposed in #34750. It would be also great to ensure that the other approach lands in the near future 😄