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.

Sign up for GitHub

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

Extract the attributes from the editor's state shape to enhance performance #12268

Closed
wants to merge 1 commit into from
Closed

Extract the attributes from the editor's state shape to enhance performance #12268

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
118 changes: 99 additions & 19 deletions packages/editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,12 +106,38 @@ function getFlattenedBlocks( blocks ) {

stack.push( ...innerBlocks );

flattenedBlocks[ block.clientId ] = block;
flattenedBlocks[ block.clientId ] = omit( block, [ 'attributes' ] );
}

return flattenedBlocks;
}

/**
* 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 block attributes object.
*
* @param {Array} blocks Blocks to flatten.
*
* @return {Object} Flattened block attributes object.
*/
function getFlattenedBlockAttributes( blocks ) {
const flattenedBlockAttributes = {};

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 );

flattenedBlockAttributes[ block.clientId ] = block.attributes || {};
}

return flattenedBlockAttributes;
}
Copy link
Member

Choose a reason for hiding this comment

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

getFlattenedBlocks and getFlattenedBlockAttributes are very similar. They're both functions that flatten a block tree into a hash map. The only difference is what is stored in the map. Could we use a higher order function that leaves it up to the caller to decide what is stored in the map?

function flattenBlocks( blocks, iteratee ) {
	const result = {};

	const stack = [ ...blocks ];
	while ( stack.length ) {
		const { innerBlocks, ...block } = stack.shift();
		stack.push( ...innerBlocks );
		result[ block.clientId ] = iteratee( block );
	}

	return result;
}

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it would be much cleaner 👍


/**
* Given a block order map object, returns *all* of the block client IDs that are
* a descendant of the given root client ID.
Expand Down Expand Up @@ -263,6 +289,10 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
...omit( state.byClientId, visibleClientIds ),
...getFlattenedBlocks( action.blocks ),
},
attributesByClientId: {
...omit( state.attributesByClientId, visibleClientIds ),
...getFlattenedBlockAttributes( action.blocks ),
},
order: {
...omit( state.order, visibleClientIds ),
...mapBlockOrder( action.blocks ),
Expand Down Expand Up @@ -369,6 +399,60 @@ export const editor = flow( [
...getFlattenedBlocks( action.blocks ),
};

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

const changes = omit( action.updates, 'attributes' );
if ( keys( changes ).length === 0 ) {
Copy link
Member

Choose a reason for hiding this comment

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

nit: We could use _.isEmpty() here, though not sure how it affects performance.

if ( isEmpty( changes ) ) {

return state;
}

return {
...state,
[ action.clientId ]: {
...state[ action.clientId ],
...omit( action.updates, 'attributes' ),
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to call omit() again here.

Suggested change
...omit( action.updates, 'attributes' ),
...changes,

},
};

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

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

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

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

return state;
},

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

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

case 'UPDATE_BLOCK_ATTRIBUTES':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
Expand All @@ -383,41 +467,39 @@ export const editor = flow( [
}

return result;
}, state[ action.clientId ].attributes );
}, 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 ].attributes ) {
if ( nextAttributes === state[ action.clientId ] ) {
return state;
}

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

// TODO: Check if this action is necessary
case 'UPDATE_BLOCK':
// Ignore updates if block isn't known
if ( ! state[ action.clientId ] ) {
if ( ! state[ action.clientId ] || ! action.updates.attributes ) {
return state;
}

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

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

case 'REPLACE_BLOCKS':
Expand All @@ -427,7 +509,7 @@ export const editor = flow( [

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

case 'REMOVE_BLOCKS':
Expand All @@ -441,18 +523,16 @@ export const editor = flow( [
return state;
}

return mapValues( state, ( block ) => {
if ( block.name === 'core/block' && block.attributes.ref === id ) {
// TODO fix this by moving this to a higher level (we need the block name)
return mapValues( state, ( attributes ) => {
if ( attributes.ref && attributes.ref === id ) {
return {
...block,
attributes: {
...block.attributes,
ref: updatedId,
},
...attributes,
ref: updatedId,
};
}

return block;
return attributes;
} );
}
}
Expand Down
13 changes: 8 additions & 5 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -629,8 +629,7 @@ export const getBlock = createSelector(
if ( ! block ) {
return null;
}

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

// Inject custom source attribute values.
//
Expand Down Expand Up @@ -659,6 +658,7 @@ export const getBlock = createSelector(
},
( state, clientId ) => [
state.editor.present.blocks.byClientId[ clientId ],
state.editor.present.blocks.attributesByClientId[ clientId ],
getBlockDependantsCacheBust( state, clientId ),
state.editor.present.edits.meta,
state.initialEdits.meta,
Expand Down Expand Up @@ -1090,6 +1090,7 @@ export const getMultiSelectedBlocks = createSelector(
state.editor.present.blocks.order,
state.blockSelection.start,
state.blockSelection.end,
state.editor.present.blocks.attributesByClientId,
state.editor.present.blocks.byClientId,
state.editor.present.edits.meta,
state.initialEdits.meta,
Expand Down Expand Up @@ -1140,7 +1141,7 @@ const isAncestorOf = createSelector(
return possibleAncestorId === idToCheck;
},
( state ) => [
state.editor.present.blocks,
state.editor.present.blocks.order,
],
);

Expand Down Expand Up @@ -1947,7 +1948,8 @@ export const getInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.byClientId,
state.editor.present.blocks.order,
Copy link
Member

Choose a reason for hiding this comment

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

Nice —this should help a lot! 🔥

Copy link
Member

Choose a reason for hiding this comment

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

Does order matter for the inserter items? Can we reason differently about nested blocks where it could have an impact? Do I miss anything?

state.preferences.insertUsage,
state.settings.allowedBlockTypes,
state.settings.templateLock,
Expand Down Expand Up @@ -1981,7 +1983,8 @@ export const hasInserterItems = createSelector(
},
( state, rootClientId ) => [
state.blockListSettings[ rootClientId ],
state.editor.present.blocks,
state.editor.present.blocks.order,
state.editor.present.blocks.byClientId,
Copy link
Member

Choose a reason for hiding this comment

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

The same about the order as above.

state.settings.allowedBlockTypes,
state.settings.templateLock,
state.reusableBlocks.data,
Expand Down