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

Jump to bottom

Track the parent block to optimize hierarchy selectors #16392

Merged
merged 9 commits into from
Jul 5, 2019
Merged

Track the parent block to optimize hierarchy selectors #16392

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
f1726fb
Track the parent block to optimize hierarchy selectors
youknowriad Jul 2, 2019
2d3ea3a
Fix unit tests
youknowriad Jul 2, 2019
705d0fd
Changes per review
youknowriad Jul 2, 2019
afefd02
Avoid memory leaks in the parent reducer
youknowriad Jul 2, 2019
b21c2bf
Add unit tests for the parents reducer
youknowriad Jul 2, 2019
9d97b1b
Fix all memory leaks
youknowriad Jul 3, 2019
e05ab9e
Changes per review
youknowriad Jul 4, 2019
6087e69
Change action type for clarity
youknowriad Jul 5, 2019
3002180
Use string keys in unit test
youknowriad Jul 5, 2019
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
102 changes: 90 additions & 12 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,23 @@ 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 = '' ) {
return blocks.reduce( ( result, block ) => Object.assign(
result,
{ [ block.clientId ]: rootClientId },
mapBlockParents( block.innerBlocks, block.clientId )
), {} );
}

/**
* Helper method to iterate through all blocks, recursing into inner blocks,
* applying a transformation function to each one.
Expand Down Expand Up @@ -264,16 +281,34 @@ function withIgnoredBlockChange( reducer ) {
* @return {Function} Enhanced reducer function.
*/
const withInnerBlocksRemoveCascade = ( reducer ) => ( state, action ) => {
if ( state && action.type === 'REMOVE_BLOCKS' ) {
const clientIds = [ ...action.clientIds ];
const getAllChildren = ( clientIds ) => {
const result = [ ...clientIds ];
youknowriad marked this conversation as resolved.
Show resolved Hide resolved

// For each removed client ID, include its inner blocks to remove,
// recursing into those so long as inner blocks exist.
for ( let i = 0; i < clientIds.length; i++ ) {
clientIds.push( ...state.order[ clientIds[ i ] ] );
for ( let i = 0; i < result.length; i++ ) {
if ( state.order[ result[ i ] ] ) {
Copy link
Member

Choose a reason for hiding this comment

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

Is this condition related to your comment at #16392 (comment) with regards to orphaned references? Should we comment it as a temporary fix if it is meant to be temporary?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no, to be completely honest, I tried but failed to find why this condition is necessary. When you remove an "empty paragraph" using backspace, there's a JS error that is triggered if you remove this condition.

result.push( ...state.order[ result[ i ] ] );
}
}
return result;
};

action = { ...action, clientIds };
if ( state ) {
switch ( action.type ) {
case 'REMOVE_BLOCKS':
action = {
...action,
removedClientIds: getAllChildren( action.clientIds ),
Copy link
Member

Choose a reason for hiding this comment

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

Is there a specific reason we need to rename these properties from clientIds, rather than just "enhancing" the existing value to account for the cascade? Seems like it might make this a bit simpler to implement, since you wouldn't need the switch to vary the behavior.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've been caught off guard in several occasions because of this. The fact that the property don't contain the same value between reducers (selection and blocks), and also the difference between the action creator (which I tend to take a look at to understand the action) led me to this change.

It is still a different action but instead but I think the fact that we introduce a new key makes it less confusing.

Copy link
Contributor

Choose a reason for hiding this comment

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

I get the motivation for this. It could, however, be pretty jarring for someone who notices the discrepancy and pulls up the definition of removeBlocks, since it clearly only returns clientIds in its payload.

Perhaps update removeBlocks to make that relationship clearer?

// actions.js
yield {
	type: 'REMOVE_BLOCKS',
	clientIds,
	removedClientIds: undefined // Explain bla bla higher-order reducer
};

In alternative, if we're really strict about action types, it would be more "correct" to let this higher-order reducer divert from one action type to a new one:

// reducer.js
if ( state ) {
	switch ( action.type ) {
		case 'REMOVE_BLOCKS':
			action = {
				type: DERIVED_TYPE_SIMILAR_TO_REMOVE_BLOCKS,
				removedClientIds: …

Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, with controls implementation, these days we could probably incorporate this into the action creator itself, rather than as a reducer enhancer. That way the logic is consolidated to one place.

};
break;
case 'REPLACE_BLOCKS':
action = {
...action,
replacedClientIds: getAllChildren( action.clientIds ),
};
break;
}
}

return reducer( state, action );
Expand Down Expand Up @@ -306,6 +341,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 @@ -441,12 +480,12 @@ export const blocks = flow(
}

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

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

return state;
Expand Down Expand Up @@ -517,12 +556,12 @@ export const blocks = flow(
}

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

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

return state;
Expand Down Expand Up @@ -618,7 +657,7 @@ export const blocks = flow(
const mappedBlocks = mapBlockOrder( action.blocks );

return flow( [
( nextState ) => omit( nextState, clientIds ),
( nextState ) => omit( nextState, action.replacedClientIds ),
( nextState ) => ( {
...nextState,
...omit( mappedBlocks, '' ),
Expand All @@ -645,17 +684,56 @@ export const blocks = flow(
case 'REMOVE_BLOCKS':
return flow( [
// Remove inner block ordering for removed blocks
( nextState ) => omit( nextState, action.clientIds ),
( nextState ) => omit( nextState, action.removedClientIds ),

// Remove deleted blocks from other blocks' orderings
( nextState ) => mapValues( nextState, ( subState ) => (
without( subState, ...action.clientIds )
without( subState, ...action.removedClientIds )
) ),
] )( state );
}

return state;
},

// While technically redundant data as the inverse of `order`, it serves as
// an optimization for the selectors which derive the ancestry of a block.
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 {
...omit( state, action.replacedClientIds ),
...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':
return omit( state, action.removedClientIds );
}

return state;
},
} );

/**
Expand Down
45 changes: 14 additions & 31 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
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 ) => {
youknowriad marked this conversation as resolved.
Show resolved Hide resolved
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
Loading