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

Prevent RESET_BLOCKS from affecting reusable blocks #11746

Merged
merged 4 commits into from
Nov 19, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
55 changes: 53 additions & 2 deletions packages/editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,28 @@ 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 = '' ) {
noisysocks marked this conversation as resolved.
Show resolved Hide resolved
return reduce( blocksOrder[ rootClientId ], ( result, clientId ) => [
...result,
clientId,
...getNestedBlockClientIds( blocksOrder, clientId ),
], [] );
}

/**
* Returns an object against which it is safe to perform mutating operations,
* given the original object and its current working copy.
Expand Down Expand Up @@ -211,6 +233,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 {
Copy link
Member

Choose a reason for hiding this comment

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

It makes me think, rather than duplicate the logic of the reducer, is there a way we could inject the existing reusable blocks into the action to make sure they're persisted after the reset?

Copy link
Member

Choose a reason for hiding this comment

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

One advantage of this: I'd rather see RESET_BLOCKS remain in the reducer. In fact, I want it to be the only thing there (removing SETUP_EDITOR_STATE instead). See #11641.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah OK. I've pushed up 7d3d116 which changes things so that we instead augment RESET_BLOCKS with a list of client IDs that are referenced by reusable blocks and thus shouldn't be touched.

...state,
Copy link
Member

Choose a reason for hiding this comment

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

Are we trying to pretend that we don't know there's only the two properties byClientId and order that we're already returning anyways? 😄

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.
Expand Down Expand Up @@ -280,6 +331,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( {
Expand All @@ -289,7 +342,6 @@ export const editor = flow( [
] )( {
byClientId( state = {}, action ) {
switch ( action.type ) {
case 'RESET_BLOCKS':
case 'SETUP_EDITOR_STATE':
return getFlattenedBlocks( action.blocks );

Expand Down Expand Up @@ -392,7 +444,6 @@ export const editor = flow( [

order( state = {}, action ) {
switch ( action.type ) {
case 'RESET_BLOCKS':
case 'SETUP_EDITOR_STATE':
return mapBlockOrder( action.blocks );

Expand Down
52 changes: 52 additions & 0 deletions packages/editor/src/store/test/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -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, {
Expand Down