Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 2 commits into from
Aug 5, 2019

Conversation

epiqueras
Copy link
Contributor

@epiqueras epiqueras commented Aug 1, 2019

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.

@epiqueras epiqueras self-assigned this Aug 1, 2019
@epiqueras epiqueras added [Package] Core data /packages/core-data [Package] Editor /packages/editor labels Aug 1, 2019
@epiqueras epiqueras added this to the Future milestone Aug 1, 2019
@epiqueras epiqueras force-pushed the add/core-data-support-for-entity-edits branch from 5b5494e to 9f0b52c Compare August 2, 2019 11:38
@epiqueras epiqueras requested a review from talldan as a code owner August 2, 2019 11:48
@epiqueras 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
@@ -71,6 +71,19 @@ _Returns_

- `?Array`: An array of autosaves for the post, or undefined if there is none.

<a name="getCurrentUndoOffset" href="#getCurrentUndoOffset">#</a> **getCurrentUndoOffset**

Returns the current undo offset for the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "offset" mean in that context?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll clarify.

acc[ key ] = isEqual( record[ key ], edits[ key ] ) ? undefined : edits[ key ];
return acc;
}, {} ),
transientEdits,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Good idea, I'll do that.

recordId,
// Clear edits when they are equal to their persisted counterparts
// so that the property is not considered dirty.
edits: Object.keys( edits ).reduce( ( acc, key ) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

* @param {Object} recordId ID of the record.
*/
export function* saveEditedEntityRecord( kind, name, recordId ) {
const edits = yield select( 'getEntityRecordEdits', kind, name, recordId );
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

@@ -35,6 +35,7 @@ function* loadPostTypeEntities() {
kind: 'postType',
baseURL: '/wp/v2/' + postType.rest_base,
name,
transientEdits: { _blocks: true },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Related discussions here #10827

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could work yes.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

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.

@epiqueras epiqueras force-pushed the add/core-data-support-for-entity-edits branch from 90b72e2 to ca5109f Compare August 2, 2019 19:56
// so that the property is not considered dirty.
edits: Object.keys( edits ).reduce( ( acc, key ) => {
const value = mergedEdits[ key ] ?
merge( record[ key ], edits[ key ] ) :
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

// 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 ] ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@epiqueras
Copy link
Contributor Author

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

@epiqueras epiqueras merged commit 1df83a3 into master Aug 5, 2019
@epiqueras epiqueras deleted the add/core-data-support-for-entity-edits branch August 5, 2019 13:30
@youknowriad youknowriad modified the milestones: Future, Gutenberg 6.3 Aug 9, 2019
gziolo pushed a commit that referenced this pull request Aug 29, 2019
* 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.

* Core Data: Update docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Core data /packages/core-data [Package] Editor /packages/editor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants