Core Data: Add support for entity edits and undo history. by epiqueras · Pull Request #16867 · WordPress/gutenberg

epiqueras · 2019-08-01T20:40:37Z

Based on #16761

Description

This PR follows a similar approach to adding edit and history support as #16761, but simplifies a lot of the logic by not relying on hacking property descriptors to let properties opt out of history tracking and change detection. Instead, it lets entity configs define which properties should opt out.

It also has a different take on the store structure which makes it play nicer with the current code and thus minimizes required changes.

The approach to undo/redo has also changed slightly, because it wasn't handling certain scenarios like undo => edit => redo.

How has this been tested?

The editor store refactor from #16761 was cherry picked on top and everything worked as expected.

Types of Changes

New Feature: Add edit, undo/redo history, and change detection support to core data entities.

Checklist:

My code is tested.
My code follows the WordPress code style.
My code follows the accessibility standards.
My code has proper inline documentation.
I've included developer documentation if appropriate.

youknowriad · 2019-08-02T12:40:26Z

What does "offset" mean in that context?

I'll clarify.

youknowriad · 2019-08-02T12:41:35Z

This is already available in the reducer, why duplicate it in the action? (I guess to ease access to this value in a sub reducer), can't we leverage a higher-order reducer to add it to the action instead?

can't we leverage a higher-order reducer to add it to the action instead

Good idea, I'll do that.

youknowriad · 2019-08-02T12:42:01Z

Isn't this behavior already done in the reducer? at least it feels like a better place for it.

I needed access to the entity, I guess I can also use a higher order reducer for this.

youknowriad · 2019-08-02T12:42:25Z

should we consider returning early if there's no edits?

youknowriad · 2019-08-02T12:42:53Z

Why do we use _ ? To denote that it's transient? Isn't it redundant?

Separate from this PR: we should document the entity config somewhere

Also, I know that some properties of the post object are not handled/merged separately (edits and persisted) and they're merged with a deep merge. Thinking of meta for instance (see the editor selectors..), I was thinking we probably need something here to "mark" these properties?

Why do we use _ ? To denote that it's transient? Isn't it redundant?

Yeah, it was in my head from the other PR, I'll remove it.

Also, I know that some properties of the post object are not handled/merged separately (edits and persisted) and they're merged with a deep merge. Thinking of meta for instance (see the editor selectors..), I was thinking we probably need something here to "mark" these properties?

What if we just deeply merge edits if they are objects?

What if we just deeply merge edits if they are objects?

We discussed this a long time for the PR that introduced the behavior and I think there's a valid reason (object values that we don't want to merge recursively). I'll have to look for the original PR to find the details

Related discussions here #10827

Mmmm, there will also be properties that should be merged to different depths. I think meta only merges shallowly.

What do you think of an editsMergeDepths object, which for now would have meta: 1.

This could work yes.

youknowriad

Do you think we should separate the core-data changes from the editor changes. I feel we can land the core-data changes separately (as they have value even without using them in the editor package). (Essentially to ease reviews/iterations)

epiqueras · 2019-08-02T14:42:52Z

Do you think we should separate the core-data changes from the editor changes. I feel we can land the core-data changes separately (as they have value even without using them in the editor package). (Essentially to ease reviews/iterations)

Yeah, definitely, I just wanted to test things together, I probably shouldn't have pushed, but I am about to reduce the scope here again and implement your suggestions.

youknowriad · 2019-08-02T20:00:49Z

+		// so that the property is not considered dirty.
+		edits: Object.keys( edits ).reduce( ( acc, key ) => {
+			const value = mergedEdits[ key ] ?
+				merge( record[ key ], edits[ key ] ) :


This works a bit differently than what we currently have in the editor package. I think we only store in the edits reducer the modified sub properties and do the merge in the selectors. This might be simpler though.

Yeah, we need this for undos to work in this context.

youknowriad

LGTM 👍 Great work. The undo comments are mostly thoughts for future possible enhancements maybe.

youknowriad · 2019-08-05T09:31:14Z

+ * @return {number} The current undo offset.
+ */
+export function getCurrentUndoOffset( state ) {
+	return state.undo.offset;


While I understand the purpose now, I do wonder whether this should be exposed or not, as it seems like just an implementation detail. Any particular reason for having this selector?

To not repeat it in the following two, but yeah, it could be private for now. Although I do potentially see people using it to show how many redos are available or something like that.

it's different than the offset though. Let's make it private for now.

It's the negation of the offset, yeah kind of confusing. I'll make it private in the autosaves PR.

youknowriad · 2019-08-05T09:34:43Z

+UNDO_INITIAL_STATE.offset = 0;
+export function undo( state = UNDO_INITIAL_STATE, action ) {
+	switch ( action.type ) {
+		case 'EDIT_ENTITY_RECORD':


Something to think about for later maybe: This could be made more generic. Instead of checking the action type, we could rely on the presence of undo property in the meta key in order to support any random action.

Yeah, that would be nice. We can add that if it's needed.

youknowriad · 2019-08-05T09:36:15Z

+			// Transient edits don't create an undo level, but are
+			// reachable in the next meaningful edit to which they
+			// are merged. They are defined in the entity's config.
+			if ( ! Object.keys( action.edits ).some( ( key ) => ! action.transientEdits[ key ] ) ) {


Kind of related to the previews comment, but we could decide to only set the meta undo key if there are non-transient edits.

But we still need transient edits to always go through this so that they are applied/unapplied.

epiqueras · 2019-08-05T13:29:56Z

Thanks for expediting these reviews. I'll merge and move on to autosaves now 😄 🚀

* Core Data: Add support for entity edits and undo history. * Core Data: Update docs.

epiqueras requested review from ajitbohra, chrisvanpatten, mkaz, nerrad and youknowriad as code owners August 1, 2019 20:40

epiqueras self-assigned this Aug 1, 2019

epiqueras added [Package] Core data /packages/core-data [Package] Editor /packages/editor labels Aug 1, 2019

epiqueras added this to the Future milestone Aug 1, 2019

epiqueras force-pushed the add/core-data-support-for-entity-edits branch from 5b5494e to 9f0b52c Compare August 2, 2019 11:38

epiqueras requested a review from talldan as a code owner August 2, 2019 11:48

epiqueras force-pushed the add/core-data-support-for-entity-edits branch 2 times, most recently from f5f95d7 to 90b72e2 Compare August 2, 2019 12:31

youknowriad reviewed Aug 2, 2019

View reviewed changes

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

ca5109f

epiqueras force-pushed the add/core-data-support-for-entity-edits branch from 90b72e2 to ca5109f Compare August 2, 2019 19:56

youknowriad reviewed Aug 2, 2019

View reviewed changes

Core Data: Update docs.

aafa91d

youknowriad approved these changes Aug 5, 2019

View reviewed changes

epiqueras merged commit 1df83a3 into master Aug 5, 2019

epiqueras deleted the add/core-data-support-for-entity-edits branch August 5, 2019 13:30

epiqueras mentioned this pull request Aug 5, 2019

Core Data: Add support for autosaving entities. #16903

Merged

5 tasks

youknowriad removed this from the Future milestone Aug 9, 2019

youknowriad added this to the Gutenberg 6.3 milestone Aug 9, 2019

epiqueras mentioned this pull request Aug 14, 2019

Core Data: Return updated record in saveEntityRecord. #17030

Merged

5 tasks

mmtr mentioned this pull request Aug 27, 2019

Private posts/pages triggers unsaved changes warnings #17209

Closed

gziolo pushed a commit that referenced this pull request Aug 29, 2019

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

de214b9

* Core Data: Add support for entity edits and undo history. * Core Data: Update docs.

gziolo pushed a commit that referenced this pull request Aug 29, 2019

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

64a188c

* Core Data: Add support for entity edits and undo history. * Core Data: Update docs.

epiqueras mentioned this pull request Aug 29, 2019

Record selection changes in history (undo) #16428

Closed

5 tasks

epiqueras mentioned this pull request Sep 7, 2019

Overview: Custom Entity Sources #17368

Closed

youknowriad added the Full Site Editing label Oct 9, 2019

aduth mentioned this pull request Nov 4, 2019

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

Closed

7 tasks

snyk-io Bot mentioned this pull request Aug 13, 2024

[Snyk] Upgrade @wordpress/element from 5.28.0 to 6.3.0 Playgirlkaybraz11/wp-xeet#8

Open

Conversation

epiqueras commented Aug 1, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

How has this been tested?

Types of Changes

Checklist:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Aug 2, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

youknowriad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

epiqueras commented Aug 5, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

epiqueras commented Aug 1, 2019 •

edited

Loading