Skip to content

Commit

Permalink
Improve typing performance by splitting attributes in state tree.
Browse files Browse the repository at this point in the history 
Attributes have been moved to `attributesByClientId` in order to
reduce the impact of typing throughout the state tree.
See #11782.

Review fixes pass #1

Simplify block flattening code in reducers.

Remove alignment toolbar optimization; should be a different PR

Fix minor bug in test

Fix failing getBlockDependantsCacheBust test

Remove new `*WithoutAttributes` selectors.

We'll go with a different approach: use the existing selectors, but
keep the dependencies as they are. The attributes may get stale, but
it doesn't matter if they're not being used.

Change attributesById structure to not have an inner attributes

Simplifying some selector dependencies

Simplify withSaveReusableBlock a bit further and clarify its existence

Reuse constant instead of running omit again

Remove no longer needed mapClientIds

Simplifying reducers after review comments.

Further changes to selectors after review

Fix types in JSDoc

Renaming attributesByClientId to attributes

Further selector fixes after review
  • Loading branch information
@sgomes
sgomes committed Dec 4, 2018
1 parent 2fc34bc commit 99f8a2b
Show file tree
Hide file tree
Showing 4 changed files with 601 additions and 195 deletions.
213 changes: 154 additions & 59 deletions packages/editor/src/store/reducer.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ import {
omitBy,
keys,
isEqual,
isEmpty,
overSome,
get,
} from 'lodash';
Expand Down Expand Up @@ -78,29 +79,52 @@ function mapBlockOrder( blocks, rootClientId = '' ) {
}

/**
* Given an array of blocks, returns an object containing all blocks, recursing
* into inner blocks. Keys correspond to the block client ID, the value of
* which is the block object.
* Helper method to iterate through all blocks, recursing into inner blocks,
* applying a transformation function to each one.
* Returns a flattened object with the transformed blocks.
*
* @param {Array} blocks Blocks to flatten.
* @param {Function} transform Transforming function to be applied to each block.
*
* @return {Object} Flattened blocks object.
* @return {Object} Flattened object.
*/
function getFlattenedBlocks( blocks ) {
const flattenedBlocks = {};
function flattenBlocks( blocks, transform ) {
const result = {};

const stack = [ ...blocks ];
while ( stack.length ) {
// `innerBlocks` is redundant data which can fall out of sync, since
// this is reflected in `blocks.order`, so exclude from appended block.
const { innerBlocks, ...block } = stack.shift();

stack.push( ...innerBlocks );

flattenedBlocks[ block.clientId ] = block;
result[ block.clientId ] = transform( block );
}

return flattenedBlocks;
return result;
}

/**
* Given an array of blocks, returns an object containing all blocks, without
* attributes, recursing into inner blocks. Keys correspond to the block client
* ID, the value of which is the attributes object.
*
* @param {Array} blocks Blocks to flatten.
*
* @return {Object} Flattened block attributes object.
*/
function getFlattenedBlocksWithoutAttributes( blocks ) {
return flattenBlocks( blocks, ( block ) => omit( block, 'attributes' ) );
}

/**
* Given an array of blocks, returns an object containing all block attributes,
* recursing into inner blocks. Keys correspond to the block client ID, the
* value of which is the attributes object.
*
* @param {Array} blocks Blocks to flatten.
*
* @return {Object} Flattened block attributes object.
*/
function getFlattenedBlockAttributes( blocks ) {
return flattenBlocks( blocks, ( block ) => block.attributes );
}

/**
Expand Down Expand Up @@ -252,7 +276,11 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
...state,
byClientId: {
...omit( state.byClientId, visibleClientIds ),
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
},
attributes: {
...omit( state.attributes, visibleClientIds ),
...getFlattenedBlockAttributes( action.blocks ),
},
order: {
...omit( state.order, visibleClientIds ),
Expand All @@ -264,6 +292,43 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
return reducer( state, action );
};

/**
* Higher-order reducer which targets the combined blocks reducer and handles
* the `SAVE_REUSABLE_BLOCK_SUCCESS` action. This action can't be handled by
* regular reducers and needs a higher-order reducer since it needs access to
* both `byClientId` and `attributes` simultaneously.
*
* @param {Function} reducer Original reducer function.
*
* @return {Function} Enhanced reducer function.
*/
const withSaveReusableBlock = ( reducer ) => ( state, action ) => {
if ( state && action.type === 'SAVE_REUSABLE_BLOCK_SUCCESS' ) {
const { id, updatedId } = action;

// If a temporary reusable block is saved, we swap the temporary id with the final one
if ( id === updatedId ) {
return state;
}

state = { ...state };

state.attributes = mapValues( state.attributes, ( attributes, clientId ) => {
const { name } = state.byClientId[ clientId ];
if ( name === 'core/block' && attributes.ref === id ) {
return {
...attributes,
ref: updatedId,
};
}

return attributes;
} );
}

return reducer( state, action );
};

/**
* Undoable reducer returning the editor post state, including blocks parsed
* from current HTML markup.
Expand Down Expand Up @@ -342,6 +407,8 @@ export const editor = flow( [

withBlockReset,

withSaveReusableBlock,

// Track whether changes exist, resetting at each post save. Relies on
// editor initialization firing post reset as an effect.
withChangeDetection( {
Expand All @@ -352,63 +419,114 @@ export const editor = flow( [
byClientId( state = {}, action ) {
switch ( action.type ) {
case 'SETUP_EDITOR_STATE':
return getFlattenedBlocks( action.blocks );
return getFlattenedBlocksWithoutAttributes( action.blocks );

case 'RECEIVE_BLOCKS':
return {
...state,
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'UPDATE_BLOCK_ATTRIBUTES':
case 'UPDATE_BLOCK':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
return state;
}

// Consider as updates only changed values
const nextAttributes = reduce( action.attributes, ( result, value, key ) => {
if ( value !== result[ key ] ) {
result = getMutateSafeObject( state[ action.clientId ].attributes, result );
result[ key ] = value;
}

return result;
}, state[ action.clientId ].attributes );

// Skip update if nothing has been changed. The reference will
// match the original block if `reduce` had no changed values.
if ( nextAttributes === state[ action.clientId ].attributes ) {
// Do nothing if only attributes change.
const changes = omit( action.updates, 'attributes' );
if ( isEmpty( changes ) ) {
return state;
}

// Otherwise merge attributes into state
return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
attributes: nextAttributes,
...changes,
},
};

case 'INSERT_BLOCKS':
return {
...state,
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'REPLACE_BLOCKS':
if ( ! action.blocks ) {
return state;
}

return {
...omit( state, action.clientIds ),
...getFlattenedBlocksWithoutAttributes( action.blocks ),
};

case 'REMOVE_BLOCKS':
return omit( state, action.clientIds );
}

return state;
},

attributes( state = {}, action ) {
switch ( action.type ) {
case 'SETUP_EDITOR_STATE':
return getFlattenedBlockAttributes( action.blocks );

case 'RECEIVE_BLOCKS':
return {
...state,
...getFlattenedBlockAttributes( action.blocks ),
};

case 'UPDATE_BLOCK':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
// Ignore updates if block isn't known or there are no attribute changes.
if ( ! state[ action.clientId ] || ! action.updates.attributes ) {
return state;
}

return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
...action.updates,
...action.updates.attributes,
},
};

case 'UPDATE_BLOCK_ATTRIBUTES':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
return state;
}

// Consider as updates only changed values
const nextAttributes = reduce( action.attributes, ( result, value, key ) => {
if ( value !== result[ key ] ) {
result = getMutateSafeObject( state[ action.clientId ], result );
result[ key ] = value;
}

return result;
}, state[ action.clientId ] );

// Skip update if nothing has been changed. The reference will
// match the original block if `reduce` had no changed values.
if ( nextAttributes === state[ action.clientId ] ) {
return state;
}

// Otherwise replace attributes in state
return {
...state,
[ action.clientId ]: nextAttributes,
};

case 'INSERT_BLOCKS':
return {
...state,
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlockAttributes( action.blocks ),
};

case 'REPLACE_BLOCKS':
Expand All @@ -418,34 +536,11 @@ export const editor = flow( [

return {
...omit( state, action.clientIds ),
...getFlattenedBlocks( action.blocks ),
...getFlattenedBlockAttributes( action.blocks ),
};

case 'REMOVE_BLOCKS':
return omit( state, action.clientIds );

case 'SAVE_REUSABLE_BLOCK_SUCCESS': {
const { id, updatedId } = action;

// If a temporary reusable block is saved, we swap the temporary id with the final one
if ( id === updatedId ) {
return state;
}

return mapValues( state, ( block ) => {
if ( block.name === 'core/block' && block.attributes.ref === id ) {
return {
...block,
attributes: {
...block.attributes,
ref: updatedId,
},
};
}

return block;
} );
}
}

return state;
Expand Down
30 changes: 17 additions & 13 deletions packages/editor/src/store/selectors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -642,7 +642,7 @@ export const getBlockAttributes = createSelector(
return null;
}

let { attributes } = block;
let attributes = state.editor.present.blocks.attributes[ clientId ];

// Inject custom source attribute values.
//
Expand All @@ -667,6 +667,7 @@ export const getBlockAttributes = createSelector(
},
( state, clientId ) => [
state.editor.present.blocks.byClientId[ clientId ],
state.editor.present.blocks.attributes[ clientId ],
state.editor.present.edits.meta,
state.initialEdits.meta,
state.currentPost.meta,
Expand Down Expand Up @@ -698,9 +699,8 @@ export const getBlock = createSelector(
};
},
( state, clientId ) => [
state.editor.present.blocks.byClientId[ clientId ],
getBlockDependantsCacheBust( state, clientId ),
...getBlockAttributes.getDependants( state, clientId ),
getBlockDependantsCacheBust( state, clientId ),
]
);

Expand Down Expand Up @@ -729,9 +729,14 @@ export const getBlocks = createSelector(
( clientId ) => getBlock( state, clientId )
);
},
( state ) => [
state.editor.present.blocks,
]
( state, rootClientId ) => {
if ( ! rootClientId ) {
return [
state.editor.present.blocks,
];
}
return getBlock.getDependants( state, rootClientId );
}
);

/**
Expand Down Expand Up @@ -1125,10 +1130,8 @@ export const getMultiSelectedBlocks = createSelector(
return multiSelectedBlockClientIds.map( ( clientId ) => getBlock( state, clientId ) );
},
( state ) => [
state.editor.present.blocks.order,
state.blockSelection.start,
state.blockSelection.end,
state.editor.present.blocks.byClientId,
...getMultiSelectedBlockClientIds.getDependants( state ),
state.editor.present.blocks,
state.editor.present.edits.meta,
state.initialEdits.meta,
state.currentPost.meta,
Expand Down Expand Up @@ -1178,7 +1181,7 @@ const isAncestorOf = createSelector(
return possibleAncestorId === idToCheck;
},
( state ) => [
state.editor.present.blocks,
state.editor.present.blocks.order,
],
);

Expand Down Expand Up @@ -1985,7 +1988,8 @@ export const getInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.byClientId,
state.editor.present.blocks.order,
state.preferences.insertUsage,
state.settings.allowedBlockTypes,
state.settings.templateLock,
Expand Down Expand Up @@ -2019,7 +2023,7 @@ export const hasInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.byClientId,
state.settings.allowedBlockTypes,
state.settings.templateLock,
state.reusableBlocks.data,
Expand Down
Loading

0 comments on commit 99f8a2b

Please sign in to comment.