Solving some semi-related shortcomings with block attributes

[noisysocks]

Part of #41236.

We have occasionally ran into a few limitations of block attributes while developing the block editor. In this post I'll describe some common classes of problem and, for each one, link to some rough prototypes that demonstrate how it may be solved.

1) Constructing a block with data from a transform

Problem

When creating a block, sometimes we need to pass data to it that is used for initialisation but then should not be serialised into post_content.

Examples:

• When dragging-and-dropping a file into the block editor, a File block is created via a file transform. The file is stored in a temporary blob URL and passed to the File block via the href attribute. The file block, when mounted, uploads the temporary file. When the upload completes, the File block updates the href attribute with the actual file URL.

  This creates an extraneous undo level which can be seen by pressing Undo once the file is complete. The File block returns to an invalid state where href is set to the temporary blob URL.

  Furthermore, if a user saves the post while the file is uploading, then the temporary blob URL is saved to post_content which is invalid.

  This exact problem applies to the Image and Video blocks as well.

• We wish to have a block transformation that can transform a Legacy Widget block containing a Menu widget into a Navigation block. See Widget Importer: ensure it works with importing menus #47285. To do this elegantly we could have the transform create a Navigation block and pass the classicMenuId to the Navigation block's edit function. There is currently no way to do this, however, without creating an extra attribute and therefore extraneous undo levels as above.

Potential solutions

• Using "block params" (Proposal: Block Params #49391), we could pass data from the transform to the edit function in a param.
• Using "internal attributes" (Proposal: Internal block attributes #49457), we could pass the data from the transform to the edit function in an attribute marked internal.
• We could simply pass data from the transform to the edit function using a shared Map (Use Maps to pass "constructor args" to blocks from a transform #49456) and avoid any API changes.
• We could pass data from the transform to the edit function by setting an attribute that isn't defined in block.json. Because the attribute isn't in the block schema, it won't be serialised or copied. This is currently undocumented behaviour (used by the widgets editor) but we could document it.

2) Associating Widget IDs with blocks

Problem

The widget editor works by adding a hidden __internalWidgetId attribute to each block in the editor. This allows the editor to tell which widget to update when the user presses Save. If a block doesn't have a __internalWidgetId set then the editor knows to create a new widget.

The implementation relies on the fact that attributes within block.attributes that do not have a corresponding definition in blockType.attributes are removed by wp.blocks.serialize() and wp.blocks.cloneBlock(). This is undocumented behaviour—a more robust solution would be nice to have.

Potential solutions

• Using "block params" (Proposal: Block Params #49391), we could set the widget ID on the params object i.e. block.params.widgetId = widgetId. This ensures that it won't be serialised or copied in a documented way.
• Using "internal attributes" (Proposal: Internal block attributes #49457), we could add an internal widgetId attribute to all blocks. This makes it explicit that the attribute will not be serialised or copied.
• Using a block copy filter (Proposal: Block copy filter #49464) and the existing blocks.getSaveElement filter, we could add filters that removes the widgetId attribute from blocks before they are duplicated or serialised.

3) Storing foreign keys in a block

Problem

From #29693:

Blocks cannot use attributes to store information that is unique internal to the block instance but also persisted across page loads. Here's an illustrative example:

I'm working on the Jetpack Pay with PayPal block. It contains a productId attribute which uniquely identifies the actual product record referenced by the block. When a new block is inserted this attribute will be undefined; we hit the API to create a new product, and store the returned id in this attribute.

When the block is duplicated, I'd like to create a new product record with identical fields (title/price/etc). But because the productId is cloned along with all the other attributes, both blocks will point to the same resource. It's possible to keep track of the block's unique clientId and reset the product when this value changes — but while this detects block duplication, it also changes every time the post loads, and I only want to reset the product on duplication.

Potential solutions

• Similar to (but not quite the same as) "internal attributes" (Proposal: Internal block attributes #49457), we could have "non-copyable attributes" denoted with copy: false that means that an attribute is not copied when the block is duplicated. The third party would then set copy: false on the attribute.
• Using a block copy function (Proposal: Block copy function #49463), third parties could implement a copy function on their block that removes the attribute.
• Using a block copy filter (Proposal: Block copy filter #49464), third parties could implement a copy filter on their block that removes the attribute.

4) Derived attributes

Problem

Occasionally we have block attributes that are derived from other data that we wish to still be serialised into post_content and we do not wish for changes to the attribute to create undo levels, however.

Examples:

• packages/block-library/src/gallery/v1/gallery-image.js:89 - url and alt are derived from image
• packages/block-library/src/query/edit/query-content.js:77 - queryId is initialised to a unique number
• packages/block-library/src/cover/edit/index.js:142 - isDark derived from image
• packages/block-library/src/gallery/edit.js:182 - id and align are derived from image
• packages/block-library/src/file/edit.js:112 - fileId initialised automatically

Potential solutions

This is much trickier as the cases are so varied.

In some cases, a proper way to do internationalisation within the output of save() would make the need for a derived attribute redundant.

In some cases, the attribute always stored derived data. In this instances we could think about perhaps having an undo: false flag on the attribute similar to internal: true and copy: false above.

In other cases, the attribute alternates between being user-sourced and derived. In these instances we more or less have to mark the call to setAttributes() as one that should not create an undo level. This is currently done by calling __unstableMarkNextChangeAsNonPersistent. We could potentially make this a native part of setAttributes() e.g. setAttributes( changes, { undo: false } ).

Side note: Exploring "virtual attributes"

I did some exploration into creating a kind of proxy class called VirtualAttribute which lets you customise whether an attribute is copied, serialised, creates undo levels, etc. at runtime.

setAttributes( {
	url: new VirtualAttribute( blobURL, {
		undo: false,
		serialize: false
	} ),
} );

I don't think this solution is practical however. There are two problems:

• It doesn't help at all with problem (3) above because, in that case, we want the productId attribute to be serialised but not copied. We could use VirtualAttribute to mark it as non-copyable but this will not persist after the page is reloaded and blocks are re-parsed from post_content.
• The implementation of serialize: false is very complicated. Consider the case where you call setAttributes( { foo: 1 } ) and then setAttributes( { foo: new VirtualAttribute( 2, { serialize: false } ) } ). attributes.foo would then be 2 but serialize( block ) would result in { "foo": 1 }. To implement this, the block editor store would have to track both the last value of each attribute as well as the last serialisable value of each attribute. It makes more sense for this to be a property of the attribute at the schema level.

[noisysocks]

@youknowriad @gziolo: Here's a big write-up of a few problems we've been attempting to solve using a new abstraction ("transient attributes", etc.) Check out the PRs I've linked to and let me know if there's anything there you really like or really hate.

---

My 2¢:

I don't think there's a silver bullet.

I'm leaning towards either this set of changes if we feel that these problems are common enough to warrant an API solution:

• Add internal attributes (Proposal: Internal block attributes #49457).
  • Relatedly, add non-copyable copy: false attributes.
• Continue using __unstableMarkNextChangeAsNonPersistent. (Perhaps stabilise it? or make it a first-class part of setAttributes?)

Or this set of changes if we think these problems are pretty rare and that something minimal is best:

• Pass data from the transform to the edit function by setting an attribute that isn't defined in block.json. Properly document that this is supported behaviour, and add tests for it.
• Add a block copy filter (Proposal: Block copy filter #49464) for third party use.
• Continue using __unstableMarkNextChangeAsNonPersistent. (Perhaps stabilise it? or make it a first-class part of setAttributes?)

[youknowriad]

Thanks for the write up. Personally, I feel like having flags that trigger some particular behavior for attributes is a good approach. I specially like both "internal" (I'm assuming this one doesn't pass to "save" functions) and "copy". The only thing here for me is that I'm not sure how to name or whether we should "group" these properties somehow but also I'm not too concerned, whichever we pick could work, we just need to think forward: are there other flags we'd want in the future (even if we don't introduce today).

I have more reservations about the 4th use-cases. It seems to me that we should try to understand these a bit more:

1- For instance, for the ones that are "derived" from the image, can't we just make the "setAttributes" with the same call setting the image? Maybe there's some async behavior preventing us from doing so?

There are more questions to be asked here:

• What happened if the "save" is triggered after the image has been set but before the computed attribute update, do we end up with a broken "save"? Maybe these shouldn't have been attributes in the first place, it seems they're only there to augment the saved markup.

• filedId, queryId? Feels like there should be a way for a block to actually provide a "unique default" value. Or an "initializer" function for an attribute that is run automatically on createBlock: I'm guessing these are executed in a useEffect on mount, I've found that useEffect on block mounting generally mean bugs.

[noisysocks]

The only thing here for me is that I'm not sure how to name or whether we should "group" these properties

Yeah, this is always hard. Originally I was leaning towards describing the action e.g. serialize, undo, copy and grouping underneath supports i.e. supports.serialize. I can't think of any situations where serialize ≠ undo though so I decided to group them into a single flag. I also think private is a suitable name. If we wanted to allow more forward compatibility we could use access: 'internal' instead of internal: true.

are there other flags we'd want in the future (even if we don't introduce today).

The only thing that comes to my mind right now is perhaps we'd want to control which attributes are sent to the server as part of a ServerSideRender or something like that. Maybe that's another point in favour of access because you could have access: 'private' vs. access: 'internal'. (Feels like I'm writing C# again!)

I have more reservations about the 4th use-cases. It seems to me that we should try to understand these a bit more:

Agreed. I didn't spend too much time investigating all of these cases. They're all very different to one another. I think how we address them and start to slowly transition away from __unstableMarkNextChangeAsNonPersistent is a whole separate project by itself.

An attribute "initializer" is a good idea though that would work for some of these cases.

I don't really hate __unstableMarkNextChangeAsNonPersistent to be honest, though I think it should be more explicit via "transactions".

startUndoTransaction();
setAttributes( ... );
setAttributes( ... );
whatever();
commitUndoTransaction();

[gziolo]

Thank you so much for researching all these interconnected issues. All open PRs are really helpful in reasoning about the problems and possible solutions. I also appreciate you considering "virtual attributes" as one of the options that I mentioned,m. I totally agree in retrospect that it’s not only technically difficult to integrate but it most likely would be hard to code for folks.

Now, some thoughts I have after carefully studying this discussion and checking all linked resources.

Add internal attributes (#49457).
Relatedly, add non-copyable copy: false attributes.

We discuss this idea further in another thread: #49466 (comment). Putting aside the final syntax, I like how conceptually simple this solution is. Folks need only to define an attribute for transient information that is available as anything else, but all changes aren't stored in HTML, they are skipped by undo/redo, and they won't be copied when duplicating a block.

Now based, on that we can offer a way to disable copying the value by a similar flag in the definition of attributes. That would work in case someone still wants to persist the value.

We were joking about borrowing access concepts from Java or C#. It could be that the first case could be abstracted as private and the second as protected. It probably would be very hard to discover and document though.

At the same time, there is this question of what happens when we have atomic flags and someone sets internal and copy as true. It’s probably easy to implement but we need to account for it.

[gziolo]

Using "internal attributes" (#49457), we could pass the data from the transform to the edit function in an attribute marked internal.

It's also an alternative approach to #34750 where @stacimc proposed extending the __experimentRole to cover also internal as one of the options. Example:

"attributes": {
	"_blobURL": {
		"type": "string",
		"__experimentalRole": "internal"
	}
}

In effect, this PR could potentially close #29693.

---

Similar to (but not quite the same as) "internal attributes" (#49457), we could have "non-copyable attributes" denoted with copy: false that means that an attribute is not copied when the block is duplicated. The third party would then set copy: false on the attribute.

Here we also have a previous attempt #38643 to address the issue by @stacimc where he proposed a following syntax:

"attributes": {
	"myAttributeThatShouldNotBeCopied": {
		"type": "string",
		"default": "my-default-value",
		"__experimentalSupports": {
			"copy": false
		}
	}
}

That would close #29693, too.

[noisysocks]

I'm not too keen on role: 'internal'. To me "role" is the semantic meaning of something. Content, image, URL, etc. I think access: 'internal' would be more appropriate if we don't want to use a simple flag (internal: true).

[gziolo]

That’s a good point. The role would be expressed as metadata or something along those lines for something that exists only in memory during block editing.

Can you think of any other suitable value for access that would justify picking it over a simple internal: true?

[noisysocks]

From #49466 (reply in thread):

The only thing that comes to my mind right now is perhaps we'd want to control which attributes are sent to the server as part of a ServerSideRender or something like that. Maybe that's another point in favour of access because you could have access: 'private' vs. access: 'internal'. (Feels like I'm writing C# again!)

It's a stretch :)

[gziolo]

Also package private or protected like in Java 😅

Anyway, access with private/internal plus public (default) as values would work, too. We use schema/block.json so we can make the experience comparable to using a simple boolean.

[gziolo]

3) Storing foreign keys in a block

Problem

From #29693:

Blocks cannot use attributes to store information that is unique internal to the block instance but also persisted across page loads. Here's an illustrative example:

I'm working on the Jetpack Pay with PayPal block. It contains a productId attribute which uniquely identifies the actual product record referenced by the block. When a new block is inserted this attribute will be undefined; we hit the API to create a new product, and store the returned id in this attribute.

When the block is duplicated, I'd like to create a new product record with identical fields (title/price/etc). But because the productId is cloned along with all the other attributes, both blocks will point to the same resource. It's possible to keep track of the block's unique clientId and reset the product when this value changes — but while this detects block duplication, it also changes every time the post loads, and I only want to reset the product on duplication.

Potential solutions

• Similar to (but not quite the same as) "internal attributes" (Proposal: Internal block attributes #49457), we could have "non-copyable attributes" denoted with copy: false that means that an attribute is not copied when the block is duplicated. The third party would then set copy: false on the attribute.
• Using a block copy function (Proposal: Block copy function #49463), third parties could implement a copy function on their block that removes the attribute.
• Using a block copy filter (Proposal: Block copy filter #49464), third parties could implement a copy filter on their block that removes the attribute.

I am inclined to go with two API changes that address the problem for this set of use cases. I think it would also help to address "2. Associating Widget IDs with blocks", too. That's in fact a revised version of the proposal:

1. "we could have "non-copyable attributes" denoted with copy: false" – fully on board with the idea.
2. That would be combination of the copy function and the filter that I explained in detail in Proposal: Block copy filter #49464 (comment). We could borrow the idea from transforms.ungroup proposal and offer transforms.copy as a way to modify the list of attributes, or even blocks that get created when copying the block.