From 0c92c67ac97d151be3f820ccb3d15b72dfde7183 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Mon, 12 Nov 2018 17:28:59 +1100 Subject: [PATCH 1/4] Prevent RESET_BLOCKS from affecting reusable blocks Changes the editor reducer so that RESET_BLOCKS will only remove blocks that are actually in the post, i.e. blocks that are a descendent of the `''` `blocks.order` key. This fixes a bug where editing a post in Text Mode would break any reusable blocks in the post. --- packages/editor/src/store/reducer.js | 58 ++++++++++++++++++++++- packages/editor/src/store/test/reducer.js | 52 ++++++++++++++++++++ 2 files changed, 108 insertions(+), 2 deletions(-) diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 6bfb468cd71578..83d4c1b588b3bd 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -101,6 +101,31 @@ function getFlattenedBlocks( blocks ) { return flattenedBlocks; } +/** + * Given a block order map object, returns *all* of the block client IDs that are + * a descendant of the given root client ID. + * + * Calling this with `rootClientId` set to `''` results in a list of client IDs + * that are in the post. That is, it excludes blocks like fetched reusable + * blocks which are stored into state but not visible. + * + * @param {Object} blocksOrder Object that maps block client IDs to a list of + * nested block client IDs. + * @param {?string} rootClientId The root client ID to search. Defaults to ''. + * + * @return {Array} List of descendant client IDs. + */ +function getNestedBlockClientIds( blocksOrder, rootClientId = '' ) { + const attachedClientIds = []; + if ( blocksOrder[ rootClientId ] ) { + blocksOrder[ rootClientId ].forEach( ( clientId ) => { + attachedClientIds.push( clientId ); + attachedClientIds.push( ...getNestedBlockClientIds( blocksOrder, clientId ) ); + } ); + } + return attachedClientIds; +} + /** * Returns an object against which it is safe to perform mutating operations, * given the original object and its current working copy. @@ -211,6 +236,35 @@ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { return reducer( state, action ); }; +/** + * Higher-order reducer which targets the combined blocks reducer and handles + * the `RESET_BLOCKS` action. When dispatched, this action will replace all + * blocks that exist in the post, leaving blocks that exist only in state (e.g. + * reusable blocks) alone. + * + * @param {Function} reducer Original reducer function. + * + * @return {Function} Enhanced reducer function. + */ +const withBlockReset = ( reducer ) => ( state, action ) => { + if ( state && action.type === 'RESET_BLOCKS' ) { + const visibleClientIds = getNestedBlockClientIds( state.order ); + return { + ...state, + byClientId: { + ...omit( state.byClientId, visibleClientIds ), + ...getFlattenedBlocks( action.blocks ), + }, + order: { + ...omit( state.order, visibleClientIds ), + ...mapBlockOrder( action.blocks ), + }, + }; + } + + return reducer( state, action ); +}; + /** * Undoable reducer returning the editor post state, including blocks parsed * from current HTML markup. @@ -280,6 +334,8 @@ export const editor = flow( [ blocks: flow( [ combineReducers, + withBlockReset, + // Track whether changes exist, resetting at each post save. Relies on // editor initialization firing post reset as an effect. withChangeDetection( { @@ -289,7 +345,6 @@ export const editor = flow( [ ] )( { byClientId( state = {}, action ) { switch ( action.type ) { - case 'RESET_BLOCKS': case 'SETUP_EDITOR_STATE': return getFlattenedBlocks( action.blocks ); @@ -392,7 +447,6 @@ export const editor = flow( [ order( state = {}, action ) { switch ( action.type ) { - case 'RESET_BLOCKS': case 'SETUP_EDITOR_STATE': return mapBlockOrder( action.blocks ); diff --git a/packages/editor/src/store/test/reducer.js b/packages/editor/src/store/test/reducer.js index d40b07b0b36bd7..0d9561c15aaa8d 100644 --- a/packages/editor/src/store/test/reducer.js +++ b/packages/editor/src/store/test/reducer.js @@ -1074,6 +1074,58 @@ describe( 'state', () => { } ); describe( 'blocks', () => { + it( 'should not reset any blocks that are not in the post', () => { + const actions = [ + { + type: 'RESET_BLOCKS', + blocks: [ + { + clientId: 'block1', + innerBlocks: [ + { clientId: 'block11', innerBlocks: [] }, + { clientId: 'block12', innerBlocks: [] }, + ], + }, + ], + }, + { + type: 'RECEIVE_BLOCKS', + blocks: [ + { + clientId: 'block2', + innerBlocks: [ + { clientId: 'block21', innerBlocks: [] }, + { clientId: 'block22', innerBlocks: [] }, + ], + }, + ], + }, + ]; + const original = deepFreeze( actions.reduce( editor, undefined ) ); + + const state = editor( original, { + type: 'RESET_BLOCKS', + blocks: [ + { + clientId: 'block3', + innerBlocks: [ + { clientId: 'block31', innerBlocks: [] }, + { clientId: 'block32', innerBlocks: [] }, + ], + }, + ], + } ); + + expect( state.present.blocks.byClientId ).toEqual( { + block2: { clientId: 'block2' }, + block21: { clientId: 'block21' }, + block22: { clientId: 'block22' }, + block3: { clientId: 'block3' }, + block31: { clientId: 'block31' }, + block32: { clientId: 'block32' }, + } ); + } ); + describe( 'byClientId', () => { it( 'should return with attribute block updates', () => { const original = deepFreeze( editor( undefined, { From 7d3d116dff60ccf2379cbdbb809724228bb4ee98 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Tue, 13 Nov 2018 11:27:04 +1100 Subject: [PATCH 2/4] Augment RESET_BLOCKS action instead of replacing it Keep the RESET_BLOCKS logic in the reducer by instead using a higher-order reducer to augment RESET_BLOCKS with a list of client IDs that should remain in state as they are referenced by a reusable block. --- packages/editor/src/store/reducer.js | 72 ++++++++--------------- packages/editor/src/store/test/reducer.js | 55 +++++------------ 2 files changed, 41 insertions(+), 86 deletions(-) diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 83d4c1b588b3bd..85cbdb0ae10ae9 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -15,6 +15,8 @@ import { isEqual, overSome, get, + map, + pick, } from 'lodash'; /** @@ -101,31 +103,6 @@ function getFlattenedBlocks( blocks ) { return flattenedBlocks; } -/** - * Given a block order map object, returns *all* of the block client IDs that are - * a descendant of the given root client ID. - * - * Calling this with `rootClientId` set to `''` results in a list of client IDs - * that are in the post. That is, it excludes blocks like fetched reusable - * blocks which are stored into state but not visible. - * - * @param {Object} blocksOrder Object that maps block client IDs to a list of - * nested block client IDs. - * @param {?string} rootClientId The root client ID to search. Defaults to ''. - * - * @return {Array} List of descendant client IDs. - */ -function getNestedBlockClientIds( blocksOrder, rootClientId = '' ) { - const attachedClientIds = []; - if ( blocksOrder[ rootClientId ] ) { - blocksOrder[ rootClientId ].forEach( ( clientId ) => { - attachedClientIds.push( clientId ); - attachedClientIds.push( ...getNestedBlockClientIds( blocksOrder, clientId ) ); - } ); - } - return attachedClientIds; -} - /** * Returns an object against which it is safe to perform mutating operations, * given the original object and its current working copy. @@ -237,29 +214,18 @@ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { }; /** - * Higher-order reducer which targets the combined blocks reducer and handles - * the `RESET_BLOCKS` action. When dispatched, this action will replace all - * blocks that exist in the post, leaving blocks that exist only in state (e.g. - * reusable blocks) alone. + * Higher-order reducer targeting the combined reducer, augmenting the + * RESET_BLOCKS action with a list of client IDs that should remain in editor + * state as they are being referenced by a reusable block. * * @param {Function} reducer Original reducer function. * * @return {Function} Enhanced reducer function. */ -const withBlockReset = ( reducer ) => ( state, action ) => { +const withReusableBlockClientIdsOnReset = ( reducer ) => ( state, action ) => { if ( state && action.type === 'RESET_BLOCKS' ) { - const visibleClientIds = getNestedBlockClientIds( state.order ); - return { - ...state, - byClientId: { - ...omit( state.byClientId, visibleClientIds ), - ...getFlattenedBlocks( action.blocks ), - }, - order: { - ...omit( state.order, visibleClientIds ), - ...mapBlockOrder( action.blocks ), - }, - }; + const reusableBlockClientIds = map( state.reusableBlocks.data, 'clientId' ); + action = { ...action, reusableBlockClientIds }; } return reducer( state, action ); @@ -334,8 +300,6 @@ export const editor = flow( [ blocks: flow( [ combineReducers, - withBlockReset, - // Track whether changes exist, resetting at each post save. Relies on // editor initialization firing post reset as an effect. withChangeDetection( { @@ -345,6 +309,12 @@ export const editor = flow( [ ] )( { byClientId( state = {}, action ) { switch ( action.type ) { + case 'RESET_BLOCKS': + return { + ...pick( state, action.reusableBlockClientIds ), + ...getFlattenedBlocks( action.blocks ), + }; + case 'SETUP_EDITOR_STATE': return getFlattenedBlocks( action.blocks ); @@ -447,6 +417,12 @@ export const editor = flow( [ order( state = {}, action ) { switch ( action.type ) { + case 'RESET_BLOCKS': + return { + ...pick( state, action.reusableBlockClientIds ), + ...mapBlockOrder( action.blocks ), + }; + case 'SETUP_EDITOR_STATE': return mapBlockOrder( action.blocks ); @@ -1201,7 +1177,11 @@ export function autosave( state = null, action ) { return state; } -export default optimist( combineReducers( { +export default flow( [ + combineReducers, + withReusableBlockClientIdsOnReset, + optimist, +] )( { editor, initialEdits, currentPost, @@ -1219,4 +1199,4 @@ export default optimist( combineReducers( { autosave, settings, postSavingLock, -} ) ); +} ); diff --git a/packages/editor/src/store/test/reducer.js b/packages/editor/src/store/test/reducer.js index 0d9561c15aaa8d..118daaf787e04b 100644 --- a/packages/editor/src/store/test/reducer.js +++ b/packages/editor/src/store/test/reducer.js @@ -1074,55 +1074,30 @@ describe( 'state', () => { } ); describe( 'blocks', () => { - it( 'should not reset any blocks that are not in the post', () => { - const actions = [ - { - type: 'RESET_BLOCKS', - blocks: [ - { - clientId: 'block1', - innerBlocks: [ - { clientId: 'block11', innerBlocks: [] }, - { clientId: 'block12', innerBlocks: [] }, - ], - }, - ], - }, - { - type: 'RECEIVE_BLOCKS', - blocks: [ - { - clientId: 'block2', - innerBlocks: [ - { clientId: 'block21', innerBlocks: [] }, - { clientId: 'block22', innerBlocks: [] }, - ], - }, - ], - }, - ]; - const original = deepFreeze( actions.reduce( editor, undefined ) ); - + it( 'should not reset blocks referenced by a reusable block', () => { + const original = deepFreeze( editor( undefined, { + type: 'RESET_BLOCKS', + blocks: [ + { clientId: 'block1', innerBlocks: [] }, + { clientId: 'block2', innerBlocks: [] }, + ], + } ) ); const state = editor( original, { type: 'RESET_BLOCKS', blocks: [ - { - clientId: 'block3', - innerBlocks: [ - { clientId: 'block31', innerBlocks: [] }, - { clientId: 'block32', innerBlocks: [] }, - ], - }, + { clientId: 'block3', innerBlocks: [] }, ], + reusableBlockClientIds: [ 'block2' ], } ); expect( state.present.blocks.byClientId ).toEqual( { block2: { clientId: 'block2' }, - block21: { clientId: 'block21' }, - block22: { clientId: 'block22' }, block3: { clientId: 'block3' }, - block31: { clientId: 'block31' }, - block32: { clientId: 'block32' }, + } ); + expect( state.present.blocks.order ).toEqual( { + '': [ 'block3' ], + block2: [], + block3: [], } ); } ); From 059344617288d47ad20f47e7fa47d66df307b5b2 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 15 Nov 2018 11:17:11 +1100 Subject: [PATCH 3/4] Revert "Augment RESET_BLOCKS action instead of replacing it" This reverts commit 7d3d116dff60ccf2379cbdbb809724228bb4ee98. --- packages/editor/src/store/reducer.js | 72 +++++++++++++++-------- packages/editor/src/store/test/reducer.js | 55 ++++++++++++----- 2 files changed, 86 insertions(+), 41 deletions(-) diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 85cbdb0ae10ae9..83d4c1b588b3bd 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -15,8 +15,6 @@ import { isEqual, overSome, get, - map, - pick, } from 'lodash'; /** @@ -103,6 +101,31 @@ function getFlattenedBlocks( blocks ) { return flattenedBlocks; } +/** + * Given a block order map object, returns *all* of the block client IDs that are + * a descendant of the given root client ID. + * + * Calling this with `rootClientId` set to `''` results in a list of client IDs + * that are in the post. That is, it excludes blocks like fetched reusable + * blocks which are stored into state but not visible. + * + * @param {Object} blocksOrder Object that maps block client IDs to a list of + * nested block client IDs. + * @param {?string} rootClientId The root client ID to search. Defaults to ''. + * + * @return {Array} List of descendant client IDs. + */ +function getNestedBlockClientIds( blocksOrder, rootClientId = '' ) { + const attachedClientIds = []; + if ( blocksOrder[ rootClientId ] ) { + blocksOrder[ rootClientId ].forEach( ( clientId ) => { + attachedClientIds.push( clientId ); + attachedClientIds.push( ...getNestedBlockClientIds( blocksOrder, clientId ) ); + } ); + } + return attachedClientIds; +} + /** * Returns an object against which it is safe to perform mutating operations, * given the original object and its current working copy. @@ -214,18 +237,29 @@ const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => { }; /** - * Higher-order reducer targeting the combined reducer, augmenting the - * RESET_BLOCKS action with a list of client IDs that should remain in editor - * state as they are being referenced by a reusable block. + * Higher-order reducer which targets the combined blocks reducer and handles + * the `RESET_BLOCKS` action. When dispatched, this action will replace all + * blocks that exist in the post, leaving blocks that exist only in state (e.g. + * reusable blocks) alone. * * @param {Function} reducer Original reducer function. * * @return {Function} Enhanced reducer function. */ -const withReusableBlockClientIdsOnReset = ( reducer ) => ( state, action ) => { +const withBlockReset = ( reducer ) => ( state, action ) => { if ( state && action.type === 'RESET_BLOCKS' ) { - const reusableBlockClientIds = map( state.reusableBlocks.data, 'clientId' ); - action = { ...action, reusableBlockClientIds }; + const visibleClientIds = getNestedBlockClientIds( state.order ); + return { + ...state, + byClientId: { + ...omit( state.byClientId, visibleClientIds ), + ...getFlattenedBlocks( action.blocks ), + }, + order: { + ...omit( state.order, visibleClientIds ), + ...mapBlockOrder( action.blocks ), + }, + }; } return reducer( state, action ); @@ -300,6 +334,8 @@ export const editor = flow( [ blocks: flow( [ combineReducers, + withBlockReset, + // Track whether changes exist, resetting at each post save. Relies on // editor initialization firing post reset as an effect. withChangeDetection( { @@ -309,12 +345,6 @@ export const editor = flow( [ ] )( { byClientId( state = {}, action ) { switch ( action.type ) { - case 'RESET_BLOCKS': - return { - ...pick( state, action.reusableBlockClientIds ), - ...getFlattenedBlocks( action.blocks ), - }; - case 'SETUP_EDITOR_STATE': return getFlattenedBlocks( action.blocks ); @@ -417,12 +447,6 @@ export const editor = flow( [ order( state = {}, action ) { switch ( action.type ) { - case 'RESET_BLOCKS': - return { - ...pick( state, action.reusableBlockClientIds ), - ...mapBlockOrder( action.blocks ), - }; - case 'SETUP_EDITOR_STATE': return mapBlockOrder( action.blocks ); @@ -1177,11 +1201,7 @@ export function autosave( state = null, action ) { return state; } -export default flow( [ - combineReducers, - withReusableBlockClientIdsOnReset, - optimist, -] )( { +export default optimist( combineReducers( { editor, initialEdits, currentPost, @@ -1199,4 +1219,4 @@ export default flow( [ autosave, settings, postSavingLock, -} ); +} ) ); diff --git a/packages/editor/src/store/test/reducer.js b/packages/editor/src/store/test/reducer.js index 118daaf787e04b..0d9561c15aaa8d 100644 --- a/packages/editor/src/store/test/reducer.js +++ b/packages/editor/src/store/test/reducer.js @@ -1074,30 +1074,55 @@ describe( 'state', () => { } ); describe( 'blocks', () => { - it( 'should not reset blocks referenced by a reusable block', () => { - const original = deepFreeze( editor( undefined, { - type: 'RESET_BLOCKS', - blocks: [ - { clientId: 'block1', innerBlocks: [] }, - { clientId: 'block2', innerBlocks: [] }, - ], - } ) ); + it( 'should not reset any blocks that are not in the post', () => { + const actions = [ + { + type: 'RESET_BLOCKS', + blocks: [ + { + clientId: 'block1', + innerBlocks: [ + { clientId: 'block11', innerBlocks: [] }, + { clientId: 'block12', innerBlocks: [] }, + ], + }, + ], + }, + { + type: 'RECEIVE_BLOCKS', + blocks: [ + { + clientId: 'block2', + innerBlocks: [ + { clientId: 'block21', innerBlocks: [] }, + { clientId: 'block22', innerBlocks: [] }, + ], + }, + ], + }, + ]; + const original = deepFreeze( actions.reduce( editor, undefined ) ); + const state = editor( original, { type: 'RESET_BLOCKS', blocks: [ - { clientId: 'block3', innerBlocks: [] }, + { + clientId: 'block3', + innerBlocks: [ + { clientId: 'block31', innerBlocks: [] }, + { clientId: 'block32', innerBlocks: [] }, + ], + }, ], - reusableBlockClientIds: [ 'block2' ], } ); expect( state.present.blocks.byClientId ).toEqual( { block2: { clientId: 'block2' }, + block21: { clientId: 'block21' }, + block22: { clientId: 'block22' }, block3: { clientId: 'block3' }, - } ); - expect( state.present.blocks.order ).toEqual( { - '': [ 'block3' ], - block2: [], - block3: [], + block31: { clientId: 'block31' }, + block32: { clientId: 'block32' }, } ); } ); From a4ed06465beccdf3819e6e4d184b2e428aa0e798 Mon Sep 17 00:00:00 2001 From: Robert Anderson Date: Thu, 15 Nov 2018 11:19:01 +1100 Subject: [PATCH 4/4] Use reduce() to simplify getNestedBlockClientIds() --- packages/editor/src/store/reducer.js | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/packages/editor/src/store/reducer.js b/packages/editor/src/store/reducer.js index 83d4c1b588b3bd..9768ddeb0ed436 100644 --- a/packages/editor/src/store/reducer.js +++ b/packages/editor/src/store/reducer.js @@ -116,14 +116,11 @@ function getFlattenedBlocks( blocks ) { * @return {Array} List of descendant client IDs. */ function getNestedBlockClientIds( blocksOrder, rootClientId = '' ) { - const attachedClientIds = []; - if ( blocksOrder[ rootClientId ] ) { - blocksOrder[ rootClientId ].forEach( ( clientId ) => { - attachedClientIds.push( clientId ); - attachedClientIds.push( ...getNestedBlockClientIds( blocksOrder, clientId ) ); - } ); - } - return attachedClientIds; + return reduce( blocksOrder[ rootClientId ], ( result, clientId ) => [ + ...result, + clientId, + ...getNestedBlockClientIds( blocksOrder, clientId ), + ], [] ); } /**