-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Introduce a dedicated autosaves endpoint for handling autosave behavior #6257
Changes from 68 commits
9b41d7c
004c830
9b0a3a4
b4ad672
aba8548
c6d5cce
345a508
bb99f4c
5e85a53
491ce55
17a67f6
81ca0de
4da3ef1
b189a3c
7769231
d4400a9
ab44c63
5b0e36d
949557e
e4d20ff
0833e15
6b07e56
becb66b
ecc1527
16ce6f1
124895b
fd84ca5
dad5683
6c74256
5018748
93881fc
5771f51
7585b00
ed7d553
82a1b16
e02c636
a3c61f7
a204ece
6c4b7a2
e5e9e25
7cd9547
ba0a25f
bc79d1f
7a9c123
76e6296
5ce9de1
750355f
f28f2de
b4b95e0
b68c7a5
159d6e0
9142591
6e7c57c
5ee1cae
7247780
193d32e
35f41ee
9af3e3f
e83bdc2
dfc22ed
00d586a
07cccd7
dbd3ab6
4204ad2
dfe6bc3
6cd8729
7e8b6a2
bb26c8c
a4eab38
0ca5be0
d253b12
b06eb18
668907b
0d1f9b5
92b8d79
376379d
74935ff
85a67cd
c81ab2a
d795e1d
2531102
264965d
f1df3ff
b7042f7
1fe8e20
c31eb05
f88fe54
5b5107b
2012003
89131f1
e2e05e5
3e5e744
2042645
bd29ca0
751a47c
7b1945a
7fd7c79
7034cdc
20f2b43
f669754
7b4bb19
18e1018
1d40167
f8724bd
7a3e87b
02f5a15
de5c887
8347b52
66505be
77ce650
e0c33c8
4e82c42
5d25123
747f90c
dc2e758
8d5dcf0
83ad219
4821f49
2dfc666
102b944
563a294
0bec4ea
f3e564a
11f9930
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,7 @@ export function PostPublishButton( { | |
visibility, | ||
isPublishable, | ||
isSaveable, | ||
isAutosaving, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Where is the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. setting was lost in some recent changes, fixed in dad5683 |
||
user, | ||
onSubmit = noop, | ||
forceIsSaving, | ||
|
@@ -44,7 +45,7 @@ export function PostPublishButton( { | |
} | ||
|
||
const className = classnames( 'editor-post-publish-button', { | ||
'is-saving': isSaving, | ||
'is-saving': isSaving && ! isAutosaving, | ||
} ); | ||
|
||
const onClick = () => { | ||
|
@@ -75,6 +76,7 @@ export default compose( [ | |
isEditedPostSaveable, | ||
isEditedPostPublishable, | ||
getCurrentPostType, | ||
isAutosavingPost, | ||
} = select( 'core/editor' ); | ||
return { | ||
isSaving: forceIsSaving || isSavingPost(), | ||
|
@@ -83,6 +85,7 @@ export default compose( [ | |
isSaveable: isEditedPostSaveable(), | ||
isPublishable: forceIsDirty || isEditedPostPublishable(), | ||
postType: getCurrentPostType(), | ||
isAutosaving: isAutosavingPost(), | ||
}; | ||
} ), | ||
withDispatch( ( dispatch ) => { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -351,9 +351,18 @@ export function editPost( edits ) { | |
}; | ||
} | ||
|
||
export function savePost() { | ||
/** | ||
* Returns an action object to save the post. | ||
* | ||
* @param {Object} options Options for the save. | ||
* @param {boolean} options.autosave Perform an autosave if true. | ||
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function savePost( options ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do these There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added |
||
return { | ||
type: 'REQUEST_POST_UPDATE', | ||
options, | ||
}; | ||
} | ||
|
||
|
@@ -391,7 +400,7 @@ export function mergeBlocks( blockAUid, blockBUid ) { | |
* | ||
* @return {Object} Action object. | ||
*/ | ||
export function autosave() { | ||
export function doAutosave() { | ||
return { | ||
type: 'AUTOSAVE', | ||
}; | ||
|
Original file line number | Diff line number | Diff line change | ||
---|---|---|---|---|
|
@@ -49,9 +49,7 @@ import { | |||
getCurrentPostType, | ||||
getEditedPostContent, | ||||
getPostEdits, | ||||
isCurrentPostPublished, | ||||
isEditedPostDirty, | ||||
isEditedPostNew, | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why has this been removed? This is causing an error because the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that usage is now removed |
||||
isEditedPostSaveable, | ||||
getBlock, | ||||
getBlockCount, | ||||
|
@@ -102,41 +100,66 @@ export default { | |||
content: getEditedPostContent( state ), | ||||
id: post.id, | ||||
}; | ||||
|
||||
dispatch( { | ||||
type: 'UPDATE_POST', | ||||
edits: toSend, | ||||
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID }, | ||||
} ); | ||||
dispatch( removeNotice( SAVE_POST_NOTICE_ID ) ); | ||||
const basePath = wp.api.getPostTypeRoute( getCurrentPostType( state ) ); | ||||
wp.apiRequest( { path: `/wp/v2/${ basePath }/${ post.id }`, method: 'PUT', data: toSend } ).then( | ||||
( newPost ) => { | ||||
dispatch( resetPost( newPost ) ); | ||||
dispatch( { | ||||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||||
previousPost: post, | ||||
post: newPost, | ||||
edits: toSend, | ||||
optimist: { type: COMMIT, id: POST_UPDATE_TRANSACTION_ID }, | ||||
} ); | ||||
}, | ||||
( err ) => { | ||||
dispatch( { | ||||
type: 'REQUEST_POST_UPDATE_FAILURE', | ||||
error: get( err, [ 'responseJSON' ], { | ||||
code: 'unknown_error', | ||||
message: __( 'An unknown error occurred.' ), | ||||
} ), | ||||
post, | ||||
edits, | ||||
optimist: { type: REVERT, id: POST_UPDATE_TRANSACTION_ID }, | ||||
const isAutosave = action.options && action.options.autosave; | ||||
|
||||
if ( isAutosave ) { | ||||
toSend.parent = post.id; | ||||
wp.apiRequest( { path: `/wp/v2/${ basePath }/${ post.id }/autosaves`, method: 'POST', data: toSend } ).then( | ||||
( autosave ) => { | ||||
dispatch( { | ||||
type: 'RESET_AUTOSAVE', | ||||
post: autosave, | ||||
} ); | ||||
|
||||
dispatch( { | ||||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we dispatch
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Right, I think its mostly useful where you want to show the UI in the updated state "optimistically" (eg adding a todo, you show the todo added), then if things fail for some reason you can show an error and roll back the change. Not sure how useful that is for the autosave? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
good question, let me dig in a bit further to see why this is here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think of it more in the pure data sense: We assume some success state where a new autosave will become the referenced value, and if a failure is to occur, we revert back to the prior state value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @aduth this is used in the I see how the naming is confusing, maybe I can try resetting There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||||
previousPost: post, | ||||
post: post, | ||||
isAutosave: true, | ||||
} ); | ||||
}, | ||||
() => { | ||||
dispatch( { | ||||
type: 'RESET_AUTOSAVE', | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is the behavior here to revert back to the previous state when an autosave fails? This is already accomplished for the proper save via There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The reason we dispatch gutenberg/editor/store/reducer.js Line 592 in 07cccd7
Is redux-optimist appropriate for this use case? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As I recall, Redux optimist behaves in a way that an action dispatched can either be committed (confirmed) or reverted, in which case the original dispatched action is pretended to have never existed, though all other subsequent will proceed as expected. It could be relevant here, though seems like it could be just as well to reset explicitly. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think its fine to set this explicitly here. Optimist is really useful when you want to show the ui state completed (optimistically) then revert that if it somehow fails. In this case it seems fine to show the autosave is working until it completes or fails. |
||||
post: post, | ||||
} ); | ||||
} ); | ||||
} | ||||
); | ||||
} else { | ||||
dispatch( { | ||||
type: 'UPDATE_POST', | ||||
edits: toSend, | ||||
optimist: { type: BEGIN, id: POST_UPDATE_TRANSACTION_ID }, | ||||
} ); | ||||
dispatch( removeNotice( SAVE_POST_NOTICE_ID ) ); | ||||
wp.apiRequest( { path: `/wp/v2/${ basePath }/${ post.id }`, method: 'PUT', data: toSend } ).then( | ||||
( newPost ) => { | ||||
dispatch( resetPost( newPost ) ); | ||||
dispatch( { | ||||
type: 'REQUEST_POST_UPDATE_SUCCESS', | ||||
previousPost: post, | ||||
post: newPost, | ||||
edits: toSend, | ||||
optimist: { type: COMMIT, id: POST_UPDATE_TRANSACTION_ID }, | ||||
} ); | ||||
}, | ||||
( err ) => { | ||||
dispatch( { | ||||
type: 'REQUEST_POST_UPDATE_FAILURE', | ||||
error: get( err, [ 'responseJSON' ], { | ||||
code: 'unknown_error', | ||||
message: __( 'An unknown error occurred.' ), | ||||
} ), | ||||
post, | ||||
edits, | ||||
optimist: { type: REVERT, id: POST_UPDATE_TRANSACTION_ID }, | ||||
} ); | ||||
} | ||||
); | ||||
} | ||||
}, | ||||
REQUEST_POST_UPDATE_SUCCESS( action, store ) { | ||||
const { previousPost, post } = action; | ||||
const { previousPost, post, isAutosave } = action; | ||||
const { dispatch } = store; | ||||
|
||||
const publishStatus = [ 'publish', 'private', 'future' ]; | ||||
|
@@ -145,8 +168,9 @@ export default { | |||
|
||||
let noticeMessage; | ||||
let shouldShowLink = true; | ||||
if ( ! isPublished && ! willPublish ) { | ||||
// If saving a non published post, don't show any notice | ||||
|
||||
if ( isAutosave || ( ! isPublished && ! willPublish ) ) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When will There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. right, good catch. removed in f88fe54 |
||||
// If autosaving, or saving a non published post, don't show any notice | ||||
noticeMessage = null; | ||||
} else if ( isPublished && ! willPublish ) { | ||||
// If undoing publish status, show specific notice | ||||
|
@@ -309,20 +333,11 @@ export default { | |||
return; | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The line above this: We check There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I added a check to prevent triggering a new autosave while an autosave is occurring in f669754. I tested this using a throttled connection and a low autosave interval and in my testing it prevented duplicate autosave requests. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With the changes in 7b4bb19, should the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe worth testing the behavior of Cmd+S, which fires itself as Edit: Particularly, what happens when I press Cmd+S in quick succession (second press before the first autosave completes). |
||||
} | ||||
|
||||
if ( ! isEditedPostNew( state ) && ! isEditedPostDirty( state ) ) { | ||||
return; | ||||
} | ||||
|
||||
if ( isCurrentPostPublished( state ) ) { | ||||
// TODO: Publish autosave. | ||||
// - Autosaves are created as revisions for published posts, but | ||||
// the necessary REST API behavior does not yet exist | ||||
// - May need to check for whether the status of the edited post | ||||
// has changed from the saved copy (i.e. published -> pending) | ||||
if ( ! isEditedPostDirty( state ) ) { | ||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With this, could we just eliminate the previous condition? if ( ! isEditedPostNew( state ) && ! isEditedPostDirty( state ) ) { There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes, removed in 6e7c57c |
||||
return; | ||||
} | ||||
|
||||
dispatch( savePost() ); | ||||
dispatch( savePost( { autosave: true } ) ); | ||||
}, | ||||
SETUP_EDITOR( action, { getState } ) { | ||||
const { post } = action; | ||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -584,6 +584,26 @@ export function isTyping( state = false, action ) { | |
return state; | ||
} | ||
|
||
/** | ||
* Reducer returning autosave state. True if an autosave is currently occurring. | ||
* | ||
* @param {boolean} state Current state. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {Boolean} Updated state. | ||
*/ | ||
export function isAutosaving( state = false, action ) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add JSDoc describing what state is being tracked here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added in 4204ad2 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. it( 'should return ??? when the post is published while autosave is in progress', () => {
const original = isAutosaving( undefined, savePost( { autosave: true } ) );
const state = isAutosaving( original, savePost() );
expect( state ).toBe( /* ??? */ );
} ); Note: UI limitations imposed aren't enough. If we want it to be a case prevented, at the very least it should be considered as part of an It maybe speaks to a broader concern of: Is having separate state handling for "is autosaving" vs. "is saving" causing us more headaches to handle than if they were one in the same? Does it make sense for them to be considered as equivalent? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. possibly. autosaves and saves are very distinct and an autosave could be happening and then a save fired, these shouldn't interfere with each other. ideally, you should be able to click the update button even while an autosave is firing? if you are concerned about race conditions between the two, we could drop isAutosaving and prevent saves during autosaves and autosaves during saves. |
||
switch ( action.type ) { | ||
case 'REQUEST_POST_UPDATE': | ||
const isAutosave = action.options && action.options.autosave; | ||
return !! isAutosave; | ||
case 'RESET_AUTOSAVE': | ||
return false; | ||
} | ||
|
||
return state; | ||
} | ||
|
||
/** | ||
* Reducer returning the block selection's state. | ||
* | ||
|
@@ -1056,8 +1076,26 @@ export const blockListSettings = ( state = {}, action ) => { | |
return state; | ||
}; | ||
|
||
/** | ||
* Reducer returning the most recent autosave. | ||
* | ||
* @param {Object} state The autosave object. | ||
* @param {Object} action Dispatched action. | ||
* | ||
* @return {Object} Updated state. | ||
*/ | ||
export const autosave = ( state = null, action ) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we add JSDoc describing what state is being tracked here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. added in dfe6bc3 |
||
switch ( action.type ) { | ||
case 'RESET_AUTOSAVE': | ||
return action.post; | ||
} | ||
|
||
return state; | ||
}; | ||
|
||
export default optimist( combineReducers( { | ||
editor, | ||
isAutosaving, | ||
currentPost, | ||
isTyping, | ||
blockSelection, | ||
|
@@ -1070,5 +1108,6 @@ export default optimist( combineReducers( { | |
notices, | ||
sharedBlocks, | ||
template, | ||
autosave, | ||
settings, | ||
} ) ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I seem to recall raising previously, but am unable to find the conversation and resulting rebuttal (if one exists): When would
isAutosaveable
betrue
andisSaveable
befalse
? The issue I'm getting to here is that it seems likeisSaveable
should be built in to the selector logic forisAutosaveable
and thus made redundant as a separate prop here, simplifying toisDirty && isAutosaveable
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case
isSaveable
maps toisEditedPostSaveable
which tests if the post contains a title, an excerpt, or non-empty content. if you edit the content of an existing post and remove everything, the post is no longer savable, this prevents calling the autosave in that case. its possibly redundant to have this here and ineffects.js
, should i remove it there?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should
isEditedPostSaveable
be tested as part of theisAutosaveable
selector? In other words, is it ever the case that an autosave is valid whereisEditedPostSaveable
would otherwise returnfalse
? This would allow us to simplify both the effect and this component, where we could simply testisAutosaveable
and be assured that the post is also "saveable" (with valid content).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, thats a good idea. I can move that test into
isAutosavable
to clear up the logic here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated in 7b4bb19, adding the
isEditedPostSaveable
logic toisPostAutosaveable
and removing the extra calls toisEditedPostSaveable
here