Simplify meta attribute sources and attempt to prepare a generalization of custom sources

[youknowriad]

This is still WIP (I meant to create a draft PR but clicked the wrong button) :)

[youknowriad]

I'm not satisfied with this trick (this value being an array) but it's the only way I found right now to fix all the use-cases which are:

• We need to avoid calling resetBlocks if the new "value" prop received is the same as the one already in the block-editor store (to avoid resetting the "undo" levels). (undo e2e test failing)
• Sometimes calling some changes like inserting a block with a template (columns block for instance) result in two onChange calls being performed synchronously which will eventually trigger two componentDidUpdate calls but eventually the first componentDidUpdate will call resetBlocks if we only compare against the latest blocks value in the block-editor state causing the editor to reset to the previous blocks value (and not the last one). (columns e2e test failing)

The fix here is a bit fragile though, it basically says: don't call "resetBlocks" unless the new prop received is not one of the last persisted blocks value passed to onChange.

[aduth]

I'm a bit concerned about this idea of tracking the "last block update", partly because it adds a fair amount to the implementation to track (and some more cumbersome comparison logic), partly because it locks us into considering updates as only ever applying to one block at a time.

Is this synchronization behavior not something we could be doing as a result of BlockEditorProvider calling its onChange or onInput, assuming that this is a "new" value of blocks, and iterating over each to see if any meta updates have occurred? Is the worry here one of performance in iterating the blocks? Which is granted, but maybe some ways to mitigate like tracking which blocks would have meta attributes in the first place (similar idea as in #12555).

[aduth]

Oh, this refactor makes me quite happy to see 😄 This should serve as a nice optimization.

[youknowriad]

I was a bit disappointed to notice that it doesn't change a lot in terms for performance impact. (The cache bust is the bottleneck right now)

[aduth]

It kinda makes me wonder why we're authoring these sources as if they're some generic interface, when ultimately we call it very explicitly and with a very use-case-specific set of arguments.

[youknowriad]

Agreed, right now it's not the perform generic API, it's a first attempt. I think ideally I'd like something composed of two APIs: one API to the list of changes (edits) to apply to the editable elements (post object and ultimately, template object as well)

const edits = applyBlockAttributesEdits( attributesConfig, block.attributes );
// edits here is somehting like { post: { meta: { key: value } }

the second API is how to update block attributes based on the editable objects (post and tempalte)

const block = fillSourceAttributes( block, post, template )

Still not perfect but you get the idea.

[aduth]

A thought: What if we treat the source synchronization just as any other store control?

That way, they could yield using existing select controls to retrieve whatever store data they need (like here, the post meta).

for ( let i = 0; i < blocks.length; i++ ) {
	for ( let source of sources ) {
		blocks[ i ] = yield* source.sync( blocks[ i ] );
	}
}

// ...

function* sync( block ) {
	const meta = yield select( 'core/editor', 'getEditedPostAttribute', 'meta' );

	// ...
}

[youknowriad]

This does make sense, we'll be calling the same selector over and over for each block though, so I wonder about performance.

[aduth]

This does make sense, we'll be calling the same selector over and over for each block though, so I wonder about performance.

I was thinking about it some more afterward, and at least as far as the looping goes, I was thinking the sources could be defined as named exports from a folder corresponding to the attribute source:

// src/store/sources/index.js
export { meta } from './meta';

// src/store/actions.js
import * as sources from './sources';

for ( let i = 0; i < blocks.length; i++ ) {
	const block = blocks[ i ];
	for ( const schema of getBlockType( block.name ).attributes ) {
		if ( sources[ schema.type ] ) {
			blocks[ i ] = yield* source.sync( blocks[ i ] );
		}
	}
}

To your point, I'd wonder if we could memoize (per sync pass) results from some source "common values" which aren't block specific (like meta).

const sourceValues = {};
for ( let i = 0; i < blocks.length; i++ ) {
	const block = blocks[ i ];
	for ( const schema of getBlockType( block.name ).attributes ) {
		if ( ! sources[ schema.type ] ) {
			continue;
		}

		if ( ! sourceValues.hasOwnProperty( schema.type ) ) {
			sourceValues[ schema.type ] = yield* source.getValues();
		}

		blocks[ i ] = yield* source.sync( blocks[ i ], sourceValues[ schema.type ] );
	}
}

In the above, there's still a potential performance concern from iterating every attribute for every block. Something like #16316 benefits from the fact that this is consolidated into the initial parse phase.

[youknowriad]

yeah, let's try that maybe? this API can remain private for a while.

[aduth]

We can change this to @yields, which helps preserve the documentation.

[youknowriad]

I'm a bit concerned about this idea of tracking the "last block update", partly because it adds a fair amount to the implementation to track (and some more cumbersome comparison logic), partly because it locks us into considering updates as only ever applying to one block at a time.

Conceptually speaking, it's not necessary. I had a concern in terms of performance (having to go throught all the blocks on each update). It could be simplified, we can for instance keep just the "clientIds" of the updated blocks instead of the changes themselves.

At the moment, meta attributes is not the "default" which means even without it, it's probably still performant as we'd ignore the looping entirely but I'm thinking the more we'd go into site building, we'd need more "meta-like" sources and it could become more of an issue.

Is this synchronization behavior not something we could be doing as a result of BlockEditorProvider calling its onChange or onInput, assuming that this is a "new" value of blocks, and iterating over each to see if any meta updates have occurred?

So the performance part is addressed above but the reason it's not in BlockEditorProvider is more "conceptual" in considering the BlockEditorProvider as something that updates blocks and block attributes but doesn't care how these attributes are originally sourced, propagated leaving this responsibility to the editor module instead.

[aduth]

Did you mean to return null here, as it would cause any other action than the ones above to reset state? I guess if the idea is that it's only effective for the single action which updates the block?

[youknowriad]

yes, I didn't give it too much thoughts but yeah the idea is that we don't care about the value after. I don't think it would harm if we return "state" but I wonder if we'd need to "reset" the value in some actions I might have missed.

[youknowriad]

Tbh, this "last changes" thing was consider very experimental. If we follow the approach of this PR, I wouldn't mind if we do a full sync on update. Maybe the performance impact is small? we should measure.

[aduth]

I'm wondering if we need to account for these actions. It seems that interpreting something like a block replacement to trigger meta updates might not always be expected (though conversely, maybe it is).

[youknowriad]

Well, I don't know. I agree that it's less important in the way we use "RESET_BLOCKS" in Core but there's nothing stopping third-party plugins from using the actions right.

[youknowriad]

Closing this for now, as it's now largely covered (and more) in #16402