From c382c79c2392b3e0db54f32caa084e33d1cff265 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 10 Apr 2019 15:47:02 -0400 Subject: [PATCH 1/4] Block Editor: Avoid creating new state reference on unchanging isPersistentChange --- packages/block-editor/CHANGELOG.md | 4 ++++ packages/block-editor/src/store/reducer.js | 7 ++++++- packages/block-editor/src/store/test/reducer.js | 13 +++++++++++++ 3 files changed, 23 insertions(+), 1 deletion(-) diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index d5e9d80b71aef..bc6e7685a126f 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -15,6 +15,10 @@ - `CopyHandler` will now only catch cut/copy events coming from its `props.children`, instead of from anywhere in the `document`. +### Internal + +- Improved handling of blocks state references for unchanging states. + ## 1.0.0 (2019-03-06) ### New Features diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 5e91c85f524bc..d643dca7cb65c 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -206,9 +206,14 @@ function withPersistentBlockChange( reducer ) { // Defer to previous state value (or default) unless changing or // explicitly marking as persistent. if ( state === nextState && ! isExplicitPersistentChange ) { + const nextIsPersistentChange = get( state, [ 'isPersistentChange' ], true ); + if ( state.isPersistentChange === nextIsPersistentChange ) { + return state; + } + return { ...nextState, - isPersistentChange: get( state, [ 'isPersistentChange' ], true ), + isPersistentChange: nextIsPersistentChange, }; } diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index 62adf23f298fd..c1fff7d03deda 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -1500,6 +1500,19 @@ describe( 'state', () => { expect( state.isPersistentChange ).toBe( true ); } ); + it( 'should retain reference for same state, same persistence', () => { + const original = deepFreeze( blocks( undefined, { + type: 'RESET_BLOCKS', + blocks: [], + } ) ); + + const state = blocks( original, { + type: '__INERT__', + } ); + + expect( state ).toBe( original ); + } ); + it( 'should not consider received blocks as persistent change', () => { const state = blocks( undefined, { type: 'RECEIVE_BLOCKS', From a35fa8269343a20c451669cc2a31307db665ce9b Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 10 Apr 2019 15:54:03 -0400 Subject: [PATCH 2/4] Block Editor: Consider received blocks state change as ignored --- packages/block-editor/CHANGELOG.md | 1 + .../src/components/provider/index.js | 8 ++- packages/block-editor/src/store/reducer.js | 53 ++++++++++++------- packages/block-editor/src/store/selectors.js | 19 +++++++ .../block-editor/src/store/test/reducer.js | 19 ++++--- .../e2e-tests/specs/change-detection.test.js | 17 ++++++ 6 files changed, 90 insertions(+), 27 deletions(-) diff --git a/packages/block-editor/CHANGELOG.md b/packages/block-editor/CHANGELOG.md index bc6e7685a126f..c275c7319f9c4 100644 --- a/packages/block-editor/CHANGELOG.md +++ b/packages/block-editor/CHANGELOG.md @@ -18,6 +18,7 @@ ### Internal - Improved handling of blocks state references for unchanging states. +- Updated handling of blocks state to effectively ignored programmatically-received blocks data (e.g. reusable blocks received from editor). ## 1.0.0 (2019-03-06) diff --git a/packages/block-editor/src/components/provider/index.js b/packages/block-editor/src/components/provider/index.js index f321b19dc1de8..9841f79d509a9 100644 --- a/packages/block-editor/src/components/provider/index.js +++ b/packages/block-editor/src/components/provider/index.js @@ -69,6 +69,7 @@ class BlockEditorProvider extends Component { const { getBlocks, isLastBlockChangePersistent, + __unstableIsLastBlockChangeIgnored, } = registry.select( 'core/block-editor' ); let blocks = getBlocks(); @@ -81,7 +82,12 @@ class BlockEditorProvider extends Component { } = this.props; const newBlocks = getBlocks(); const newIsPersistent = isLastBlockChangePersistent(); - if ( newBlocks !== blocks && this.isSyncingIncomingValue ) { + if ( + newBlocks !== blocks && ( + this.isSyncingIncomingValue || + __unstableIsLastBlockChangeIgnored() + ) + ) { this.isSyncingIncomingValue = false; blocks = newBlocks; isPersistent = newIsPersistent; diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index d643dca7cb65c..9813b9e93bd57 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -188,16 +188,6 @@ export function isUpdatingSameBlockAttribute( action, lastAction ) { function withPersistentBlockChange( reducer ) { let lastAction; - /** - * Set of action types for which a blocks state change should be considered - * non-persistent. - * - * @type {Set} - */ - const IGNORED_ACTION_TYPES = new Set( [ - 'RECEIVE_BLOCKS', - ] ); - return ( state, action ) => { let nextState = reducer( state, action ); @@ -217,16 +207,6 @@ function withPersistentBlockChange( reducer ) { }; } - // Some state changes should not be considered persistent, namely those - // which are not a direct result of user interaction. - const isIgnoredActionType = IGNORED_ACTION_TYPES.has( action.type ); - if ( isIgnoredActionType ) { - return { - ...nextState, - isPersistentChange: false, - }; - } - nextState = { ...nextState, isPersistentChange: ( @@ -244,6 +224,38 @@ function withPersistentBlockChange( reducer ) { }; } +/** + * Higher-order reducer intended to augment the blocks reducer, assigning an + * `isIgnoredChange` property value corresponding to whether a change in state + * can be considered as ignored. A change is considered ignored when the result + * of an action not incurred by direct user interaction. + * + * @param {Function} reducer Original reducer function. + * + * @return {Function} Enhanced reducer function. + */ +function withIgnoredBlockChange( reducer ) { + /** + * Set of action types for which a blocks state change should be considered + * non-persistent. + * + * @type {Set} + */ + const IGNORED_ACTION_TYPES = new Set( [ + 'RECEIVE_BLOCKS', + ] ); + + return ( state, action ) => { + const nextState = reducer( state, action ); + + if ( nextState !== state ) { + nextState.isIgnoredChange = IGNORED_ACTION_TYPES.has( action.type ); + } + + return nextState; + }; +} + /** * Higher-order reducer targeting the combined blocks reducer, augmenting * block client IDs in remove action to include cascade of inner blocks. @@ -385,6 +397,7 @@ export const blocks = flow( withBlockReset, withSaveReusableBlock, withPersistentBlockChange, + withIgnoredBlockChange, )( { byClientId( state = {}, action ) { switch ( action.type ) { diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index fb431d1a48d0f..005744bc023fe 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -1387,6 +1387,25 @@ export function isLastBlockChangePersistent( state ) { return state.blocks.isPersistentChange; } +/** + * Returns true if the most recent block change is be considered ignored, or + * false otherwise. An ignored change is one not to be committed by + * BlockEditorProvider, neither via `onChange` nor `onInput`. + * + * TODO: Removal Plan: Changes incurred by RECEIVE_BLOCKS should not be ignored + * if in-fact they result in a change in blocks state. The current need to + * ignore changes incurred not as a result of user interaction should be + * accounted for in the refactoring of reusable blocks as occurring within + * their own separate block editor / state (#7119). + * + * @param {Object} state Block editor state. + * + * @return {boolean} Whether the most recent block change was ignored. + */ +export function __unstableIsLastBlockChangeIgnored( state ) { + return state.blocks.isIgnoredChange; +} + /** * Returns the value of a post meta from the editor settings. * diff --git a/packages/block-editor/src/store/test/reducer.js b/packages/block-editor/src/store/test/reducer.js index c1fff7d03deda..fe63fc34cd65b 100644 --- a/packages/block-editor/src/store/test/reducer.js +++ b/packages/block-editor/src/store/test/reducer.js @@ -233,7 +233,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -295,7 +296,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -386,7 +388,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -478,7 +481,8 @@ describe( 'state', () => { const state = blocks( existingState, action ); expect( state ).toEqual( { - isPersistentChange: expect.anything(), + isPersistentChange: true, + isIgnoredChange: false, byClientId: { clicken: { clientId: 'chicken', @@ -512,6 +516,7 @@ describe( 'state', () => { attributes: {}, order: {}, isPersistentChange: true, + isIgnoredChange: false, } ); } ); @@ -1512,8 +1517,10 @@ describe( 'state', () => { expect( state ).toBe( original ); } ); + } ); - it( 'should not consider received blocks as persistent change', () => { + describe( 'isIgnoredChange', () => { + it( 'should consider received blocks as ignored change', () => { const state = blocks( undefined, { type: 'RECEIVE_BLOCKS', blocks: [ { @@ -1523,7 +1530,7 @@ describe( 'state', () => { } ], } ); - expect( state.isPersistentChange ).toBe( false ); + expect( state.isIgnoredChange ).toBe( true ); } ); } ); } ); diff --git a/packages/e2e-tests/specs/change-detection.test.js b/packages/e2e-tests/specs/change-detection.test.js index 0e529a95ca512..7c9dab21d4d79 100644 --- a/packages/e2e-tests/specs/change-detection.test.js +++ b/packages/e2e-tests/specs/change-detection.test.js @@ -288,4 +288,21 @@ describe( 'Change detection', () => { await assertIsDirty( true ); } ); + + it( 'should not prompt when receiving reusable blocks', async () => { + // Regression Test: Verify that non-modifying behaviors does not incur + // dirtiness. Previously, this could occur as a result of either (a) + // selecting a block, (b) opening the inserter, or (c) editing a post + // which contained a reusable block. The root issue was changes in + // block editor state as a result of reusable blocks data having been + // received, reflected here in this test. + // + // TODO: This should be considered a temporary test, existing only so + // long as the experimental reusable blocks fetching data flow exists. + // + // See: https://github.com/WordPress/gutenberg/issues/14766 + await page.evaluate( () => window.wp.data.dispatch( 'core/editor' ).__experimentalReceiveReusableBlocks( [] ) ); + + await assertIsDirty( false ); + } ); } ); From 1f9a41c7dba7105689431b23e196cb5bb99287e3 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Wed, 10 Apr 2019 16:00:21 -0400 Subject: [PATCH 3/4] fixup a35fa8269 --- packages/block-editor/src/store/reducer.js | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/packages/block-editor/src/store/reducer.js b/packages/block-editor/src/store/reducer.js index 9813b9e93bd57..0c7448d1a2a74 100644 --- a/packages/block-editor/src/store/reducer.js +++ b/packages/block-editor/src/store/reducer.js @@ -236,8 +236,7 @@ function withPersistentBlockChange( reducer ) { */ function withIgnoredBlockChange( reducer ) { /** - * Set of action types for which a blocks state change should be considered - * non-persistent. + * Set of action types for which a blocks state change should be ignored. * * @type {Set} */ From 4efa59b3ba3250dcd69bddf893f48f8d100146b6 Mon Sep 17 00:00:00 2001 From: Andrew Duthie Date: Thu, 11 Apr 2019 07:54:14 -0400 Subject: [PATCH 4/4] Block Editor: Regenerate documentation --- .../developers/data/data-core-block-editor.md | 14 ++++++++++++++ packages/block-editor/src/store/selectors.js | 11 +++++------ 2 files changed, 19 insertions(+), 6 deletions(-) diff --git a/docs/designers-developers/developers/data/data-core-block-editor.md b/docs/designers-developers/developers/data/data-core-block-editor.md index 38110f42aa2c7..5bdea7a18a34e 100644 --- a/docs/designers-developers/developers/data/data-core-block-editor.md +++ b/docs/designers-developers/developers/data/data-core-block-editor.md @@ -775,6 +775,20 @@ via its `onChange` callback, in addition to `onInput`. Whether the most recent block change was persistent. +### __unstableIsLastBlockChangeIgnored + +Returns true if the most recent block change is be considered ignored, or +false otherwise. An ignored change is one not to be committed by +BlockEditorProvider, neither via `onChange` nor `onInput`. + +*Parameters* + + * state: Block editor state. + +*Returns* + +Whether the most recent block change was ignored. + ## Actions ### resetBlocks diff --git a/packages/block-editor/src/store/selectors.js b/packages/block-editor/src/store/selectors.js index 005744bc023fe..d2f26e0df0e35 100644 --- a/packages/block-editor/src/store/selectors.js +++ b/packages/block-editor/src/store/selectors.js @@ -1392,17 +1392,16 @@ export function isLastBlockChangePersistent( state ) { * false otherwise. An ignored change is one not to be committed by * BlockEditorProvider, neither via `onChange` nor `onInput`. * - * TODO: Removal Plan: Changes incurred by RECEIVE_BLOCKS should not be ignored - * if in-fact they result in a change in blocks state. The current need to - * ignore changes incurred not as a result of user interaction should be - * accounted for in the refactoring of reusable blocks as occurring within - * their own separate block editor / state (#7119). - * * @param {Object} state Block editor state. * * @return {boolean} Whether the most recent block change was ignored. */ export function __unstableIsLastBlockChangeIgnored( state ) { + // TODO: Removal Plan: Changes incurred by RECEIVE_BLOCKS should not be + // ignored if in-fact they result in a change in blocks state. The current + // need to ignore changes not a result of user interaction should be + // accounted for in the refactoring of reusable blocks as occurring within + // their own separate block editor / state (#7119). return state.blocks.isIgnoredChange; }