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

Lodash: Refactor block editor away from _.map() #47214

Merged
merged 1 commit into from
Jan 19, 2023
Merged

Lodash: Refactor block editor away from _.map() #47214

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
12 changes: 3 additions & 9 deletions packages/block-editor/src/components/block-settings-menu-controls/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { map } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -40,10 +35,9 @@ const BlockSettingsMenuControlsSlot = ( {
const ids =
clientIds !== null ? clientIds : getSelectedBlockClientIds();
return {
selectedBlocks: map(
getBlocksByClientId( ids ).filter( Boolean ),
( block ) => block.name
),
selectedBlocks: getBlocksByClientId( ids )
.filter( Boolean )
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array since we're calling it on a result of Array.prototype.filter().

.map( ( block ) => block.name ),
selectedClientIds: ids,
canRemove: canRemoveBlocks( ids ),
};
Expand Down
7 changes: 3 additions & 4 deletions packages/block-editor/src/components/inserter/block-types-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { map, groupBy } from 'lodash';
import { groupBy } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -113,7 +113,7 @@ export function BlockTypesTab( {
</InserterPanel>
) }

{ map( currentlyRenderedCategories, ( category ) => {
{ currentlyRenderedCategories.map( ( category ) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array:

gutenberg/packages/compose/src/hooks/use-async-list/index.ts

Line 47 in ecad0f0

const [ current, setCurrent ] = useState( [] as T[] );

const categoryItems = itemsPerCategory[ category.slug ];
if ( ! categoryItems || ! categoryItems.length ) {
return null;
Expand Down Expand Up @@ -148,8 +148,7 @@ export function BlockTypesTab( {
</InserterPanel>
) }

{ map(
currentlyRenderedCollections,
{ currentlyRenderedCollections.map(
Copy link
Member Author

Choose a reason for hiding this comment

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

Same as above.

( [ namespace, collection ] ) => {
const collectionItems = itemsPerCollection[ namespace ];
if ( ! collectionItems || ! collectionItems.length ) {
Expand Down
7 changes: 1 addition & 6 deletions packages/block-editor/src/components/inserter/hooks/use-patterns-state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { map } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -41,7 +36,7 @@ const usePatternsState = ( onInsert, rootClientId ) => {
const { createSuccessNotice } = useDispatch( noticesStore );
const onClickPattern = useCallback( ( pattern, blocks ) => {
onInsert(
map( blocks, ( block ) => cloneBlock( block ) ),
( blocks ?? [] ).map( ( block ) => cloneBlock( block ) ),
Copy link
Member Author

Choose a reason for hiding this comment

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

Just in case the click handler gets called with a nullish value.

pattern.name
);
createSuccessNotice(
Expand Down
7 changes: 1 addition & 6 deletions packages/block-editor/src/components/url-popover/image-url-input-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { map } from 'lodash';

/**
* WordPress dependencies
*/
Expand Down Expand Up @@ -260,7 +255,7 @@ const ImageURLInputUI = ( {
additionalControls={
! linkEditorValue && (
<NavigableMenu>
{ map( getLinkDestinations(), ( link ) => (
{ getLinkDestinations().map( ( link ) => (
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array:

gutenberg/packages/block-editor/src/components/url-popover/image-url-input-ui.js

Line 161 in d343bd5

const linkDestinations = [

<MenuItem
key={ link.linkDestination }
icon={ link.icon }
Expand Down
21 changes: 9 additions & 12 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import { map } from 'lodash';
import createSelector from 'rememo';

/**
Expand Down Expand Up @@ -202,7 +201,7 @@ export const __unstableGetClientIdWithClientIdsTree = createSelector(
*/
export const __unstableGetClientIdsTree = createSelector(
( state, rootClientId = '' ) =>
map( getBlockOrder( state, rootClientId ), ( clientId ) =>
getBlockOrder( state, rootClientId ).map( ( clientId ) =>
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array:

gutenberg/packages/block-editor/src/store/selectors.js

Line 1130 in 71a3344

return state.blocks.order.get( rootClientId || '' ) || EMPTY_ARRAY;

__unstableGetClientIdWithClientIdsTree( state, clientId )
),
( state ) => [ state.blocks.order ]
Expand Down Expand Up @@ -314,13 +313,11 @@ export const __experimentalGetGlobalBlocksByName = createSelector(
*/
export const getBlocksByClientId = createSelector(
( state, clientIds ) =>
map(
Array.isArray( clientIds ) ? clientIds : [ clientIds ],
( Array.isArray( clientIds ) ? clientIds : [ clientIds ] ).map(
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array as guaranteed by the ternary.

Copy link
Contributor

Choose a reason for hiding this comment

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

Offhand comment: fundamental selectors like this one really shouldn't be doing this kind of magic; they get called so often that they should be as simple and do as little work as possible (including instancing new things). Out of scope for this PR, though, your change looks good 👍

( clientId ) => getBlock( state, clientId )
),
( state, clientIds ) =>
map(
Array.isArray( clientIds ) ? clientIds : [ clientIds ],
( Array.isArray( clientIds ) ? clientIds : [ clientIds ] ).map(
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array as guaranteed by the ternary.

( clientId ) => state.blocks.tree.get( clientId )
)
);
Expand Down Expand Up @@ -507,18 +504,18 @@ export const getBlockParents = createSelector(
export const getBlockParentsByBlockName = createSelector(
( state, clientId, blockName, ascending = false ) => {
const parents = getBlockParents( state, clientId, ascending );
return map(
map( parents, ( id ) => ( {
return parents
Copy link
Member Author

Choose a reason for hiding this comment

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

Always an array:

gutenberg/packages/block-editor/src/store/selectors.js

Lines 481 to 488 in 71a3344

const parents = [];
let current = clientId;
while ( !! state.blocks.parents.get( current ) ) {
current = state.blocks.parents.get( current );
parents.push( current );
}
return ascending ? parents : parents.reverse();

.map( ( id ) => ( {
id,
name: getBlockName( state, id ),
} ) ).filter( ( { name } ) => {
} ) )
.filter( ( { name } ) => {
if ( Array.isArray( blockName ) ) {
return blockName.includes( name );
}
return name === blockName;
} ),
( { id } ) => id
);
} )
.map( ( { id } ) => id );
Copy link
Member Author

Choose a reason for hiding this comment

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

Already an array as guaranteed by the previous calls.

},
( state ) => [ state.blocks.parents ]
);
Expand Down
7 changes: 1 addition & 6 deletions packages/block-editor/src/utils/transform-styles/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { map } from 'lodash';

/**
* WordPress dependencies
*/
Expand All @@ -23,7 +18,7 @@ import wrap from './transforms/wrap';
* @return {Array} converted rules.
*/
const transformStyles = ( styles, wrapperClassName = '' ) => {
return map( styles, ( { css, baseURL } ) => {
return Object.values( styles ?? [] ).map( ( { css, baseURL } ) => {
Copy link
Member Author

Choose a reason for hiding this comment

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

Can be nullish, which is why the nullish coalescing:

gutenberg/packages/block-library/src/html/preview.js

Line 25 in 5e2850d

return select( blockEditorStore ).getSettings()?.styles;

And we're doing the Object.values() because there are instances where we pass an object:

gutenberg/packages/block-editor/src/hooks/font-size.js

Line 273 in 7973d9c

const activeSupports = {

although most of the rest of the usages pass an array of styles. Object.values() will correctly yield us the values for arrays, so we're good with that part, as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm curious, why did you go for [] instead of {}? They both work fine, in hindsight, I just found it surprising at first.

Copy link
Member Author

Choose a reason for hiding this comment

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

Because various usages of transformStyles() across the codebase work with either arrays or objects of styles, and I had to pick one of them 😉 Since we're doing a transformation to an array anyway, it felt better to just default to an array, regardless of the fact that we're doing an Object.values() on it. Also, the function is already documented to expect an array of styles, so we're matching that. Regardless, I think the fact that it works with both objects and arrays is a bit vague and we could definitely improve that in the future.

const transforms = [];
if ( wrapperClassName ) {
transforms.push( wrap( wrapperClassName ) );
Expand Down