Fix "undo trap" by introducing "transient" attributes

[mcsf]

Fixes #8119

Edit: I'd like to explore another solution, but leaving this open for a while to discuss.

Gallery: conversion from shortcode

Image: upload new image

Embed: convert from classic

File: upload bad file

Description

How has this been tested?

Screenshots

Types of changes

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.

[aduth]

I need more context to understand the purpose its serving, but in the meantime I'll preemptively state my distaste for the exceptional treatment to "secret" attributes.

[tofumatt]

Just piling on here: without suggesting a better solution, I worry about these too. While quite explicitly not part of the public API, I can imagine third-party folks finding and using these… and I also don't really like us using these kind of "secret" attributes in our own API.

I haven't seen it elsewhere in the codebase; it seems a bit un-React-y to me as well, but that's just, like, my opinion. 🤷‍♂️

[mcsf]

Just quickly replying from my phone: I don’t disagree with any of that, hopefully this is what the review is for. I’ve explored one or two other ways (see description of parent issue) to solve the problem, and none is entirely clean across the board. I still conceptually like the idea of “events comprised of multiple actions” better, but it required using Redux in an... original way.

Anyway, I think a solution is very much needed for the issue. What alternative routes do you see? Silent attributes are a way to easily pass a piece of state to a block’s edit method, like a seed for the block to boot. I could imagine something requiring tweaking the createBlock interface, but I don’t know how we can move forward without interfering with the interfaces.

[mcsf]

I'd like to explore another solution, but leaving this open for a while to discuss.

[mcsf]

I need more context to understand the purpose its serving, but in the meantime I'll preemptively state my distaste for the exceptional treatment to "secret" attributes.

@aduth, the parent issue should provide enough context — otherwise, ask me anything.

Does your distaste stem from the implicit nature of it (hidden attributes, no declaration in the type, a sketchy "convention" around the use of underscores), or is it more fundamentally about the idea of treating a certain kind of attribute differently?

More practically, if the solution would offer an interface like:

attr1: {
  type: 'number',
  history: false, // doesn't persist
},
attr2: {
  type: 'string',
},

would that change anything? Because I don't care about this particular interface either, I wanted to see what the smallest change could look like. But the problem stands that some sort of state is needed for blocks that:

• can easily be passed when creating a new block (from conversion, etc.)
• is not committed to the undo history, i.e. so that it can never be accidentally replayed

Let me know, so I can either refactor this into something more palatable, or nix it in favour of something else.

[aduth]

I think the distaste comes from a perspective of looking to the attributes as the minimal set of values necessary to describe a block in its saved state.

From the parent issue, this reads as problematic in and of itself:

Since the temporary states are encoded in the attributes themselves of the blocks

Temporary states should not be in the attributes. For what reason is the actual component state of the block's edit component insufficient?

I'd be okay with considering a change of attribute value as not being considered for history, but it's still a consideration of the specific implementation of how the edit component works at runtime, not part of the block type attributes definition. In other words, taking your example, I'd be more okay with setAttributes( { attr1: 5 }, { history: false } );

[mcsf]

Temporary states should not be in the attributes. For what reason is the actual component state of the block's edit component insufficient?

The issue lies in tracking the circumstances of a block’s creation: when it’s the result of a transformation — in the broader sense — it can be incomplete. If we’re loading an existing post, all blocks therein are complete. If an existing block is being manipulated by the user, it can temporarily hold state in edit, then commit the proper data to its attributes. In contrast, if dragging an image file onto the editor canvas, pasting an embeddable URL, etc., the current Transformation API dictates that these events can be mapped to a block type, but the block type’s “runtime” (edit) ends up responsible for working out any missing values.

Given this, the sane way to work with attribute resolution should be to just wait inside a transformation’s callback until all the data requests have been resolved and only then instantiate a finalised block. The problem there, of course, is that we lose the ability to indicate progress (typically an incomplete block appears and will pulsate as data is loaded, etc.).

Thus, our options would be:

• (I'd run away from this option, but it's worth describing it.) Let the transformation know and do a lot more: parse its inputs; instantiate a block with missing attributes so that the block can display progress (e.g. an Image block instantiated with a blob file will be displaying a blinking image); take care of the data resolution (either up- or downloading state); once resolved, replace the previously instantiated block with a complete one. This not only implies a lot of changes in the block API, it also requires offloading onto it what has so far been handled directly in blocks' edit methods (e.g. loading images). Finally, it would easily break if a user manipulates the temporary block instance and we don't carry those changes into the final block instance properly.

• Retain the overall architecture, but support signalling to a new block that it should be aware of some missing data or be aware of some circumstances leading to its creation. That's where either silent attributes come in, or something like:

I'd be more okay with setAttributes( { attr1: 5 }, { history: false } );

which is similar to my initial explorations (the more complex one using withHistoryAmend and the simpler, which was entirely equivalent).

[mcsf]

Finally, the other possibility is to extend not setAttributes, but createBlock. For instance:

// createBlock( name,         attributes, innerBlocks, initialEditState )
   createBlock( 'core/image', {},         [],          { blobUrl: createBlobURL( file ) } )

(where file is provided by the Image block's files transform)

This would lead to an empty (per attributes = {} in the call) Image block being created, but the interface would accommodate a passing of initialEditState to Image's edit method.

@aduth, any advice on the best direction, between those described?

[aduth]

Given this, the sane way to work with attribute resolution should be to just wait inside a transformation’s callback until all the data requests have been resolved and only then instantiate a finalised block.

Yeah, one of my initial impressions might be that we add framework-level support for transformations to return a Promise which, until resolved, displays some progress state. The issues with this, as you allude to, are that (a) we'd need something generic enough to show and (b) it's inconsistent / redundant with a block's own implementation of its progress states.

More thoughts to come...

[aduth]

Relevant issues for problematic consequences of temporary states in attributes: #5936, #11565

There could be some argument to make about supporting more formally this idea of states under which a block holds some temporary data, which in turn prevents a save from occurring.

[aduth]

Could this be something on an attribute definition, i.e. transient: true, which is a trigger both for (a) not creating a history entry and (b) not allowing save to occur until unset?

[aduth]

but the interface would accommodate a passing of initialEditState to Image's edit method.

I like this proposal in that it preserves the idea of a two-way mapping between attributes and the persisted save form, but I worry it would require duplicating many of the mechanisms we've created around attributes (setAttributes, etc).

[mcsf]

Could this be something on an attribute definition, i.e. transient: true, which is a trigger both for (a) not creating a history entry and (b) not allowing save to occur until unset?

Is this the same as the example in #11504 (comment) ? If so, we can set out to build it.

(b) not allowing save to occur until unset?

Hadn't thought about that, sounds interesting.

[aduth]

Is this the same as the example in #11504 (comment) ? If so, we can set out to build it.

Effectively, yes, though less specific to the case of history and more to this idea of temporary states.

[aduth]

Any reason not to go with that approach in serializer.js?

Doesn't that then not do anything for us so far as resolving #5936, #11565 ?

If we're saying transients are needed for cases in which the final state isn't yet known and we're contemplating a case where the resolution of that final state fails... what then do we save for the failed thing?

[aduth]

Your branch is probably stale. Most functions accept blockTypeOrName as argument.

See: #11490

[aduth]

Would truthy if ( transient) { suffice?

[aduth]

What's this all about then?

[mcsf]

[emphasis mine in both quotes]

Doesn't that then not do anything for us so far as resolving #5936, #11565 ?

Well, from the issue in question:

An autosave should not take place if there are pending uploads, otherwise the transient media information may be serialized permanently to the post.

As this branch stands, serialisation of transients won't happen. What can happen is that a block gets saved with partial information (e.g. missing an unresolved image), but at least it's not saved in a broken state of trying to fetch something.

An autosave should not take place if there are pending uploads,

This makes some sense, but I worry about blocking users out of saving due to unforeseen failures — essentially, this gives a buggy block type the power to sabotage saving of an entire post. Don't you think so? (Coming from Calypso, where in the past I experienced breakage of the saving action due to an intermittent network connection, I'd do my best not to mess with saving too much.)

[aduth]

I worry about blocking users out of saving due to unforeseen failures — essentially, this gives a buggy block type the power to sabotage saving of an entire post. Don't you think so?

I agree. I think also the alternative to save partial data is in its own way a broken experience. This seems one of those things which falls into the realm of the machine being unable to make a decision, and where we might even need to consider surfacing it to the user. For example, I could imagine a prompt about "The Image block has operations pending. Save anyways? [ Yes ] [ No ]", maybe highlighting the block in question.

[aduth]

What's the issue with keeping it in state? Could it be enough to fall under the concept of withHistory's "ignore" behavior?

Related prior art: caed8bf (#9403)

[mcsf]

As DM'd, this is problematic due to how we now expect to instantiate blocks with createBlocks( { myNormalAttr, myTransientAttr } ).

[aduth]

Summarizing direct message conversation:

• The current proposal's serialize serves the purpose of treating transient as data which should never be restored in an undo, and should only exist in the present state of attributes.
• Much of my reluctance stems from a performance-related concern in the idea that we'd need to iterate all block's attributes for all blocks for all attribute updates dispatched to the store.
  • There is some shortcutting in place to relieve some of this worry.
• An option was considered in a combination of (a) partitioning incoming attributes updates to "fork" transients into their own, separate action update and (b) implementing an ignore option for the history enhancer.

[aduth]

My primetime brain energy was dedicated elsewhere today, and here I am fruitlessly trying to consider this one with what little remains 😄 My latest concern with the more recent ideas is that: We have a lot of ways that attributes get into state (any of the cases of byClientId basically).

I started then considering again: Okay, make “transients” a thing. And if we need it from the creation of the block, should it be part of the createBlock API? My concern there is that transients aren’t native to the idea of a block, they’re very much specific to the lifetime of an editor. But I suppose the same can be (and has been) said about edit, etc.

Then I considered: What about some one-off approach of return { ...createBlock(), whatever: true } and accessing that from the block implementation, since the data should just be shoved into state? It feels very fragile though, and bypasses any benefit we could get about making assumptions from the presence of transient data (prompting on save).

Ultimately I’d like some top-level state key where the transient data lives. This helps in a few ways: Ideally not needing strange heavy-handed forking / history behaviors for edits, since it’s not considered at all. Some foreseeable hasPendingOperations selector could be just Object.keys( state.transients ).length > 0. This could potentially work well enough as another object , the fourth argument of createBlock, and a new setTransients edit prop, if that’s a compromise we’re willing to make.

I'd worried that we'd need to recreate a lot of the machinery around attributes for this new transients objects, but upon some reflection I don't know that there's really as much overlap as I might have thought, aside from surface-level similarities in attributes + setAttributes vs. transients + setTransients.

I'll plan to circle back around to this once I've had some time to rest and reflect.

[gziolo]

2 unit tests fail:

[1] Test Suites: 1 failed, 1 skipped, 296 passed, 297 of 298 total
[1] Tests: 2 failed, 1 skipped, 2633 passed, 2636 total
[1] Snapshots: 1 failed, 160 passed, 161 total

They probably should be updated to reflect changes introduced.

[catehstn]

@aduth and @mcsf -- do you have an agreed on approach for this?
@mcsf -- do you think you are going to be able to return to this and update it ahead of 4.6?

[aduth]

To update, I'd iterated some on a separate branch toward the same end-goal here, exploring what it might look like to handle transient attribute updates from the block itself after the fact. The main hiccups involve issues around when and how to "flatten" history, mainly (a) the initial entry of the block having a temporary attribute value and (b) there being possible to be many states between the attribute value first being marked as transient and the transient state becoming completed (since an upload can take many actual seconds to complete).

The branch can be found here:

https://github.com/WordPress/gutenberg/compare/try/set-attributes-transient-option

It does reasonably well to avoid saving when an image upload is in-progress, but the aforementioned issues around history flattening become quite obvious in bugginess apparent when trying to Undo after an image upload had taken place.

It's also subject to the issue described at #12327, in that transient data is meant to live outside the history of a block, but is only relevant so long as a reference to that block exists.

[catehstn]

Thanks @aduth, are you suggesting we close this PR in favour of yours? Or is your suggestion that we need to break this down into: resolve #12327, address undo bugginess, and then evaluate what's next?

[mcsf]

are you suggesting we close this PR in favour of yours? Or is your suggestion that we need to break this down into: resolve #12327, address undo bugginess, and then evaluate what's next?

I just chatted with Andrew. We have no clear path forward yet, so I've opened and closed a PR from his branch, #12724, so that we can find it later, and I'll close this one as well. Meanwhile, the original issue, #8119, remains open.