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.

Sign up for GitHub

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

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

Merged
merged 3 commits into from
Nov 8, 2018
Merged

Editor: Reshape editor state for optimized and accurate dirtiness detection #10844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fd7a8ec
Editor: Move dirty flag to blocks substate
aduth Oct 21, 2018
4e0ba38
Editor: Optimize autosaveable condition
aduth Oct 21, 2018
aa36d29
Editor: Remove redundant artificial dirtying
aduth Oct 30, 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
12 changes: 12 additions & 0 deletions docs/data/data-core-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,18 @@ the post has been saved.

Whether the post is new.

### hasChangedContent

Returns true if content includes unsaved changes, or false otherwise.

*Parameters*

* state: Editor state.

*Returns*

Whether content includes unsaved changes.

### isEditedPostDirty

Returns true if there are unsaved values for the current edit session, or
Expand Down
12 changes: 1 addition & 11 deletions packages/editor/src/store/effects/posts.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -171,18 +171,8 @@ export const requestPostUpdate = async ( action, store ) => {
* @param {Object} action action object.
* @param {Object} store Redux Store.
*/
export const requestPostUpdateSuccess = ( action, store ) => {
export const requestPostUpdateSuccess = ( action ) => {
const { previousPost, post, isAutosave, postType } = action;
const { dispatch, getState } = store;

// TEMPORARY: If edits remain after a save completes, the user must be
// prompted about unsaved changes. This should be refactored as part of
// the `isEditedPostDirty` selector instead.
//
// See: https://github.com/WordPress/gutenberg/issues/7409
if ( Object.keys( getPostEdits( getState() ) ).length ) {
dispatch( { type: 'DIRTY_ARTIFICIALLY' } );
}

// Autosaves are neither shown a notice nor redirected.
if ( isAutosave ) {
Expand Down
21 changes: 10 additions & 11 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -236,13 +236,6 @@ export const editor = flow( [
ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
shouldOverwriteState,
} ),

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
withChangeDetection( {
resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
} ),
] )( {
edits( state = {}, action ) {
switch ( action.type ) {
Expand All @@ -264,9 +257,6 @@ export const editor = flow( [

return state;

case 'DIRTY_ARTIFICIALLY':
return { ...state };

case 'UPDATE_POST':
case 'RESET_POST':
const getCanonicalValue = action.type === 'UPDATE_POST' ?
Expand All @@ -287,7 +277,16 @@ export const editor = flow( [
return state;
},

blocks: combineReducers( {
blocks: flow( [
combineReducers,

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
withChangeDetection( {
resetTypes: [ 'SETUP_EDITOR_STATE', 'REQUEST_POST_UPDATE_START' ],
ignoreTypes: [ 'RECEIVE_BLOCKS', 'RESET_POST', 'UPDATE_POST' ],
} ),
] )( {
Copy link
Member

Choose a reason for hiding this comment

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

Is everything below here just indented? It's a bit hard to see the code changes, if there are any.

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 everything below here just indented? It's a bit hard to see the code changes, if there are any.

Yes, no changes aside from indentation.

GitHub has an option to hide whitespace-only changes, which should help make things a bit clearer.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

Funny that this Higher order reducer moved from "edits" to just "blocks". It almost feels like we need an editBlocks :)

byClientId( state = {}, action ) {
switch ( action.type ) {
case 'RESET_BLOCKS':
Expand Down
48 changes: 46 additions & 2 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,26 @@ export function isEditedPostNew( state ) {
return getCurrentPost( state ).status === 'auto-draft';
}

/**
* Returns true if content includes unsaved changes, or false otherwise.
*
* @param {Object} state Editor state.
*
* @return {boolean} Whether content includes unsaved changes.
*/
export function hasChangedContent( state ) {
return (
state.editor.present.blocks.isDirty ||

// `edits` is intended to contain only values which are different from
// the saved post, so the mere presence of a property is an indicator
// that the value is different than what is known to be saved. While
// content in Visual mode is represented by the blocks state, in Text
// mode it is tracked by `edits.content`.
'content' in state.editor.present.edits
Copy link
Member

Choose a reason for hiding this comment

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

I don't really get what this second condition is checking for.

Copy link
Member Author

@aduth aduth Oct 22, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I don't really get what this second condition is checking for.

I'll add a comment to clarify, but it's a combination of two things which require this consideration:

  • While content in Visual mode is represented by the blocks state, in Text mode it is tracked by edits.content.
  • edits is intended to contain only the different property values, so it is expected that the mere presence of any property is an indicator that the value is different than what is known to be saved.

);
}

/**
* Returns true if there are unsaved values for the current edit session, or
* false if the editing state matches the saved or new post.
Expand All @@ -111,7 +131,23 @@ export function isEditedPostNew( state ) {
* @return {boolean} Whether unsaved values exist.
*/
export function isEditedPostDirty( state ) {
return state.editor.isDirty || inSomeHistory( state, isEditedPostDirty );
if ( hasChangedContent( state ) ) {
return true;
}

// Edits should contain only fields which differ from the saved post (reset
// at initial load and save complete). Thus, a non-empty edits state can be
// inferred to contain unsaved values.
if ( Object.keys( state.editor.present.edits ).length > 0 ) {
return true;
}

// Edits and change detectiona are reset at the start of a save, but a post
// is still considered dirty until the point at which the save completes.
// Because the save is performed optimistically, the prior states are held
// until committed. These can be referenced to determine whether there's a
// chance that state may be reverted into one considered dirty.
return inSomeHistory( state, isEditedPostDirty );
Copy link
Contributor

@youknowriad youknowriad Nov 7, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

So if I understand properly. We conder a post dirty until it's completely saved (successful response)? right? any particular reason? should we clarify why in a comment? (I know it's not new in this PR though)

Copy link
Member Author

Choose a reason for hiding this comment

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

So if I understand properly. We conder a post dirty until it's completely saved (successful response)? right? any particular reason? should we clarify why in a comment? (I know it's not new in this PR though)

It's because change detection (now only for blocks) is reset at the start of the save, but we still want to avoid allowing a user to reload while that request is still pending.

It's covered in this test:

gutenberg/test/e2e/specs/change-detection.test.js

Lines 216 to 230 in 26ccc15

it( 'Should prompt if changes and save is in-flight', async () => {
await page.type( '.editor-post-title__input', 'Hello World' );
// Hold the posts request so we don't deal with race conditions of the
// save completing early. Other requests should be allowed to continue,
// for example the page reload test.
await interceptSave();
// Keyboard shortcut Ctrl+S save.
await pressWithModifier( META_KEY, 'S' );
await releaseSaveIntercept();
await assertIsDirty( true );
} );

I'll see if I can form a useful comment here.

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'll see if I can form a useful comment here.

Added in rebased fd7a8ec.

}

/**
Expand Down Expand Up @@ -451,9 +487,17 @@ export function isEditedPostAutosaveable( state ) {
return true;
}

// To avoid an expensive content serialization, use the content dirtiness
// flag in place of content field comparison against the known autosave.
// This is not strictly accurate, and relies on a tolerance toward autosave
// request failures for unnecessary saves.
if ( hasChangedContent( state ) ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess this is the fix for the performance issue.

It might not be easy but I wonder if we can add an e2e test checking that we don't run save functions as we type :)

Copy link
Member Author

Choose a reason for hiding this comment

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

It might not be easy but I wonder if we can add an e2e test checking that we don't run save functions as we type :)

Hmm, might not be too hard! I'll give it a shot.

return true;
}

// If the title, excerpt or content has changed, the post is autosaveable.
const autosave = getAutosave( state );
return [ 'title', 'excerpt', 'content' ].some( ( field ) => (
return [ 'title', 'excerpt' ].some( ( field ) => (
autosave[ field ] !== getEditedPostAttribute( state, field )
) );
}
Expand Down
44 changes: 4 additions & 40 deletions packages/editor/src/store/test/effects.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ import actions, {
resetBlocks,
selectBlock,
setTemplateValidity,
editPost,
} from '../actions';
import effects, { validateBlocksToTemplate } from '../effects';
import { SAVE_POST_NOTICE_ID } from '../effects/posts';
Expand Down Expand Up @@ -222,16 +221,6 @@ describe( 'effects', () => {
describe( '.REQUEST_POST_UPDATE_SUCCESS', () => {
const handler = effects.REQUEST_POST_UPDATE_SUCCESS;

function createGetState( hasLingeringEdits = false ) {
let state = reducer( undefined, {} );
if ( hasLingeringEdits ) {
state = reducer( state, editPost( { edited: true } ) );
}

const getState = () => state;
return getState;
}

const defaultPost = {
id: 1,
title: {
Expand Down Expand Up @@ -259,14 +248,11 @@ describe( 'effects', () => {
} );

it( 'should dispatch notices when publishing or scheduling a post', () => {
const dispatch = jest.fn();
const store = { dispatch, getState: createGetState() };

const previousPost = getDraftPost();
const post = getPublishedPost();
const postType = getPostType();

handler( { post, previousPost, postType }, store );
handler( { post, previousPost, postType } );

expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
'Post published.',
Expand All @@ -280,14 +266,11 @@ describe( 'effects', () => {
} );

it( 'should dispatch notices when reverting a published post to a draft', () => {
const dispatch = jest.fn();
const store = { dispatch, getState: createGetState() };

const previousPost = getPublishedPost();
const post = getDraftPost();
const postType = getPostType();

handler( { post, previousPost, postType }, store );
handler( { post, previousPost, postType } );

expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
'Post reverted to draft.',
Expand All @@ -299,14 +282,11 @@ describe( 'effects', () => {
} );

it( 'should dispatch notices when just updating a published post again', () => {
const dispatch = jest.fn();
const store = { dispatch, getState: createGetState() };

const previousPost = getPublishedPost();
const post = getPublishedPost();
const postType = getPostType();

handler( { post, previousPost, postType }, store );
handler( { post, previousPost, postType } );

expect( dataDispatch( 'core/notices' ).createSuccessNotice ).toHaveBeenCalledWith(
'Post updated.',
Expand All @@ -320,29 +300,13 @@ describe( 'effects', () => {
} );

it( 'should do nothing if the updated post was autosaved', () => {
const dispatch = jest.fn();
const store = { dispatch, getState: createGetState() };

const previousPost = getPublishedPost();
const post = { ...getPublishedPost(), id: defaultPost.id + 1 };

handler( { post, previousPost, isAutosave: true }, store );
handler( { post, previousPost, isAutosave: true } );

expect( dataDispatch( 'core/notices' ).createSuccessNotice ).not.toHaveBeenCalled();
} );

it( 'should dispatch dirtying action if edits linger after autosave', () => {
const dispatch = jest.fn();
const store = { dispatch, getState: createGetState( true ) };

const previousPost = getPublishedPost();
const post = { ...getPublishedPost(), id: defaultPost.id + 1 };

handler( { post, previousPost, isAutosave: true }, store );

expect( dispatch ).toHaveBeenCalledTimes( 1 );
expect( dispatch ).toHaveBeenCalledWith( { type: 'DIRTY_ARTIFICIALLY' } );
} );
} );

describe( '.REQUEST_POST_UPDATE_FAILURE', () => {
Expand Down
4 changes: 2 additions & 2 deletions packages/editor/src/store/test/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -279,15 +279,15 @@ describe( 'state', () => {
unregisterBlockType( 'core/test-block' );
} );

it( 'should return history (empty edits, blocks), dirty flag by default', () => {
it( 'should return history (empty edits, blocks) by default', () => {
const state = editor( undefined, {} );

expect( state.past ).toEqual( [] );
expect( state.future ).toEqual( [] );
expect( state.present.edits ).toEqual( {} );
expect( state.present.blocks.byClientId ).toEqual( {} );
expect( state.present.blocks.order ).toEqual( {} );
expect( state.isDirty ).toBe( false );
expect( state.present.blocks.isDirty ).toBe( false );
} );

it( 'should key by reset blocks clientId', () => {
Expand Down
Loading