Editor: Implement meta as custom source

[aduth]

Closes #16282
Related (alternative to): #16075

This pull request seeks to explore an implementation of custom sources.

Implementation Notes:

The implementation inherits a fair bit from #16075 (notably "last changes" tracking), but it tries to leverage a flow where sourced attributes are updated (as applicable) and applied prior to any blocks reset from the editor module. Furthermore, per discussion at #16075 (comment), they are implemented as standard store controls, where the benefit of this approach is in increased flexibility of the implementation by avoiding the need to determine a specific set of arguments which would apply for all potential custom source implementations. Instead, each implementation can simply yield controls (such as select from @wordpress/data-controls) to retrieve whichever data they depend upon.

It diverges from some alternative proposals (#16282 (comment)) due to:

• Not having well-defined "start" and "end" cycles from within the context of a block editor
• The difficulty and overhead in keeping redundant data in sync for "sourced" data, where this implementation can derive directly from post meta (rather than to carve a separate state which needs to be kept in sync with the post)
• In the alternative, not being able to eliminate selector-time source derivation (as of the changes here, getBlockAttributes returns a static reference)

As to the proposed "Block Sources API", I'm not very attached to the specific set of arguments currently passed to apply, applyAll, and update. The idea for applyAll came about as a result of a performance concern for repeated data access, but it is not strictly necessary (apply and update alone would be nicely complementing, though similarly we could explore to add an updateAll). I didn't seek to make this publicly-extensible for the moment, though it is designed to allow for it in the future after some internal validation of the API.

Remaining Tasks:

• Unit tests to be written, if implementation deemed acceptable.

Testing Instructions:

Verify that updating a block with an attribute sourced by a meta attribute reflects the update (it is restored after a save, and it applies to all other blocks which source from the same meta).

As an example, I think this demo plugin should still work: https://gist.github.com/pento/19b35d621709042fc899e394a9387a54

Caveats:

• Block validation fails if the meta value is persisted into post content (Validation fails if attribute is persisted to post content & meta. #4989). I aim to tackle this separately, where validation occurs as a separated process from the default blocks parse behavior and runs only after the meta values are applied.
• There seems to be an issue where other blocks sourced from the same meta do not re-render immediately. Clicking to select those other blocks will trigger the render and reflect the update. I suspect there may be an issue with either the editor's async rendering, or how we choose to memoize these blocks' components rendering.

[aduth]

I'm not sure if it's an issue with Babel transpilation or a distinct behavior for how named exports "objects" are iterated, but I would have preferred to use a more concise for ( const source of sources ) { syntax here, yet I was encountering runtime errors ¯\_(ツ)_/¯

[epiqueras]

Objects are not Iterable. You would need to for of Object.keys/values/entries/etc(sources) or export an array of sources.

[epiqueras]

This would be a good place to call an updateAll if it exists.

[aduth]

Objects are not Iterable. You would need to for of Object.keys/values/entries/etc(sources) or export an array of sources.

Oh! That seems obvious in retrospect. I guess for ( const source of Object.values( sources ) ) ) { might be what I'm looking for here then.

[youknowriad]

Very first thought :) Seeing this made me wonder about an idea. Not sure yet how valuable it is or if it will allow us to improve things. but this callback could serve as a way to make the updates the blocks and "return" them.

I didn't read the PR completely but If I'm not mistaken this callback forces the caller to "set blocks" twice. When this callback is called and when onInput is called too?

(I might be completely wrong here, as I didn't dive yet)

[aduth]

I didn't read the PR completely but If I'm not mistaken this callback forces the caller to "set blocks" twice. When this callback is called and when onInput is called too?

No, because onBlockAttributesChange isn't actually applying anything to the blocks, it's only triggering the side effect (updating the post). As you mention, it's the subsequent onChange / onInput which calls resetBlocks, at which point those values are reflected in all impacted blocks.

[aduth]

No, because onBlockAttributesChange isn't actually applying anything to the blocks, it's only triggering the side effect (updating the post). As you mention, it's the subsequent onChange / onInput which calls resetBlocks, at which point those values are reflected in all impacted blocks.

An unfortunate consequence of this behavior: Since we rely on the subsequent resetBlocks and there's nothing about editPost itself which immediately triggers an application of sourced attribute values, if someone were to call editPost directly with updated meta values, the current implementation would not immediately update blocks which derive from this meta.

[epiqueras]

Can we make this part of the onChange/onInput actions instead of adding another action?

[epiqueras]

An unfortunate consequence of this behavior: Since we rely on the subsequent resetBlocks and there's nothing about editPost itself which immediately triggers an application of sourced attribute values, if someone were to call editPost directly with updated meta values, the current implementation would not immediately update blocks which derive from this meta.

We need a way for custom sources to subscribe to events/actions that should re-apply their values.

[youknowriad]

❤️ I love this README :)

[aduth]

Seeing this made me wonder about an idea. Not sure yet how valuable it is or if it will allow us to improve things. but this callback could serve as a way to make the updates the blocks and "return" them.

This prompted me to consider that we likely have a problem here with how the BlockEditorProvider considers its value as canonical, and upon an "outbound" sync will ignore the value provided in the next render. This could perhaps explain the issues with other meta blocks not being updated immediately. Your idea here could work at least for the one block being updated, but doesn't account for other meta blocks which source from the same meta property.

In another of my experimental branches, I had considered whether it would be enough that the BlockEditorProvider ensure the next value it receives in a sync matches what it had expected based on what was sent. We could use this in the implementation here, then only "apply" values to blocks if they differ from what was sent by the block editor, thus triggering the BlockEditorProvider to reset its blocks based on what was applied from the editor.

Admittedly, this is one thing the alternative considered proposal might handle well, at least so far as the block editor doesn't need to become aware of this new value resulting from a sync, since it retains the role of being the canonical source of values.

[aduth]

In thinking again about the "Source API" here, and partly with regard to my previous comment at least so far as the added complexity in avoiding to mutate (and only create new references) for blocks "applied", I wonder if we could eliminate applyAll, and align what's proposed here as align closer to the update interface.

In other words:

• update( attributeSchema, value )
• apply( attributeSchema )

This way, the framework can decide whether the value from apply differs from what's expected, cloning the blocks array as necessary.

The downside here is that we revert back to a previously-mentioned potential performance concern. I think this can be mitigated by the fact we only run apply when known to be a block of a given source. Furthermore, this only happens at specific intervals (block updates) that it's likely not to become a bottleneck.

[aduth]

From #16402 (comment):

An unfortunate consequence of this behavior: Since we rely on the subsequent resetBlocks and there's nothing about editPost itself which immediately triggers an application of sourced attribute values, if someone were to call editPost directly with updated meta values, the current implementation would not immediately update blocks which derive from this meta.

We could solve this in a similar way to #16075 by having built-in awareness to editPost that sourced attributes need to be updated, but this breaks down the isolation of a source implementation, lessening the usefulness of this as a generic or reusable interface.

Considering how it might be made to be generic, the sources would need some way to subscribe to the store, and in the case of meta, receive dependent data upon whose changing should cause a re-application of sourced data on blocks.

[epiqueras]

I am really liking this approach. Props on coming up with such a clean way to introduce it. 👏

I think the only unsolved problem is:

We need a way for custom sources to subscribe to events/actions that should re-apply their values.

So that things like direct editPost calls don't throw things out of sync. Maybe custom sources can export something like:

export const reApplyOn = [ 'EDIT_POST' ]

[epiqueras]

...change is one which is not an extension of a composed input...

This is a bit hard to grasp. Maybe an example would help.

[epiqueras]

Any sequence of updates to the same block attribute are treated as non-persistent, except for the first.

Why is this desired? If I edit a color twice, the second edit wouldn't persist?

[aduth]

Why is this desired? If I edit a color twice, the second edit wouldn't persist?

I think the choice of "persistent" as the term here can be potentially misleading. The intent was for it to be akin to the distinction between input and change events in the DOM API:

The input event is fired every time the value of the element changes. This is unlike the change event, which only fires when the value is committed, such as by pressing the enter key, selecting a value from a list of options, and the like.

https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/input_event
https://developer.mozilla.org/en-US/docs/Web/API/HTMLElement/change_event

In the context of the editor, the use-case is for Undo levels. You don't want Cmd+Z to undo paragraph changes one character at a time, but rather as units (in our case, reflected as sequences of updates to the same block attribute).

When I was first thinking about this documentation, I had in mind to explicitly mention how it's used for Undo/Redo in the editor, but neglected to write it when I'd returned to my computer 😄

[epiqueras]

Thanks, that's much clearer now. I remember running into that code, but forgot about it when reading this.

[epiqueras]

The component will try to use all of these props (except for "children") and throw errors. We should mark them as required.

[aduth]

The component will try to use all of these props (except for "children") and throw errors. We should mark them as required.

I was actually thinking we should make them optional. At least in the case of onInput and onChange, it's quite likely most usage will only care to provide one or the other.

I agree that the documentation here is not accurate per the current implementation. I didn't want to document an undesirable requirement, however. Maybe we should seek to make it optional as a separate task to this pull request.

[epiqueras]

Makes sense 👍

[epiqueras]

Can we make this part of the onChange/onInput actions instead of adding another action?

[epiqueras]

Reducer return an
updated attributes values

Typos, maybe?

[epiqueras]

An unfortunate consequence of this behavior: Since we rely on the subsequent resetBlocks and there's nothing about editPost itself which immediately triggers an application of sourced attribute values, if someone were to call editPost directly with updated meta values, the current implementation would not immediately update blocks which derive from this meta.

We need a way for custom sources to subscribe to events/actions that should re-apply their values.

[epiqueras]

Nice use of generator delegation!

[epiqueras]

Objects are not Iterable. You would need to for of Object.keys/values/entries/etc(sources) or export an array of sources.

[epiqueras]

This would be a good place to call an updateAll if it exists.

[epiqueras]

This is awesome! 🚀

[aduth]

Can we make this part of the onChange/onInput actions instead of adding another action?

Maybe. @youknowriad mentioned a specific concern to me that this might help address; namely, because we currently have the source updating and reset and separate actions, there's technically a state which is out of sync between those two actions. This would not be directly reachable by any user interaction, but is not desirable from purely a data perspective.

He'd incorporated the block updates into the reset step in his work of #16075. The main concern I have with this is that we expand the responsibilities of a reset action (normally just a setter) to become more aware of the cause of those changes, which to me is still a two-stage process, except bundled into a single action.

[epiqueras]

In another of my experimental branches, I had considered whether it would be enough that the BlockEditorProvider ensure the next value it receives in a sync matches what it had expected based on what was sent. We could use this in the implementation here, then only "apply" values to blocks if they differ from what was sent by the block editor, thus triggering the BlockEditorProvider to reset its blocks based on what was applied from the editor.

That looks like it would fix this issue.

In thinking again about the "Source API" here, and partly with regard to my previous comment at least so far as the added complexity in avoiding to mutate (and only create new references) for blocks "applied", I wonder if we could eliminate applyAll, and align what's proposed here as align closer to the update interface.

Yeah that makes more sense now that we need that equality check in the block editor provider componentDidUpdate.

Considering how it might be made to be generic, the sources would need some way to subscribe to the store, and in the case of meta, receive dependent data upon whose changing should cause a re-application of sourced data on blocks.

Something along the lines of:

export const reApplyOn = [ 'EDIT_POST' ]

// Also:
export const reApplyOn = { 'EDIT_POST': ( state, action ) => true || false }

[epiqueras]

Maybe. @youknowriad mentioned a specific concern to me that this might help address; namely, because we currently have the source updating and reset and separate actions, there's technically a state which is out of sync between those two actions. This would not be directly reachable by any user interaction, but is not desirable from purely a data perspective.

Yeah, having it in one action would be better.

[aduth]

In speaking directly with @epiqueras , we mentioned a few ideas for revisions.

Per prior comments #16402 (comment), #16402 (comment), and #16402 (review), we need some way to subscribe to changes. I think this could also be used as a way to provide "shared" resources in applying attributes to the blocks, which is currently what the proposed applyAll was meant to address. By operating in retrieving singular block attribute values, we can also move some of the complexities of managing blocks references updates to the framework level.

Sample interface:

export function* getDependencies() {}
export function* apply( attributeSchema, dependencies ) {}
export function* update( attributeSchema, value ) {}

Meta example:

export function* getDependencies() {
	return { meta: yield select( 'core/editor', 'getEditedPostAttribute', 'meta' ) };
}

export function apply( attributeSchema, { meta } ) {
	return meta[ attributeSchema.meta ];
}

export function* update( attributeSchema, value ) {
	yield editPost( { meta: { [ attributeSchema.meta ]: value } } );
}

Per comments #16402 (comment) and #16402 (comment), we may want to explore incorporating awareness of the specific block updates which contributed to resetEditorBlocks as part of the same action. We can move what we have today in the logic which calls to onBlockAttributesChange to be as part of the onInput and onChange handlers, comparing whether the last block updates have been changed. This moves much more in the direction of @youknowriad's original pull request at #16075.

Per comment #16402 (comment), we should incorporate this fix to assure that when the BlockEditorProvider performs an "outbound sync", the value it receives next is what it assumes it should be, and reset if it is different (in the case of meta updates, we will produce a new value reference if there are multiple meta blocks which need to be updated in response to the change).

[epiqueras]

From exploring with @youknowriad how this would work with a post-content block that uses inner blocks. We think we need a special case of the API outlined above.

registerBlockType settings should accept an innerBlocksSource property that allows you to set a custom source.

Here is what the implementation for post-content could look like:

export function* getDependencies() {
    return yield select( 'core/editor', 'getEditedPostAttribute', 'content' );
}

export function apply( attributeSchema,  content ) {
    return parse(content);
}

export function* update( attributeSchema, value ) {
    yield editPost( { content: serialize(value) );
}

Differences to custom sources used for attributes:

• apply's return value should replace the block's inner blocks.
• Editing inner blocks does not set attributes on the parent block so update here would never fire. We probably don't want it to fire on every edit anyways, because serializing is expensive. So, we need update to be called just once at the start of savePost.

[aduth]

I've pushed up some revisions:

• Since Adds a cache key to the blocks reducer in order to optimize the getBlock selector #16407 was merged with specific handling for updates to meta properties, this pull request now removes that handling, as it seeks to make these unnecessary.
• Dependencies updates are now managed by a special action generator which effectively subscribes to store changes. It was done this way to keep sourcing handling colocated within the store, and to ensure that the sources dependencies getter could be implemented as a generator to make use of data controls.
  • To treat it as a proper subscription with unsubscribe, a new tearDown action was added, which controls the existing isReady state flag.
• From 5adb8ab (an alternate branch try/editor-custom-sources), we now ensure that the BlockEditorProvider only ignores the next value after an outbound sync if the value matches what it expected to be synced. This ensures that a full reset would occur if a meta block being updated should cause cascading effects to other meta blocks using the same property.

As can be seen in the code, the revisions achieve the proposed interface mentioned in my previous comment.

What still needs work:

• resetEditorBlocks is called twice because. One is caused by the editPost coming from a block's attribute update, the other from the BlockEditorProvider's default onInput / onChange callbacks. As mentioned in my previous comment, we should try to consolidate this to keep a single resetEditorBlocks action which is aware of how to call the source update functions and reconcile to avoid duplicate resets.
• There's a bit of wastefulness in the current implementation for how dependencies are passed to the source apply function. Ideally this can reuse the same cache store as in the "subscribe" behavior mentioned above.

[epiqueras]

Nice work!

I think I have an idea that can kill both those remaining birds with one stone:

export const { resetEditorBlocks, subscribeSources } = ( () => {
	const lastDependencies = new WeakMap(); // Shared cache.
	const updateDependencies = function*() {}; // Returns true if they have changed, and false if not.

	return {
		*resetEditorBlocks( blocks, options = {} ) {
			if ( aCustomSourcedAttributedHasChanged ) {
				callUpdates();

				// This makes sure `subscribeSources` doesn't call back here unless something directly updates a source.
				updateDependencies();
			}

			return {
				type: 'RESET_EDITOR_BLOCKS',
				// Reuses cache!
				blocks: yield* getBlocksWithSourcedAttributes( blocks, lastDependencies ),
				// Integrates with undo logic!
				shouldCreateUndoLevel: options.__unstableShouldCreateUndoLevel !== false,
			};
		},

		// Won't call `resetEditorBlocks` if `resetEditorBlocks` already updated dependencies.
		*subscribeSources() {},
	};
} )();

This approach shares the cache, makes sure we only call resetEditorBlocks once, and we get integration with undo logic for free.

[aduth]

@epiqueras Good idea. It seems to illustrate: Maybe we shouldn't bother with the IIFE at all, and just create a variable(s) in the shared top-level scope?

This hints to another problem, however: Dependencies should be tracked unique per each registry. I have a few ideas for how we might incorporate this, but both add some further complexity to the solution:

1. Change our dependency tracking to account per registry. For example, lastDependencies.get( source ) becomes lastDependencies.get( registry ).get( source ).
   • We would need some way to get a reference to the registry from within the action, likely via another control GET_REGISTRY: createRegistryControl( ( registry ) => () => registry )
2. Manage last dependencies in state, via a reducer + selector combination

If we hope for this sources behavior to become reusable at some point, these add a fair bit of overhead to how it would need to be implemented. At this point though, I think these goals could be optimized in future refactoring.

[epiqueras]

Good catch! I think we can implement your first suggestion easily by making AWAIT_NEXT_STATE_CHANGE resolve to the registry, but your second suggestion will scale to more use cases and fits our patterns better. We can use the same part of the store we use for last attribute changes.

…

On Mon, Jul 8, 2019 at 7:11 AM Andrew Duthie ***@***.***> wrote: @epiqueras <https://github.com/epiqueras> Good idea. It seems to illustrate: Maybe we shouldn't bother with the IIFE at all, and just create a variable(s) in the shared top-level scope? This hints to another problem, however: Dependencies should be tracked unique per each registry. I have a few ideas for how we might incorporate this, but both add some further complexity to the solution: 1. Change our dependency tracking to account *per registry*. For example, lastDependencies.get( source ) becomes lastDependencies.get( registry ).get( source ). - We would need some way to get a reference to the registry from within the action, likely via another control GET_REGISTRY: createRegistryControl( ( registry ) => () => registry ) 2. Manage last dependencies in state, via a reducer + selector combination If we hope for this sources behavior to become reusable at some point, these add a fair bit of overhead to how it would need to be implemented. At this point though, I think these goals could be optimized in future refactoring. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#16402?email_source=notifications&email_token=AESFA2AZRSK7MPR5KWCZ3FLP6MVFPA5CNFSM4H47X6S2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZM4KHQ#issuecomment-509199646>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AESFA2A4OJ3JMQCLRT4FR5DP6MVFPANCNFSM4H47X6SQ> .

[aduth]

I might run into it again in the future; I don't recall why I needed to reorder resetEditorBlocks, but it causes an issue where a new post will prompt about unsaved changes when SCRIPT_DEBUG is true, due to a fragile guarantee the current order establishes that causes "dirtiness" state to be unset by the setupEditorState action. This incidentally resolves the fact we dispatch this setupEditor action twice in an editing session, since it happens in constructor (rather than componentDidMount) and React helpfully detects side-effects via double-invoking intended non-side-effect lifecycle functions.

[aduth]

Oh, I remember now: We want the sourced attributes to be applied in resetEditorBlocks, which requires that we set the post state first (so that post meta can be read).

In that case, we might need to address the underlying issue with the EditorProvider lifecycle, or endure the prompts in SCRIPT_DEBUG (preferably not).

[epiqueras]

From our conversation:

So resetEditorBlocks sets the flag to true if the blocks are different.
setupEditorState sets it to false again.

Basically it only worked because setupEditorState was called as last, so it reset the dirty flag

The dirtying was actually pretty simple if we leave things as they are, and just call resetPost before resetEditorBlocks (so that meta values are available for applying)

[aduth]

At some point, we should also look to refactor this so that the editor module doesn't need to store a copy of the "current" post, but instead calls to the @wordpress/core-data module to apply the edits on the canonical post object.

wp.data.select( 'core' ).getEntityRecord( 'postType', 'post', wp.data.select( 'core/editor' ).getCurrentPostId() );

[epiqueras]

getDependencies should also take the schema so we can do things like:

export function* getDependencies( schema ) {
	return yield select( 'core/editor', 'getEditedPostAttribute', schema.postAttribute );
}

For a postAttribute source.

[epiqueras]

Even better:

export function* getDependencies( { attribute, property } ) {
	const dependency = yield select( 'core/editor', 'getEditedPostAttribute', attribute );
	return property ? _.get( dependency, property ) : dependency;
}

For a title source:

"attributes": {
		"title": {
			"type": "string",
			"source": "post",
			"attribute": "title"
		}
	}

For meta:

"attributes": {
		"something": {
			"type": "string",
			"source": "post",
			"attribute": "meta",
			"property": "something"
		}
	}

[aduth]

getDependencies should also take the schema so we can do things like:

The problem with this is that it can be distinct by block type, rather than strictly for all blocks / block types of a given source. The specific meta property from which attribute values are sourced can vary by block type.

[aduth]

I've rebased to resolve conflicts introduced by #16184. I started squashing a few commits to clean up the history, but it got a bit unwieldy, so I left it more-or-less as it was (shouldn't matter much anyways since I'll Squash and Merge). I added a new commit 468f908 which removes an earlier approach to retrieving the registry in the resolution of subscribeSources awaitNextStateChange, since there was a later addition of a getRegistry control anyways.

I'm planning to merge this shortly.

As follow-up tasks considered in this pull request:

• Resolve blocks validation issues with custom sources (Validation fails if attribute is persisted to post content & meta. #4989). This pull request does not improve this situation. I anticipate the resolution here involves deferring blocks validation until after a first pass to apply source values.
• Mentioned in passing at Editor: Implement meta as custom source #16402 (comment), there's a potential refactor with getCurrentPost. It's not very relevant for custom sources, but worth exploring separately. I've created a new issue at Editor: Refactor currentPost state to use core-data package #16520 to track this.
• In iterations, consider reorganization of the functions / logic to better isolate the complexity
• Start exploring other custom sources (e.g. post properties in Editor: Operate on template CPT posts and add a default template with post title and content blocks. #16485)

[aduth]

Thanks @epiqueras and @youknowriad for your detailed feedback and suggestions here!

[mcsf]

This is really great work that I've only now caught up with. Left some minor notes.

[mcsf]

Is the if condition expected to evaluate differently across for iterations? If not, we could place it as a condition for stepping into the for loop.

[mcsf]

Can probably factor out this assignment and the optional WeakMap initialisation that comes before it.

[aduth]

Thanks for reviewing, @mcsf! Both are valid points. See #16839.