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

Introduce a dedicated autosaves endpoint for handling autosave behavior #6257

Merged
merged 124 commits into from
May 31, 2018
Merged
Show file tree
Hide file tree
Changes from 25 commits
Commits
Show all changes
124 commits
Select commit Hold shift + click to select a range
9b41d7c
autosaves with rest controller, first pass
Apr 17, 2018
004c830
use newer title call
Apr 18, 2018
9b0a3a4
autosave: compare raw attributes for change check
Apr 18, 2018
b4ad672
Clean up autosave effects
Apr 18, 2018
aba8548
pull isAutosaving from props in PostSavedState
Apr 18, 2018
c6d5cce
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 18, 2018
345a508
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 18, 2018
bb99f4c
from .14
Apr 18, 2018
5e85a53
effects, improve toggle autosave timing, enable autosaving for publis…
Apr 18, 2018
491ce55
Disable ‘save draft’ after draft is autosaved
Apr 18, 2018
17a67f6
revert post saved state changes, keep save button active regardless o…
Apr 18, 2018
81ca0de
remove unrelated showAutosaveAlert
Apr 18, 2018
4da3ef1
fixes for eslint
Apr 18, 2018
b189a3c
leverage withChangeDetection for autosaves
Apr 18, 2018
7769231
Merge branch 'master' into fix/autosaves
Apr 18, 2018
d4400a9
fixes for php slint
Apr 18, 2018
ab44c63
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 22, 2018
5b0e36d
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 23, 2018
949557e
Merge branch 'master' into fix/autosaves
Apr 23, 2018
e4d20ff
rename WP_REST_Autosaves_Controller -> GB_REST_Autosaves_Controller …
Apr 23, 2018
0833e15
Add a docblock for savePost action
Apr 23, 2018
6b07e56
check state.autosave exists before using
Apr 24, 2018
becb66b
Fix autosave tests
Apr 24, 2018
ecc1527
Revert "leverage withChangeDetection for autosaves"
Apr 24, 2018
16ce6f1
rename rest controler file to match class passing phplint
Apr 24, 2018
124895b
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 26, 2018
fd84ca5
Merge branch 'master' into fix/autosaves
Apr 26, 2018
dad5683
set up isAutosaving
Apr 27, 2018
6c74256
rename controller WP_REST_Autosaves_Controller and only load if class…
Apr 27, 2018
5018748
Cleanup doc blocks
Apr 27, 2018
93881fc
rename revision_controller -> revisions_controller
Apr 27, 2018
5771f51
Merge branch 'master' of github.com:WordPress/gutenberg
Apr 27, 2018
7585b00
Merge branch 'master' into fix/autosaves
Apr 27, 2018
ed7d553
clean up isAutosaving reducer, removing toggleAutosave action
Apr 27, 2018
82a1b16
fixes for php-lint
Apr 27, 2018
e02c636
fix test: button stays in saved state when post not dirty
Apr 27, 2018
a3c61f7
restore state check
Apr 27, 2018
a204ece
Properly instantiate the autosaves controller
danielbachhuber Apr 27, 2018
6c4b7a2
Merge branch 'fix/autosaves' of github.com:WordPress/gutenberg into f…
Apr 30, 2018
e5e9e25
ensure isAutosave returns a boolean
Apr 30, 2018
7cd9547
Add PHP Unit tests covering WP_REST_Autosaves_Controller functionality
Apr 30, 2018
ba0a25f
Avoid duplicate route resgistration
Apr 30, 2018
bc79d1f
Rename/fix up unit tests for rest controller
Apr 30, 2018
7a9c123
fixes for phpcs
Apr 30, 2018
76e6296
Merge branch 'master' into fix/autosaves
May 3, 2018
5ce9de1
Merge branch 'master' of github.com:WordPress/gutenberg
May 3, 2018
750355f
Merge branch 'master' into fix/autosaves
May 3, 2018
f28f2de
Merge branch 'master' of github.com:WordPress/gutenberg
May 4, 2018
b4b95e0
Merge branch 'master' of github.com:WordPress/gutenberg
May 7, 2018
b68c7a5
Merge branch 'master' of github.com:WordPress/gutenberg
May 8, 2018
159d6e0
Merge branch 'master' into fix/autosaves
May 8, 2018
9142591
document autosave as a parameter property for options
May 8, 2018
6e7c57c
remove redundant check for isEditedPostNew
May 8, 2018
5ee1cae
rename currentlyAutosaving -> isAutosaving
May 8, 2018
7247780
move autosave reducer outside editor
May 8, 2018
193d32e
set the default autosave state to null
May 8, 2018
35f41ee
remove unneded object check that confused logic
May 8, 2018
9af3e3f
fix for eslint
May 8, 2018
e83bdc2
ensure href set before using in gutenberg_add_target_schema_to_links
May 8, 2018
dfc22ed
Merge branch 'master' of github.com:WordPress/gutenberg
May 11, 2018
00d586a
Merge branch 'master' into fix/autosaves
May 11, 2018
07cccd7
include the autosaves controller
May 11, 2018
dbd3ab6
docblock object -> boolean
May 11, 2018
4204ad2
document isAutosaving reducer
May 11, 2018
dfe6bc3
document autosave reducer
May 11, 2018
6cd8729
skip destructuring post
May 11, 2018
7e8b6a2
eliminate unneeded conditional
May 11, 2018
bb26c8c
rename autosavable -> autosaveable (with an e)
May 11, 2018
a4eab38
Simplify class check
May 11, 2018
0ca5be0
fixes for phpcs
May 11, 2018
d253b12
savePost on CMD-S
May 11, 2018
b06eb18
correctly return autosave from selector
May 11, 2018
668907b
correct state autosave usage
May 11, 2018
0d1f9b5
fix docblock
May 11, 2018
92b8d79
add test: should stop autosave timer when the autosave is up to date
May 11, 2018
376379d
correct test naming: should stop autosave timer when the autosave is …
May 11, 2018
74935ff
Add an e2e test for post autosaving
May 11, 2018
85a67cd
whitespace
May 11, 2018
c81ab2a
Merge branch 'master' into fix/autosaves
May 14, 2018
d795e1d
respect AUTOSAVE_INTERVAL for autosave timing
May 14, 2018
2531102
reset saving state on `RESET_AUTOSAVE`
May 14, 2018
264965d
add mu plugin to lower the autosave interval for testing
May 14, 2018
f1df3ff
docs spacing
May 14, 2018
b7042f7
Add JSDoc for hasAutosave
May 14, 2018
1fe8e20
eslint: missing trailing comma
May 14, 2018
c31eb05
Add tests for the hasAutosave selector
May 14, 2018
f88fe54
remove unused isAutosave in REQUEST_POST_UPDATE_SUCCESS
May 14, 2018
5b5107b
adjust initial state for autosave test
May 14, 2018
2012003
only pass autosaveInterval
May 14, 2018
89131f1
Testing: Trigger programmatic autosave
aduth May 14, 2018
e2e05e5
Merge branch 'fix/autosaves' of github.com:WordPress/gutenberg into f…
May 15, 2018
3e5e744
Merge branch 'master' of github.com:WordPress/gutenberg
May 23, 2018
2042645
Merge branch 'master' into fix/autosaves
May 23, 2018
bd29ca0
revert 2012003
May 23, 2018
751a47c
remove unused `className`
May 23, 2018
7b1945a
use isAutosaving
May 23, 2018
7fd7c79
Drop autosave interval back to 10 seconds, because 60 is really slow
danielbachhuber May 23, 2018
7034cdc
clarify use of isEditedPostSaveable in effects/AUTOSAVE
May 23, 2018
20f2b43
Merge branch 'fix/autosaves' of github.com:WordPress/gutenberg into f…
May 23, 2018
f669754
Prevent autosaves if an autosave is in progress.
May 23, 2018
7b4bb19
Add isEditedPostSaveable logic to isPostAutosaveable
May 24, 2018
18e1018
autosave monitor tests - use isAutosaveable
May 24, 2018
1d40167
fix tests for autosave monitor
May 24, 2018
f8724bd
Autosave: Cancel scheduled autosave when autosaving starts
aduth May 25, 2018
7a3e87b
Autosave: Style: Use property shorthand for autosaveInterval assignment
aduth May 25, 2018
02f5a15
Revert "revert 2012003"
aduth May 25, 2018
de5c887
State: Rename `isPostAutosaveable` to `isEditedPostAutosaveable`
aduth May 25, 2018
8347b52
State: Implement `isEditedPostAutosaveable` as composing `isAutosavin…
aduth May 25, 2018
66505be
State: Implement autosave action as action creator
aduth May 25, 2018
77ce650
State: Improve isAutosavingPost selector JSDoc
aduth May 25, 2018
e0c33c8
State: Revert doAutosave action creator name to autosave
aduth May 25, 2018
4e82c42
Testing: Avoid potential race conditions on Cmd+S save fail
aduth May 25, 2018
5d25123
State: Prevent save request if post is not saveable
aduth May 25, 2018
747f90c
State: Implement autosaving as superset of saving condition
aduth May 25, 2018
dc2e758
State: Consider save-in-progress as unsaveable
aduth May 25, 2018
8d5dcf0
Testing: Evaluate autosave completion by class presence
aduth May 25, 2018
83ad219
Keyboard Shortcuts: Disallow non-dirty save requests
aduth May 25, 2018
4821f49
State: Track autosave object as subset of received properties
aduth May 25, 2018
2dfc666
State: Remove redundant condition from autosaveable
aduth May 25, 2018
102b944
State: Simplify isEditedPostAutosaveable as Array#some of fields
aduth May 25, 2018
563a294
State: Deprecate getEditedPostExcerpt selector
aduth May 25, 2018
0bec4ea
Merge branch 'master' into fix/autosaves
danielbachhuber May 31, 2018
f3e564a
Remove tests originally removed in bdcbf714345045b5c96ef2d61ee986a1cf…
danielbachhuber May 31, 2018
11f9930
Fix linting
danielbachhuber May 31, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 11 additions & 6 deletions editor/components/autosave-monitor/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,13 @@ import { withSelect, withDispatch } from '@wordpress/data';

export class AutosaveMonitor extends Component {
componentDidUpdate( prevProps ) {
const { isDirty, isSaveable } = this.props;
if ( prevProps.isDirty !== isDirty ||
prevProps.isSaveable !== isSaveable ) {
this.toggleTimer( isDirty && isSaveable );
const { isDirty, isSaveable, isAutosavable } = this.props;
if (
prevProps.isDirty !== isDirty ||
prevProps.isSaveable !== isSaveable ||
prevProps.isAutosavable !== isAutosavable
) {
this.toggleTimer( isDirty && isSaveable && isAutosavable );
}
}

Expand All @@ -35,13 +38,15 @@ export class AutosaveMonitor extends Component {

export default compose( [
withSelect( ( select ) => {
const { isEditedPostDirty, isEditedPostSaveable } = select( 'core/editor' );
const { isEditedPostDirty, isEditedPostSaveable, isPostAutosavable } = select( 'core/editor' );
return {
isDirty: isEditedPostDirty(),
isSaveable: isEditedPostSaveable(),
isAutosavable: isPostAutosavable(),
};
} ),

withDispatch( ( dispatch ) => ( {
autosave: dispatch( 'core/editor' ).autosave,
autosave: dispatch( 'core/editor' ).doAutosave,
} ) ),
] )( AutosaveMonitor );
2 changes: 1 addition & 1 deletion editor/components/autosave-monitor/test/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ describe( 'AutosaveMonitor', () => {

describe( '#componentDidUpdate()', () => {
it( 'should start autosave timer when having become dirty and saveable', () => {
wrapper.setProps( { isDirty: true, isSaveable: true } );
wrapper.setProps( { isDirty: true, isSaveable: true, isAutosavable: true } );

expect( toggleTimer ).toHaveBeenCalledWith( true );
} );
Expand Down
3 changes: 2 additions & 1 deletion editor/components/post-publish-button/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export function PostPublishButton( {
visibility,
isPublishable,
isSaveable,
isAutosaving,
Copy link
Member

Choose a reason for hiding this comment

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

Where is the isAutosaving prop being passed?

Copy link
Member Author

Choose a reason for hiding this comment

The 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,
Expand All @@ -44,7 +45,7 @@ export function PostPublishButton( {
}

const className = classnames( 'editor-post-publish-button', {
'is-saving': isSaving,
'is-saving': isSaving && ! isAutosaving,
} );

const onClick = () => {
Expand Down
6 changes: 4 additions & 2 deletions editor/components/post-saved-state/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,13 +35,13 @@ export class PostSavedState extends Component {
}

render() {
const { isNew, isPublished, isDirty, isSaving, isSaveable, onSave } = this.props;
const { isNew, isPublished, isDirty, isSaving, isSaveable, onSave, isAutosaving } = this.props;
const { forceSavedMessage } = this.state;
if ( isSaving ) {
return (
<span className="editor-post-saved-state is-saving">
<Dashicon icon="cloud" />
{ __( 'Saving' ) }
{ isAutosaving ? __( 'Autosaving' ) : __( 'Saving' ) }
</span>
);
}
Expand Down Expand Up @@ -84,6 +84,7 @@ export default compose( [
isSavingPost,
isEditedPostSaveable,
getCurrentPost,
isAutosavingPost,
} = select( 'core/editor' );
return {
post: getCurrentPost(),
Expand All @@ -92,6 +93,7 @@ export default compose( [
isDirty: forceIsDirty || isEditedPostDirty(),
isSaving: forceIsSaving || isSavingPost(),
isSaveable: isEditedPostSaveable(),
isAutosaving: isAutosavingPost(),
};
} ),
withDispatch( ( dispatch ) => ( {
Expand Down
19 changes: 17 additions & 2 deletions editor/store/actions.js
Original file line number Diff line number Diff line change
Expand Up @@ -353,9 +353,17 @@ export function editPost( edits ) {
};
}

export function savePost() {
/**
* Returns an action object to save the post.
*
* @param {Object} options Set autosave: true for an autosave.
Copy link
Member

Choose a reason for hiding this comment

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

Minor: JSDoc has a solution for documenting properties of an argument that we could leverage:

http://usejsdoc.org/tags-param.html#parameters-with-properties

Copy link
Member Author

Choose a reason for hiding this comment

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

nice, changed in 9142591

*
* @return {Object} Action object.
*/
export function savePost( options ) {
Copy link
Member

Choose a reason for hiding this comment

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

Do these options merit a doc block?

Copy link
Member Author

Choose a reason for hiding this comment

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

added

return {
type: 'REQUEST_POST_UPDATE',
options,
};
}

Expand Down Expand Up @@ -393,12 +401,19 @@ export function mergeBlocks( blockAUid, blockBUid ) {
*
* @return {Object} Action object.
*/
export function autosave() {
export function doAutosave() {
return {
type: 'AUTOSAVE',
};
}

export function toggleAutosave( isAutosaving ) {
return {
type: 'DOING_AUTOSAVE',
isAutosaving,
};
}

/**
* Returns an action object used in signalling that undo history should
* restore last popped state.
Expand Down
108 changes: 62 additions & 46 deletions editor/store/effects.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,14 @@ import {
removeBlock,
resetBlocks,
setTemplateValidity,
toggleAutosave,
} from './actions';
import {
getCurrentPost,
getCurrentPostType,
getEditedPostContent,
getPostEdits,
isCurrentPostPublished,
isEditedPostDirty,
isEditedPostNew,
Copy link
Member

Choose a reason for hiding this comment

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

Why has this been removed? This is causing an error because the AUTOSAVE effect attempts to call it:

Uncaught ReferenceError: isEditedPostNew is not defined

Copy link
Member Author

Choose a reason for hiding this comment

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

that usage is now removed

isEditedPostSaveable,
getBlock,
getBlockCount,
Expand Down Expand Up @@ -102,41 +101,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 ) {
dispatch( toggleAutosave( true ) );
toSend.parent = post.id;
wp.apiRequest( { path: `/wp/v2/${ basePath }/${ post.id }/autosaves`, method: 'POST', data: toSend } ).then(
( autosave ) => {
dispatch( toggleAutosave( false ) );

dispatch( {
type: 'RESET_AUTOSAVE',
post: autosave,
} );

dispatch( {
type: 'REQUEST_POST_UPDATE_SUCCESS',
Copy link
Member

Choose a reason for hiding this comment

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

Why do we dispatch REQUEST_POST_UPDATE_SUCCESS for autosaves? It seems the action is used for two purposes:

  • Displaying a notice, which we've explicitly opted out of for autosaves
  • Resetting isSaving state, which appears to only be considered for full saves, not autosaves

Copy link
Member Author

Choose a reason for hiding this comment

The 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,

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

Why do we dispatch REQUEST_POST_UPDATE_SUCCESS for autosaves? It seems the action is used for two purposes:

good question, let me dig in a bit further to see why this is here

Copy link
Member

Choose a reason for hiding this comment

The 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?

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth this is used in the saving reducer to reset the saving state. This call resets the state, and that gets reflects in the PostSavedState component - without this the saving action never completes.

I see how the naming is confusing, maybe I can try resetting saving on RESET_AUTOSAVE instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth In 2531102 I switched this to resetting saving on the RESET_AUTOSAVE, eliminating the need to dispatch REQUEST_POST_UPDATE_SUCCESS

previousPost: post,
post: post,
isAutosave: true,
} );
},
() => {
dispatch( toggleAutosave( false ) );
} );
}
);
} 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' ];
Expand All @@ -145,8 +169,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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

When will isAutosave be truthy ? (Now that we've removed REQUEST_POST_UPDATE_SUCCESS from the isAutosave flow above)

Copy link
Member Author

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -309,20 +334,11 @@ export default {
return;
Copy link
Member

Choose a reason for hiding this comment

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

The line above this: We check isEditedPostSaveable. Shouldn't this be isEditedPostAutosaveable ? Do we allow autosave to occur while autosave is already in progress? What happens then to the value of state's isAutosaving when the first autosave returns (i.e. triggers RESET_AUTOSAVE) but the second hasn't?

Copy link
Member Author

Choose a reason for hiding this comment

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

isEditedPostSaveable verifies that a post has a title, an excerpt, or non-empty content. I added inline documentation explaining the use. Fixed in 7034cdc.

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.

Copy link
Member

Choose a reason for hiding this comment

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

With the changes in 7b4bb19, should the AUTOSAVE effect be checking isEditedPostAutosaveable instead of isEditedPostSaveable (which now includes isEditedPostSaveable anyways)?

Copy link
Member

Choose a reason for hiding this comment

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

Maybe worth testing the behavior of Cmd+S, which fires itself as autosave, where we rely on the effect to stop itself from an unnecessary save.

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 ) ) {
Copy link
Member

Choose a reason for hiding this comment

The 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 ) ) {

Copy link
Member Author

Choose a reason for hiding this comment

The 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 ) {
const { post, settings } = action;
Expand Down
20 changes: 20 additions & 0 deletions editor/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -228,6 +228,15 @@ export const editor = flow( [
ignoreTypes: [ 'RECEIVE_BLOCKS' ],
} ),
] )( {
autosave( state = false, action ) {
Copy link
Member

Choose a reason for hiding this comment

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

Per #6257 (comment), I hesitate about including autosave here. Particularly, via withChangeDetection, RESET_AUTOSAVE would cause the post to be marked as dirty. I'm not sure if there are issues presently caused by this. (Should ideally be accounted for in E2E tests by #6209)

Copy link
Member Author

Choose a reason for hiding this comment

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

Right! I moved this back out of the editor reducer section in 7247780

Copy link
Member

Choose a reason for hiding this comment

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

Why do we represent the default state as a boolean, rather than a more semantically meaningful empty value like null ?

Copy link
Member Author

Choose a reason for hiding this comment

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

null is probably a better choice, chenged in 193d32e

const { post } = action;
switch ( action.type ) {
case 'RESET_AUTOSAVE':
return post;
}

return state;
},
edits( state = {}, action ) {
switch ( action.type ) {
case 'EDIT_POST':
Expand Down Expand Up @@ -584,6 +593,16 @@ export function isTyping( state = false, action ) {
return state;
}

export function currentlyAutosaving( state = false, action ) {
Copy link
Member

Choose a reason for hiding this comment

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

Noting some naming inconsistencies: saving vs. currentlyAutosaving (autosaving?). Maybe fair because this one reflects a boolean value, whereas the other is a more complex object shape. But then for boolean values we also have isTyping vs. currentlyAutosaving (isAutosaving?).

Copy link
Member Author

Choose a reason for hiding this comment

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

renamed to isAutosaving in 5ee1cae

switch ( action.type ) {
case 'DOING_AUTOSAVE':
Copy link
Member

Choose a reason for hiding this comment

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

Do we need a separate action for this? Couldn't we assume true if AUTOSAVE action is triggered, and back to false once REQUEST_POST_UPDATE_SUCCESS (or on failure, roll back)?

Copy link
Member Author

@adamsilverstein adamsilverstein Apr 27, 2018

Choose a reason for hiding this comment

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

removed/refactored in ed7d553

const { isAutosaving } = action;
return isAutosaving;
}

return state;
}

/**
* Reducer returning the block selection's state.
*
Expand Down Expand Up @@ -1008,6 +1027,7 @@ export const sharedBlocks = combineReducers( {

export default optimist( combineReducers( {
editor,
currentlyAutosaving,
currentPost,
isTyping,
blockSelection,
Expand Down
48 changes: 48 additions & 0 deletions editor/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,44 @@ export function isEditedPostEmpty( state ) {
);
}

/**
* Returns true if the post can be autosaved, or false otherwise.
*
* @param {Object} state Global application state
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc standards treat @param and @return as separate groupings with newline between, no alignment necessary between groups.

https://make.wordpress.org/core/handbook/best-practices/inline-documentation-standards/javascript/#functions

Copy link
Member Author

Choose a reason for hiding this comment

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

right, correcting

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in f1df3ff

* @return {boolean} Whether the post can be autosaved
*/
export function isPostAutosavable( state ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it "saveable" or "savable"?

We should pick one and keep consistency. Currently this is inconsistent with isPostSaveable

Copy link
Member Author

@adamsilverstein adamsilverstein May 11, 2018

Choose a reason for hiding this comment

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

Its either: https://en.oxforddictionaries.com/definition/saveable

Since you already have the 'saveable' use, I'll update my PR with this variation. its a bit easier to read.

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in bb26c8c

// If the post is autosaving, it is not autosavable.
if ( state.currentlyAutosaving ) {
return false;
}

// If we don't already have an autosave, the post is autosavable.
if ( ! hasAutosave( state ) ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is a post autosavable if it has no content? Or are we relying on the consumer (e.g. AutosaveMonitor) to also check isSavable ? Should we want to impose that? If a post can't be autosaved without also being savable, should we just include that in isPostAutosavable ?

if ( ! isPostSavable( state ) ) {
	return false;
}

Copy link
Member Author

Choose a reason for hiding this comment

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

Is a post autosavable if it has no content?

Yes I think so, if thats a change (the post isn't new). For example, if you create a post with only a title and publish it, then edit the title and clear it. When autosave fires it will save the empty state as an autosave. I guess we could avoid this if thats desired, I can check the classic editor to see what the behavior is there for empty posts.

If a post can't be autosaved without also being savable, should we just include that in isPostAutosavable ?

I couldn't find the isPostSavable or isSavable selectors, i think you mean isEditedPostSaveable

A post can't be saved if nothing has changed since the last save.
An autosave can be created - as long as the post has changed since the last autosave.

Copy link
Member

@chrisvanpatten chrisvanpatten May 11, 2018

Choose a reason for hiding this comment

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

Is a post autosavable if it has no content?

Yes I think so, if thats a change (the post isn't new). For example, if you create a post with only a title and publish it, then edit the title and clear it. When autosave fires it will save the empty state as an autosave. I guess we could avoid this if thats desired, I can check the classic editor to see what the behavior is there for empty posts.

I think slightly related is that you also get an autosave if you hit "Add New" for a new post, put your cursor in the default paragraph block, and then don't do anything (don't type, don't touch your keyboard, don't move your mouse, etc). That is not the case for the classic editor.

return true;
}

const title = getEditedPostAttribute( state, 'title' );
const excerpt = getEditedPostExcerpt( state );
const content = getEditedPostContent( state );
const autosave = state.editor.present.autosave;

// If the title, excerpt or content has changed, the post is autosavable.
if (
( autosave.title && title !== autosave.title.raw ) ||
( autosave.excerpt && excerpt !== autosave.excerpt.raw ) ||
Copy link
Member

Choose a reason for hiding this comment

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

It's not clear to me when autosave would have or not have these properties. For example, if getEditedPostExcerpt returned a non-empty value but autosave.excerpt were empty, this would not return true. Is that expected? Unit tests should clarify these expectations.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think autosave will always have the excerpt and title properties - they can't be empty, they can only be an empty string which will evaluate correctly. The reason for this is that the api always returns all fields, so excerpt is always returned even if "empty". autosave represents the last stored autosave so we can be confident an excerpt will always be set. I'm going to remove the checks for now since they are confusing and I don't think they are needed

Copy link
Member Author

Choose a reason for hiding this comment

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

fixed in 35f41ee

( autosave.content && content !== autosave.content.raw )
) {
return true;
Copy link
Member

Choose a reason for hiding this comment

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

Minor: This condition could be eliminated and replaced with a single return value:

return (
	( title !== autosave.title.raw ) ||
	( excerpt !== autosave.excerpt.raw ) ||
	( content !== autosave.content.raw )
);

Copy link
Member Author

Choose a reason for hiding this comment

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

nice! changed in 7e8b6a2

}

return false;
}

export function hasAutosave( state ) {
Copy link
Member

Choose a reason for hiding this comment

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

Can we add a JSDoc.

Copy link
Member

Choose a reason for hiding this comment

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

Unit tests would be good too.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍

Copy link
Member Author

Choose a reason for hiding this comment

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

@aduth added tests in c31eb05

return !! state.editor.present.autosave;
}

/**
* Return true if the post being edited is being scheduled. Preferring the
* unsaved status values.
Expand Down Expand Up @@ -1107,6 +1145,16 @@ export function didPostSaveRequestFail( state ) {
return !! state.saving.error;
}

/**
* Is the post autosaving?
*
* @param {Object} state Global application state
* @return {boolean} Whether the post is autosaving
*/
export function isAutosavingPost( state ) {
return !! state.currentlyAutosaving;
}

/**
* Returns a suggested post format for the current post, inferred only if there
* is a single block within the post and it is of a type known to match a
Expand Down
Loading