Core Data: Add support for autosaving entities. #16903

youknowriad · 2019-08-05T15:04:58Z

Based on the code changes, it feels that the common code is very small between autosaves and saves, which makes me wonder if they should be two separate action creators.

I thought that maybe future options could make them share more code.

Should we separate them for now?

I'm fine either way, you're in a better position to judge as you worked on it :)

youknowriad · 2019-08-05T15:11:31Z

Where is this selector defined?

gutenberg/packages/core-data/src/selectors.js

Line 440 in 9291845

export function getAutosave( state, postType, postId, authorId ) {

oh so it was defined before hand? Do you think a user can access autosaves of another user?

Probably, they are just posts right?

youknowriad · 2019-08-05T15:02:31Z

Any reason we need the currentUserId to fetch the autosaved post?

Autosaves are author-specific.

youknowriad · 2019-08-05T15:19:49Z

I'm having trouble understanding which data we're sending. Maybe a comment could help. Like do we send just specific record properties, everything, why are we merging these three records, and why that order.

I've added this comment:

// Autosaves need all expected fields to be present.
// So we fallback to the previous autosave and then
// to the actual persisted entity if the edits don't
// have a value.

I guess I'm wondering why we need the three steps merge. If I'm undersanding properly, we don't really need autosavePost here since the edits will most likely contain these as well right?

Not always.

It has 3 items because we have 2 fallbacks. First, the previous autosave, and then the persisted values.

Really? The UI only shows the edits and not the autosaved content so I'm having trouble thinking of use-cases where autosaves have more data than edits.

you would but that's an explicit action. saveEntityRecord( getAutosave() )

That would not work, you would need to merge the properties like in this PR.

Why complicate things when this will be the only usage of the API. When would you want to clear autosaved properties without changes?

How does it work today in the editor screen without the refactoring? Say I have an autosave but I ignore it and I continue editing my post, another autosave will trigger right without the updates from the previous autosave?

How would you do that in this new approach?

To be honest, I don't want to complicate anything. I actually think I'm simplifying things. Anyway, I won't block that PR for this, I feel we can tweak this later if needed but still cuirious about the answer to the question above.

We use this fallback mechanism in the editor store right now.

It works like this right now.

Say I have an autosave but I ignore it and I continue editing my post, another autosave will trigger right without the updates from the previous autosave?

It will have the updates from the previous autosave.

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

Lines 524 to 528 in 9291845

toSend = {

...pick( post, AUTOSAVE_PROPERTIES ),

...mappedAutosavePost,

...toSend,

};

It works like this right now.

Ok let's keep it but it feels like a bug to me. cc @adamsilverstein if you know whether an autosave should be created based on the previous autosave or based on the things that are shown in the UI (previous saved state + edits)

youknowriad · 2019-08-05T15:04:03Z

Is this something to be configured per entity (autosaveable fields or something)

Yeah, good idea.

youknowriad · 2019-08-05T15:03:29Z

Do you think this is a convention for all entities autosaves? Do we need to check whether a given entity supports autosaves?

I thought it was. Should it be part of the entity config?

We can start this way if it's consistent across WP entities, I was just asking.

-Original file line number
+Diff line change
@@ Expand Up / @@ -71,22 +71,6 @@ _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
-    entity records edits history. The offset
-    represents how many items from the end
-    of the history stack we are at. 0 is the
-    last edit, -1 is the second last, and so on.
-    _Parameters_
-    -   _state_ `Object`: State tree.
-    _Returns_
-    -   `number`: The current undo offset.
     <a name="getCurrentUser" href="#getCurrentUser">#</a> **getCurrentUser**
     Returns the current user.
@@ Expand Down Expand Up / @@ -359,6 +343,21 @@ _Returns_ @@
     -   `boolean`: Whether or not the user can upload media. Defaults to `true` if the OPTIONS request is being made.
+    <a name="isAutosavingEntityRecord" href="#isAutosavingEntityRecord">#</a> **isAutosavingEntityRecord**
+    Returns true if the specified entity record is autosaving, and false otherwise.
+    _Parameters_
+    -   _state_ `Object`: State tree.
+    -   _kind_ `string`: Entity kind.
+    -   _name_ `string`: Entity name.
+    -   _recordId_ `number`: Record ID.
+    _Returns_
+    -   `boolean`: Whether the entity record is autosaving or not.
     <a name="isPreviewEmbedFallback" href="#isPreviewEmbedFallback">#</a> **isPreviewEmbedFallback**
     Determines if the returned preview is an oEmbed link fallback.
@@ Expand Down Expand Up / @@ -403,7 +402,7 @@ _Parameters_ @@
     _Returns_
-    -   `?Object`: Whether the entity record is saving or not.
+    -   `boolean`: Whether the entity record is saving or not.
     <!-- END TOKEN(Autogenerated selectors) -->
@@ Expand Down Expand Up / @@ -561,6 +560,7 @@ _Parameters_ @@
     -   _kind_ `string`: Kind of the entity.
     -   _name_ `string`: Name of the entity.
     -   _recordId_ `Object`: ID of the record.
+    -   _options_ `Object`: Saving options.
     <a name="saveEntityRecord" href="#saveEntityRecord">#</a> **saveEntityRecord**
@@ Expand All / @@ -571,6 +571,7 @@ _Parameters_ @@
     -   _kind_ `string`: Kind of the received entity.
     -   _name_ `string`: Name of the received entity.
     -   _record_ `Object`: Record to be saved.
+    -   _options_ `Object`: Saving options.
     <a name="undo" href="#undo">#</a> **undo**
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -191,6 +191,7 @@ _Parameters_ @@
     -   _kind_ `string`: Kind of the entity.
     -   _name_ `string`: Name of the entity.
     -   _recordId_ `Object`: ID of the record.
+    -   _options_ `Object`: Saving options.
     <a name="saveEntityRecord" href="#saveEntityRecord">#</a> **saveEntityRecord**
@@ Expand All / @@ -201,6 +202,7 @@ _Parameters_ @@
     -   _kind_ `string`: Kind of the received entity.
     -   _name_ `string`: Name of the received entity.
     -   _record_ `Object`: Record to be saved.
+    -   _options_ `Object`: Saving options.
     <a name="undo" href="#undo">#</a> **undo**
@@ Expand Down Expand Up / @@ -280,22 +282,6 @@ _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
-    entity records edits history. The offset
-    represents how many items from the end
-    of the history stack we are at. 0 is the
-    last edit, -1 is the second last, and so on.
-    _Parameters_
-    -   _state_ `Object`: State tree.
-    _Returns_
-    -   `number`: The current undo offset.
     <a name="getCurrentUser" href="#getCurrentUser">#</a> **getCurrentUser**
     Returns the current user.
@@ Expand Down Expand Up / @@ -568,6 +554,21 @@ _Returns_ @@
     -   `boolean`: Whether or not the user can upload media. Defaults to `true` if the OPTIONS request is being made.
+    <a name="isAutosavingEntityRecord" href="#isAutosavingEntityRecord">#</a> **isAutosavingEntityRecord**
+    Returns true if the specified entity record is autosaving, and false otherwise.
+    _Parameters_
+    -   _state_ `Object`: State tree.
+    -   _kind_ `string`: Entity kind.
+    -   _name_ `string`: Entity name.
+    -   _recordId_ `number`: Record ID.
+    _Returns_
+    -   `boolean`: Whether the entity record is autosaving or not.
     <a name="isPreviewEmbedFallback" href="#isPreviewEmbedFallback">#</a> **isPreviewEmbedFallback**
     Determines if the returned preview is an oEmbed link fallback.
@@ Expand Down Expand Up / @@ -612,7 +613,7 @@ _Parameters_ @@
     _Returns_
-    -   `?Object`: Whether the entity record is saving or not.
+    -   `boolean`: Whether the entity record is saving or not.
     <!-- END TOKEN(Autogenerated selectors) -->
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -213,6 +213,7 @@ function entity( entityConfig ) { @@
     							[ action.recordId ]: {
     								pending: action.type === 'SAVE_ENTITY_RECORD_START',
     								error: action.error,
+    								isAutosave: action.isAutosave,
     							},
     						};
     				}
@@ Expand Down @@

-Original file line number
+Diff line change
@@ Expand Up / @@ -207,6 +207,25 @@ export const getEditedEntityRecord = createSelector( @@
     	( state ) => [ state.entities.data ]
     );
+    /**
+     * Returns true if the specified entity record is autosaving, and false otherwise.
+     *
+     * @param {Object} state    State tree.
+     * @param {string} kind     Entity kind.
+     * @param {string} name     Entity name.
+     * @param {number} recordId Record ID.
+     *
+     * @return {boolean} Whether the entity record is autosaving or not.
+     */
+    export function isAutosavingEntityRecord( state, kind, name, recordId ) {
+    	const { pending, isAutosave } = get(
+    		state.entities.data,
+    		[ kind, name, 'saving', recordId ],
+    		{}
+    	);
+    	return Boolean( pending && isAutosave );
+    }
     /**
      * Returns true if the specified entity record is saving, and false otherwise.
      *
@@ Expand All / @@ -215,14 +234,15 @@ export const getEditedEntityRecord = createSelector( @@
      * @param {string} name     Entity name.
      * @param {number} recordId Record ID.
      *
-     * @return {Object?} Whether the entity record is saving or not.
+     * @return {boolean} Whether the entity record is saving or not.
      */
     export function isSavingEntityRecord( state, kind, name, recordId ) {
-    	return get(
+    	const { pending, isAutosave } = get(
     		state.entities.data,
-    		[ kind, name, 'saving', recordId, 'pending' ],
-    		false
+    		[ kind, name, 'saving', recordId ],
+    		{}
     	);
+    	return Boolean( pending && ! isAutosave );
     }
     /**
@@ Expand Down Expand Up @@
      *
      * @return {number} The current undo offset.
      */
-    export function getCurrentUndoOffset( state ) {
+    function getCurrentUndoOffset( state ) {
     	return state.undo.offset;
     }
@@ Expand Down @@

Core Data: Add support for autosaving entities. #16903

Uh oh!

Core Data: Add support for autosaving entities. #16903

Uh oh!

Uh oh!

Diff view

Diff view

There are no files selected for viewing

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!

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!