Editor: Leverage core data entities for edits, history, change detection #16761

aduth · 2019-07-25T20:38:42Z

This pull request seeks to begin exploring a fairly significant refactor to minimize the responsibilities of the editor package by delegating edits tracking to core-data "entities" behavior. Doing so should allow for improved support of edits tracking to multiple entities in the same editor session (multiple posts and non-post entities such as site options or even third-party APIs). At that point, the save mechanics of the existing post editor serve more as an intent to persist (sync) all outstanding edits of any entity.

Work in Progress: Basic behaviors work as expected, but there are many issues yet to be resolved, particularly around post "dirtying" and undo history. It is entirely expected that many of the proposed changes will be extracted into their own pull requests to simplify the eventual "final" review-ready version of this pull request.

Implementation Notes:

This exploration required fleshing out the core-data module to add additional support for:

Edits tracking
Undo history

Existing functionality for these features in the editor module is effectively soft-deprecated. The selectors exist today and are not marked as deprecated, but largely only because the larger effort to port existing editor components to use equivalent core-data selectors isn't considered as part of this branch.

In enhancing the core-data module, it was found to lack some support for more advanced entities tracking that had been implemented in the editor module:

When receiving an entity, we should try to reuse value references when unchanged, since many components / custom sources define dependencies on this data, and should only be updated when the values are meaningfully different.

To represent the editor's current treatment of blocks state as the temporal, preferred representation of content, the edits implementation supports semi-hidden non-enumerable properties. Using Object.defineProperty, both blocks and content are assigned as edits of a post, but are only computed or considered for dirtiness, post history, or save payloads as appropriate. For more information, see the inline code comments of the editor's resetEditorBlocks action. With this change, the editor module no longer tracks in state full copies of the post and its content, nor the current blocks value.

General summary of pending work:

Extract out commits to separate pull requests when applicable
Handle autosave
There needs to be some deeper consideration of performance impact. I do not expect the performance of the current state of this branch is anywhere close to that of master.
Documentation
Unit tests
Consider how to represent the editor's initialEdits prop in this implementation. This is currently addressed for "Auto Draft" title with server-side filtering. I intend that these changes are valuable irrespective what comes of initialEdits, but we should consider how to continue supporting these initial edits values. One use case is the Demo page, which pre-fills edited content but the content is not intended to be dirtying.
There are a number of inline "TODO" code comments which should be addressed and removed.

youknowriad

This is just a few quick questions but I find it pretty telling that it's more code removed than added (even if it lacks documentation...).

youknowriad · 2019-07-25T21:59:03Z

packages/core-data/src/queried-data/reducer.js

+
+	const result = {};
+	for ( const key in nextItem ) {
+		if ( isEqual( nextItem[ key ], item[ key ] ) ) {


Did we do this previously for "currentPost"? Do you recall why? any concern about perf?

Did we do this previously for "currentPost"?

Yes, sort of. I believe the way it works currently is that we (for better or worse) don't actually reset (RESET_POST) the post, but only ever update (UPDATE_POST) the persisted values, where this exact behavior to replace the persisted values only if deeply equal:

gutenberg/packages/editor/src/store/reducer.js

Lines 177 to 191 in 32b7b34

case 'UPDATE_POST':

case 'RESET_POST':

const getCanonicalValue = action.type === 'UPDATE_POST' ?

( key ) => action.edits[ key ] :

( key ) => getPostRawValue( action.post[ key ] );

return reduce( state, ( result, value, key ) => {

if ( ! isEqual( value, getCanonicalValue( key ) ) ) {

return result;

}

result = getMutateSafeObject( state, result );

delete result[ key ];

return result;

}, state );

Do you recall why?

Why we did, or didn't? At least for the purposes of this pull request, it was important to ensure that dependencies don't fire as having been changed when the values are effectively the same.

For example, the meta custom source:

gutenberg/packages/editor/src/store/block-sources/meta.js

Line 23 in 32b7b34

meta: yield select( 'core/editor', 'getEditedPostAttribute', 'meta' ),

gutenberg/packages/editor/src/store/actions.js

Lines 235 to 241 in 32b7b34

if ( ! isShallowEqual( dependencies, lastDependencies ) ) {

lastBlockSourceDependencies.set( source, dependencies );

// Allow the loop to continue in order to assign latest

// dependencies values, but mark for reset.

reset = true;

}

any concern about perf?

Not really. This only happens on full reset (at load and after a save). And even then, most entities are pretty flat, so deep equality doesn't make much of a difference.

youknowriad · 2019-07-25T22:00:45Z

packages/core-data/src/reducer.js

+	return state;
+} );
+
+const saving = ifMatchingEntityConfig( ( state = {}, action ) => {


Curious why we need this. Can we use the resolving state?

Curious why we need this. Can we use the resolving state?

Unless I'm totally forgetting, that doesn't apply since resolvers are for selectors? Saving state occurs as a result of dispatching.

That being said, I could see some usefulness in knowing whether an action has "finished", and I imagine it would be possible to implement.

That being said, I could see some usefulness in knowing whether an action has "finished", and I imagine it would be possible to implement.

At the @wordpress/data level? That would be nice.

youknowriad · 2019-07-25T22:02:54Z

packages/core-data/src/reducer.js

@@ -277,6 +369,82 @@ export function autosaves( state = {}, action ) {
 	return state;
 }

+export const undo = combineReducers( {


I was doubtful about the refactoring to rely on actions instead of history for this but just the fact that it makes the implementation "separate" from the original reducer (SoC) seems cleaner.

epiqueras · 2019-07-25T22:37:36Z

This is awesome! Great work 🚀 I just finished reading all of the changes. I think this is the right approach. Let me know when you think it's polished enough to receive detailed feedback.

Let's try to get the extracted PRs merged as well as a way to trim this one down a bit.

epiqueras · 2019-07-25T22:54:44Z

packages/editor/src/store/actions.js

+	const postId = yield select( 'core/editor', 'getCurrentPostId' );
+	const edits = yield select( 'core', 'getEntityRecordEdits', 'postType', postType, postId );
+	const record = { id: postId, ...edits };
+	yield dispatch( 'core', 'saveEntityRecord', 'postType', postType, record );


Shouldn't this just persist the current edits if none are provided?

Shouldn't this just persist the current edits if none are provided?

Could you clarify what you mean by this? The edits state should ideally reflect the minimal difference between the server-persisted copy, so what's sent here is only those subset of properties.

Yeah, but is saveEntityRecord ever going to be called with anything other than the result of getEntityRecordEdits? I think that can go inside saveEntityRecord, at least as a default value.

Yeah, but is saveEntityRecord ever going to be called with anything other than the result of getEntityRecordEdits?

I think it could, yes. I might want to totally bypass local edits, and just save something outright.

That being said, I could see an argument to be made to your latter point that it might make sense as a default, or to merge pending edits into what is passed to saveEntityRecord, though it's unclear whether those edits would be preferred over or defaulted in absence of what's passed explicitly to saveEntityRecord.

I think this goes in conjunction with the result of getEntityRecord. If by default it returns the entity wiith the edits, save should also take the edits into account otherwise it's going to be confusing for third-party usage: trigger save and get a different version from the one visible before the save.

My opinion is still to keep this explicit (not bake edits) because that's how it works now otherwise we might risk some BC concerns in plugins.

defaulted in absence of what's passed explicitly to saveEntityRecord.

I think that would avoid BC issues right?

It would yes, but don't you think it's weird that getEntiityRecord would return the persisted value by default, calling saveEntityRecord would bring new changes and the user might be confused (where are they coming from)?

Maybe it's better if we have a separate action called saveEntityRecordEdits?

kadamwhite · 2019-08-06T18:12:43Z

I'm cross-linking this comment from the extracted PR for moving auto-draft handling to the server, to echo the previous question about whether the REST API controller was a more appropriate place for that change than the insert post method. As the PR's been merged I'm noting it here for visibility.

aduth · 2019-11-04T15:44:43Z

Closing as superseded by #16814, #16867, #16903, #16932.

aduth added [Type] Technical Prototype Offers a technical exploration into an idea as an example of what's possible [Status] In Progress Tracking issues with work in progress [Package] Core data /packages/core-data [Package] Editor /packages/editor labels Jul 25, 2019

This was referenced Jul 25, 2019

Data: Create registry selector with self-contained registry proxying #16692

Merged

Data: Document WPDataRegistry properties #16693

Merged

youknowriad reviewed Jul 25, 2019

View reviewed changes

epiqueras reviewed Jul 25, 2019

View reviewed changes

This was referenced Jul 26, 2019

Components: Restore default opacity to buttons #16769

Merged

Editor: Avoid passing event object to save button onSave prop #16770

Merged

aduth added 6 commits July 29, 2019 15:47

Data: Create registry selector with self-contained registry proxying

d87bef8

Compat: Force updated post status from auto-draft to draft

f5748c0

Compat: Unset assigned post title from auto-draft

a74c59c

Core Data: Preserve references of unchanged entity property values

6bd9a1b

Core Data: Add support for entity edits

efd303c

Editor: Leverage core data entities for edits, history, change detection

94c94a0

aduth force-pushed the add/data-edits branch from c100dff to 94c94a0 Compare July 29, 2019 19:47

This was referenced Jul 30, 2019

Editor: Move auto-draft status change to the server. #16814

Merged

Core Data: Add util for minimally modifying items. #16823

Merged

Core Data: Add support for entity edits and undo history. #16867

Merged

youknowriad added the Full Site Editing label Oct 9, 2019

aduth closed this Nov 4, 2019

aduth deleted the add/data-edits branch November 4, 2019 15:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Editor: Leverage core data entities for edits, history, change detection #16761

Editor: Leverage core data entities for edits, history, change detection #16761

aduth commented Jul 25, 2019 •

edited

Loading

youknowriad left a comment

youknowriad Jul 25, 2019

aduth Jul 26, 2019

youknowriad Jul 25, 2019

aduth Jul 26, 2019

epiqueras Jul 26, 2019

youknowriad Jul 25, 2019

epiqueras commented Jul 25, 2019

epiqueras Jul 25, 2019

aduth Jul 26, 2019

epiqueras Jul 26, 2019

aduth Jul 26, 2019

youknowriad Jul 26, 2019

epiqueras Jul 26, 2019

youknowriad Jul 26, 2019

epiqueras Jul 26, 2019

kadamwhite commented Aug 6, 2019

aduth commented Nov 4, 2019

	case 'UPDATE_POST':
	case 'RESET_POST':
	const getCanonicalValue = action.type === 'UPDATE_POST' ?
	( key ) => action.edits[ key ] :
	( key ) => getPostRawValue( action.post[ key ] );

	return reduce( state, ( result, value, key ) => {
	if ( ! isEqual( value, getCanonicalValue( key ) ) ) {
	return result;
	}

	result = getMutateSafeObject( state, result );
	delete result[ key ];
	return result;
	}, state );

	if ( ! isShallowEqual( dependencies, lastDependencies ) ) {
	lastBlockSourceDependencies.set( source, dependencies );

	// Allow the loop to continue in order to assign latest
	// dependencies values, but mark for reset.
	reset = true;
	}

Editor: Leverage core data entities for edits, history, change detection #16761

Editor: Leverage core data entities for edits, history, change detection #16761

Conversation

aduth commented Jul 25, 2019 • edited Loading

youknowriad left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epiqueras commented Jul 25, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kadamwhite commented Aug 6, 2019

aduth commented Nov 4, 2019

aduth commented Jul 25, 2019 •

edited

Loading