From fd7a8ec4a56cadb000574115724c7b9220a45b8d Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Sat, 20 Oct 2018 21:25:38 -0400 Subject: [PATCH 1/3] Editor: Move dirty flag to blocks substate --- docs/data/data-core-editor.md | 12 ++ packages/editor/src/store/reducer.js | 18 +- packages/editor/src/store/selectors.js | 38 +++- packages/editor/src/store/test/reducer.js | 4 +- packages/editor/src/store/test/selectors.js | 192 +++++++++++++++++--- 5 files changed, 224 insertions(+), 40 deletions(-) diff --git a/docs/data/data-core-editor.md b/docs/data/data-core-editor.md index 5b823d1893d0db..5c7e7e41a0ab69 100644 --- a/docs/data/data-core-editor.md +++ b/docs/data/data-core-editor.md @@ -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 diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 9ea2259b2c7919..a3384b45a3e062 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -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 ) { @@ -287,7 +280,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' ], + } ), + ] )( { byClientId( state = {}, action ) { switch ( action.type ) { case 'RESET_BLOCKS': diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js index a4e426507f8167..0648d248571849 100644 --- a/packages/editor/src/store/selectors.js +++ b/packages/editor/src/store/selectors.js @@ -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 + ); +} + /** * Returns true if there are unsaved values for the current edit session, or * false if the editing state matches the saved or new post. @@ -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 ); } /** diff --git a/packages/editor/src/store/test/reducer.js b/packages/editor/src/store/test/reducer.js index 005fd9127690fd..d40b07b0b36bd7 100644 --- a/packages/editor/src/store/test/reducer.js +++ b/packages/editor/src/store/test/reducer.js @@ -279,7 +279,7 @@ 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( [] ); @@ -287,7 +287,7 @@ describe( 'state', () => { 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', () => { diff --git a/packages/editor/src/store/test/selectors.js b/packages/editor/src/store/test/selectors.js index 2474cc1e2565f8..09fea22bed899f 100644 --- a/packages/editor/src/store/test/selectors.js +++ b/packages/editor/src/store/test/selectors.js @@ -28,6 +28,7 @@ const { hasEditorUndo, hasEditorRedo, isEditedPostNew, + hasChangedContent, isEditedPostDirty, isCleanNewPost, getCurrentPost, @@ -271,14 +272,100 @@ describe( 'selectors', () => { } ); } ); + describe( 'hasChangedContent', () => { + it( 'should return false if no dirty blocks nor content property edit', () => { + const state = { + editor: { + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, + }, + }; + + expect( hasChangedContent( state ) ).toBe( false ); + } ); + + it( 'should return true if dirty blocks', () => { + const state = { + editor: { + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, + }, + }; + + expect( hasChangedContent( state ) ).toBe( true ); + } ); + + it( 'should return true if content property edit', () => { + const state = { + editor: { + present: { + blocks: { + isDirty: false, + }, + edits: { + content: 'text mode edited', + }, + }, + }, + }; + + expect( hasChangedContent( state ) ).toBe( true ); + } ); + } ); + describe( 'isEditedPostDirty', () => { - it( 'should return true when post saved state dirty', () => { + it( 'should return false when blocks state not dirty nor edits exist', () => { const state = { + optimist: [], editor: { - isDirty: true, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, - saving: { - requesting: false, + }; + + expect( isEditedPostDirty( state ) ).toBe( false ); + } ); + + it( 'should return true when blocks state dirty', () => { + const state = { + optimist: [], + editor: { + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, + }, + }; + + expect( isEditedPostDirty( state ) ).toBe( true ); + } ); + + it( 'should return true when edits exist', () => { + const state = { + optimist: [], + editor: { + present: { + blocks: { + isDirty: false, + }, + edits: { + excerpt: 'hello world', + }, + }, }, }; @@ -291,38 +378,40 @@ describe( 'selectors', () => { { beforeState: { editor: { - isDirty: true, + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, }, }, }, ], editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, }; expect( isEditedPostDirty( state ) ).toBe( true ); } ); - - it( 'should return false when post saved state not dirty', () => { - const state = { - editor: { - isDirty: false, - }, - saving: { - requesting: false, - }, - }; - - expect( isEditedPostDirty( state ) ).toBe( false ); - } ); } ); describe( 'isCleanNewPost', () => { it( 'should return true when the post is not dirty and has not been saved before', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { id: 1, @@ -339,7 +428,12 @@ describe( 'selectors', () => { it( 'should return false when the post is not dirty but the post has been saved', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { id: 1, @@ -356,7 +450,12 @@ describe( 'selectors', () => { it( 'should return false when the post is dirty but the post has not been saved', () => { const state = { editor: { - isDirty: true, + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, }, currentPost: { id: 1, @@ -851,7 +950,12 @@ describe( 'selectors', () => { it( 'should return true for pending posts', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { status: 'pending', @@ -867,7 +971,12 @@ describe( 'selectors', () => { it( 'should return true for draft posts', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { status: 'draft', @@ -883,7 +992,12 @@ describe( 'selectors', () => { it( 'should return false for published posts', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { status: 'publish', @@ -899,7 +1013,12 @@ describe( 'selectors', () => { it( 'should return true for published, dirty posts', () => { const state = { editor: { - isDirty: true, + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, }, currentPost: { status: 'publish', @@ -915,7 +1034,12 @@ describe( 'selectors', () => { it( 'should return false for private posts', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { status: 'private', @@ -931,7 +1055,12 @@ describe( 'selectors', () => { it( 'should return false for scheduled posts', () => { const state = { editor: { - isDirty: false, + present: { + blocks: { + isDirty: false, + }, + edits: {}, + }, }, currentPost: { status: 'future', @@ -950,7 +1079,12 @@ describe( 'selectors', () => { status: 'private', }, editor: { - isDirty: true, + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, }, saving: { requesting: false, From 4e0ba38033f6ae03c499473308626bf910bad352 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Sat, 20 Oct 2018 21:26:07 -0400 Subject: [PATCH 2/3] Editor: Optimize autosaveable condition --- packages/editor/src/store/selectors.js | 10 ++++- packages/editor/src/store/test/selectors.js | 47 ++++++++++++++------- 2 files changed, 40 insertions(+), 17 deletions(-) diff --git a/packages/editor/src/store/selectors.js b/packages/editor/src/store/selectors.js index 0648d248571849..783a0ee72d39af 100644 --- a/packages/editor/src/store/selectors.js +++ b/packages/editor/src/store/selectors.js @@ -487,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 ) ) { + 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 ) ) ); } diff --git a/packages/editor/src/store/test/selectors.js b/packages/editor/src/store/test/selectors.js index 09fea22bed899f..58a839ff93a471 100644 --- a/packages/editor/src/store/test/selectors.js +++ b/packages/editor/src/store/test/selectors.js @@ -1348,24 +1348,19 @@ describe( 'selectors', () => { editor: { present: { blocks: { - byClientId: {}, - order: {}, - }, - edits: { - content: 'foo', + isDirty: false, }, + edits: {}, }, }, initialEdits: {}, currentPost: { title: 'foo', - content: 'foo', excerpt: 'foo', }, saving: {}, autosave: { title: 'foo', - content: 'foo', excerpt: 'foo', }, }; @@ -1373,26 +1368,46 @@ describe( 'selectors', () => { expect( isEditedPostAutosaveable( state ) ).toBe( false ); } ); - it( 'should return true if title, excerpt, or content have changed', () => { - for ( const variantField of [ 'title', 'excerpt', 'content' ] ) { - for ( const constantField of without( [ 'title', 'excerpt', 'content' ], variantField ) ) { + it( 'should return true if content has changes', () => { + const state = { + editor: { + present: { + blocks: { + isDirty: true, + }, + edits: {}, + }, + }, + currentPost: { + title: 'foo', + excerpt: 'foo', + }, + saving: {}, + autosave: { + title: 'foo', + excerpt: 'foo', + }, + }; + + expect( isEditedPostAutosaveable( state ) ).toBe( true ); + } ); + + it( 'should return true if title or excerpt have changed', () => { + for ( const variantField of [ 'title', 'excerpt' ] ) { + for ( const constantField of without( [ 'title', 'excerpt' ], variantField ) ) { const state = { editor: { present: { blocks: { - byClientId: {}, - order: {}, - }, - edits: { - content: 'foo', + isDirty: false, }, + edits: {}, }, }, initialEdits: {}, currentPost: { title: 'foo', content: 'foo', - excerpt: 'foo', }, saving: {}, autosave: { From aa36d299c6df6737cd92640b0837b0d01a9d69b2 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Tue, 30 Oct 2018 12:53:54 -0400 Subject: [PATCH 3/3] Editor: Remove redundant artificial dirtying --- packages/editor/src/store/effects/posts.js | 12 +----- packages/editor/src/store/reducer.js | 3 -- packages/editor/src/store/test/effects.js | 44 ++-------------------- 3 files changed, 5 insertions(+), 54 deletions(-) diff --git a/packages/editor/src/store/effects/posts.js b/packages/editor/src/store/effects/posts.js index 6d6d23917c2bca..c88fe00f4d878f 100644 --- a/packages/editor/src/store/effects/posts.js +++ b/packages/editor/src/store/effects/posts.js @@ -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 ) { diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index a3384b45a3e062..6bfb468cd71578 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -257,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' ? diff --git a/packages/editor/src/store/test/effects.js b/packages/editor/src/store/test/effects.js index 83aedc2e4a84ec..ce54c418d5fa11 100644 --- a/packages/editor/src/store/test/effects.js +++ b/packages/editor/src/store/test/effects.js @@ -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'; @@ -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: { @@ -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.', @@ -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.', @@ -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.', @@ -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', () => {