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

Track the parent block to optimize hierarchy selectors #16392

Merged
merged 9 commits into from
Jul 5, 2019
Merged
Show file tree
Hide file tree
Changes from 1 commit
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
71 changes: 71 additions & 0 deletions packages/block-editor/src/store/reducer.js
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,25 @@ function mapBlockOrder( blocks, rootClientId = '' ) {
return result;
}

/**
* Given an array of blocks, returns an object where each key contains
* the clientId of the block and the value is the parent of the block.
*
* @param {Array} blocks Blocks to map.
* @param {?string} rootClientId Assumed root client ID.
*
* @return {Object} Block order map object.
*/
function mapBlockParents( blocks, rootClientId = '' ) {
const result = {};
blocks.forEach( ( block ) => {
result[ block.clientId ] = rootClientId;
Object.assign( result, mapBlockParents( block.innerBlocks, block.clientId ) );
} );

return result;
}

/**
* Helper method to iterate through all blocks, recursing into inner blocks,
* applying a transformation function to each one.
Expand Down Expand Up @@ -306,6 +325,10 @@ const withBlockReset = ( reducer ) => ( state, action ) => {
...omit( state.order, visibleClientIds ),
...mapBlockOrder( action.blocks ),
},
parents: {
...omit( state.parents, visibleClientIds ),
...mapBlockParents( action.blocks ),
},
};
}

Expand Down Expand Up @@ -656,6 +679,54 @@ export const blocks = flow(

return state;
},

// This is the opposite of the order property.
// It's duplicated data used for performance reasons.
parents( state = {}, action ) {
switch ( action.type ) {
case 'RESET_BLOCKS':
return mapBlockParents( action.blocks );

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

case 'INSERT_BLOCKS':
return {
...state,
...mapBlockParents( action.blocks, action.rootClientId || '' ),
};

case 'MOVE_BLOCK_TO_POSITION': {
return {
...state,
[ action.clientId ]: action.toRootClientId || '',
};
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor: unneeded block around return statement.


case 'REPLACE_BLOCKS':
return {
// This should include the parents as well to avoid memory leak
...omit( state, action.clientIds ),
...mapBlockParents( action.blocks, state[ action.clientIds[ 0 ] ] ),
Copy link
Member

Choose a reason for hiding this comment

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

Re: My previous comment, I guess we still reference the original clientIds here. Can you clarify what you're doing here in referencing only the first of the original of the clientIds ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Basically we need the parent of the replaced blocks to assign it as parent of the inserted blocks. replacedClientIds contains more than just the "root" level which in theory could result in the wrong value here.

};

case 'REMOVE_BLOCKS':
// This should include the parents as well to avoid memory leak
return omit( state, action.clientIds );

case 'REPLACE_INNER_BLOCKS':
return {
// Inner blocks removed should be omitted from the state
...state,
...mapBlockParents( action.blocks, action.rootClientId ),
};
}

return state;
},
} );

/**
Expand Down
45 changes: 14 additions & 31 deletions packages/block-editor/src/store/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -458,22 +458,11 @@ export function getSelectedBlock( state ) {
*
* @return {?string} Root client ID, if exists
*/
export const getBlockRootClientId = createSelector(
( state, clientId ) => {
const { order } = state.blocks;

for ( const rootClientId in order ) {
if ( includes( order[ rootClientId ], clientId ) ) {
return rootClientId;
}
}

return null;
},
( state ) => [
state.blocks.order,
]
);
export const getBlockRootClientId = ( state, clientId ) => {
return state.blocks.parents[ clientId ] !== undefined ?
state.blocks.parents[ clientId ] :
null;
};

/**
* Given a block client ID, returns the root of the hierarchy from which the block is nested, return the block itself for root level blocks.
Expand All @@ -483,21 +472,15 @@ export const getBlockRootClientId = createSelector(
*
* @return {string} Root client ID
*/
export const getBlockHierarchyRootClientId = createSelector(
( state, clientId ) => {
let rootClientId = clientId;
let current = clientId;
while ( rootClientId ) {
current = rootClientId;
rootClientId = getBlockRootClientId( state, current );
}

return current;
},
( state ) => [
state.blocks.order,
]
);
export const getBlockHierarchyRootClientId = ( state, clientId ) => {
let current = clientId;
let parent;
do {
parent = current;
current = state.blocks.parents[ current ];
} while ( current );
return parent;
};

/**
* Returns the client ID of the block adjacent one at the given reference
Expand Down