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

Block editor: optimise getGlobalBlockCount/getClientIdsWithDescendants #58356

Merged
merged 2 commits into from
Jan 30, 2024
+35 −29
Merged

Block editor: optimise getGlobalBlockCount/getClientIdsWithDescendants #58356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
10406aa
Rewrite
ellatrix Jan 28, 2024
7a98afa
Fix tests
ellatrix Jan 28, 2024
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
2 changes: 1 addition & 1 deletion docs/reference-guides/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -486,7 +486,7 @@ Returns an array containing the clientIds of all descendants of the blocks given
_Parameters_

- _state_ `Object`: Global application state.
- _clientIds_ `string|string[]`: Client ID(s) for which descendant blocks are to be returned.
- _rootIds_ `string|string[]`: Client ID(s) for which descendant blocks are to be returned.

_Returns_

Expand Down
62 changes: 34 additions & 28 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 @@ -252,24 +252,37 @@ export const __unstableGetClientIdsTree = createSelector(
* given. Returned ids are ordered first by the order of the ids given, then
* by the order that they appear in the editor.
*
* @param {Object} state Global application state.
* @param {string|string[]} clientIds Client ID(s) for which descendant blocks are to be returned.
* @param {Object} state Global application state.
* @param {string|string[]} rootIds Client ID(s) for which descendant blocks are to be returned.
*
* @return {Array} Client IDs of descendants.
*/
export const getClientIdsOfDescendants = createSelector(
( state, clientIds ) => {
const givenIds = Array.isArray( clientIds ) ? clientIds : [ clientIds ];
const collectedIds = [];
for ( const givenId of givenIds ) {
for ( const descendantId of getBlockOrder( state, givenId ) ) {
collectedIds.push(
descendantId,
...getClientIdsOfDescendants( state, descendantId )
);
( state, rootIds ) => {
rootIds = Array.isArray( rootIds ) ? [ ...rootIds ] : [ rootIds ];
const ids = [];

// Add the descendants of the root blocks first.
for ( const rootId of rootIds ) {
const order = state.blocks.order.get( rootId );
if ( order ) {
ids.push( ...order );
}
}

let index = 0;

// Add the descendants of the descendants, recursively.
while ( index < ids.length ) {
Copy link
Member

Choose a reason for hiding this comment

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

I think using a stack (while ( id = stack.pop() )) for this would be easier to grok. But I suppose you'd have to push items onto the stack in reverse order which is probably slow.

Carry on 👍

const id = ids[ index ];
const order = state.blocks.order.get( id );
if ( order ) {
ids.splice( index + 1, 0, ...order );
}
index++;
Copy link
Member

Choose a reason for hiding this comment

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

what's the role of index here? it doesn't seem to be tracking the end of the array, but it looks like it could intermix the ids returned in order from different descendants.

we don't want to push these to the end of the array, right? if we insert 5 descendant ids, do we want to add the next descendants inside of those, or do we want to increment index by 5 or 6 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

We want to also search all the newly added clientIds for descendants, so we shouldn't increment. index just keeps increasing until we looped over all descendants and nested descendants.

Copy link
Member

Choose a reason for hiding this comment

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

so it doesn't matter in which order we add the descendent ids to the list?
did you consider using ids.push( ...order )?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does matter :) I used push at first, but the tests were expecting the ids in a certain order (descendants right after parent ID vs after all the parent siblings

Copy link
Member

Choose a reason for hiding this comment

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

okay I'm confused then, because it looks like we only get descendants immediately after their parents if there are only one descendant per parent

Copy link
Member Author

@ellatrix ellatrix Jan 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's deeply adding all descendants, we keep expanding the array as we find more, so the loop will also include those newly added descendants

}
return collectedIds;

return ids;
},
( state ) => [ state.blocks.order ]
);
Expand All @@ -283,19 +296,8 @@ export const getClientIdsOfDescendants = createSelector(
*
* @return {Array} ids of top-level and descendant blocks.
*/
export const getClientIdsWithDescendants = createSelector(
( state ) => {
const collectedIds = [];
for ( const topLevelId of getBlockOrder( state ) ) {
collectedIds.push(
topLevelId,
...getClientIdsOfDescendants( state, topLevelId )
);
}
return collectedIds;
},
( state ) => [ state.blocks.order ]
);
export const getClientIdsWithDescendants = ( state ) =>
Copy link
Member

@noisysocks noisysocks Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Not strictly related to your changes, but I've always thought it's awkward when writing application code that we have both of these functions. They're named similarly and I can never remember which is which. Could we deprecate getClientIdsWithDescendants and make it so that getClientIdsOfDescendants defaults to rootIds = ''?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, it's really weird that we have both of these functions, I can deprecate it in a follow-up

getClientIdsOfDescendants( state, '' );

/**
* Returns the total number of blocks, or the total number of blocks with a specific name in a post.
Expand All @@ -312,10 +314,14 @@ export const getGlobalBlockCount = createSelector(
if ( ! blockName ) {
return clientIds.length;
}
return clientIds.reduce( ( accumulator, clientId ) => {
let count = 0;
for ( const clientId of clientIds ) {
const block = state.blocks.byClientId.get( clientId );
return block.name === blockName ? accumulator + 1 : accumulator;
}, 0 );
if ( block.name === blockName ) {
count++;
}
}
return count;
},
( state ) => [ state.blocks.order, state.blocks.byClientId ]
);
Expand Down
Loading