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

Lodash: Remove from block factory API #43560

Merged
merged 1 commit into from Aug 24, 2022
Merged

Lodash: Remove from block factory API #43560

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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
52 changes: 21 additions & 31 deletions packages/blocks/src/api/factory.js
View file
Open in desktop
Expand Up @@ -2,16 +2,6 @@
* External dependencies
*/
import { v4 as uuid } from 'uuid';
import {
every,
castArray,
some,
filter,
first,
has,
isEmpty,
map,
} from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -163,14 +153,14 @@ export function cloneBlock( block, mergeAttributes = {}, newInnerBlocks ) {
* @return {boolean} Is the transform possible?
*/
const isPossibleTransformForSource = ( transform, direction, blocks ) => {
if ( isEmpty( blocks ) ) {
if ( ! blocks.length ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Adding a note that blocks could in theory be undefined (in which case blocks?.length would be preferred), but the rest of the code on this file already appears to assume that it will always be an array 👍

Copy link
Member

Choose a reason for hiding this comment

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

Tests pass so it’s probably working correctly but extra safety won’t hurt. I believe that isEmpty like most lodash methods was covering a few things including checking if we deal with an array in the first place.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, this is a common problem with migrating away from Lodash, especially in untyped codebases 😕 It's hard to separate which checks we want to keep, and which ones are not needed. Implicit conversions are even worse.

Sometimes you get it wrong and create some bugs that need to be fixed, but the end result after all that is much better: not only are you not relying on a large lib anymore, but you're now explicit about your checks, which improves readability and maintainability. It's painful, but it's worth it 🙂

Copy link
Member Author

Choose a reason for hiding this comment

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

TBH cases like isEmpty in combination with the untyped codebase makes it the ultimate hell for following through code and ensuring no bugs occur. By removing the extra ambiguity that isEmpty() added, we solve one of the problems, and then when one day we type the entire codebase, things will look great!

return false;
}

// If multiple blocks are selected, only multi block transforms
// or wildcard transforms are allowed.
const isMultiBlock = blocks.length > 1;
const firstBlockName = first( blocks ).name;
const firstBlockName = blocks[ 0 ].name;
const isValidForMultiBlocks =
isWildcardBlockTransform( transform ) ||
! isMultiBlock ||
Expand All @@ -183,7 +173,7 @@ const isPossibleTransformForSource = ( transform, direction, blocks ) => {
// for a block selection of multiple blocks of different types.
if (
! isWildcardBlockTransform( transform ) &&
! every( blocks, { name: firstBlockName } )
! blocks.every( ( block ) => block.name === firstBlockName )
) {
return false;
}
Expand All @@ -196,7 +186,7 @@ const isPossibleTransformForSource = ( transform, direction, blocks ) => {

// Check if the transform's block name matches the source block (or is a wildcard)
// only if this is a transform 'from'.
const sourceBlock = first( blocks );
const sourceBlock = blocks[ 0 ];
const hasMatchingName =
direction !== 'from' ||
transform.blocks.indexOf( sourceBlock.name ) !== -1 ||
Expand Down Expand Up @@ -241,15 +231,14 @@ const isPossibleTransformForSource = ( transform, direction, blocks ) => {
* @return {Array} Block types that the blocks can be transformed into.
*/
const getBlockTypesForPossibleFromTransforms = ( blocks ) => {
if ( isEmpty( blocks ) ) {
if ( ! blocks.length ) {
return [];
}

const allBlockTypes = getBlockTypes();

// filter all blocks to find those with a 'from' transform.
const blockTypesWithPossibleFromTransforms = filter(
allBlockTypes,
const blockTypesWithPossibleFromTransforms = allBlockTypes.filter(
( blockType ) => {
const fromTransforms = getBlockTransforms( 'from', blockType.name );
return !! findTransform( fromTransforms, ( transform ) => {
Expand All @@ -274,18 +263,18 @@ const getBlockTypesForPossibleFromTransforms = ( blocks ) => {
* @return {Array} Block types that the source can be transformed into.
*/
const getBlockTypesForPossibleToTransforms = ( blocks ) => {
if ( isEmpty( blocks ) ) {
if ( ! blocks.length ) {
return [];
}

const sourceBlock = first( blocks );
const sourceBlock = blocks[ 0 ];
const blockType = getBlockType( sourceBlock.name );
const transformsTo = blockType
? getBlockTransforms( 'to', blockType.name )
: [];

// filter all 'to' transforms to find those that are possible.
const possibleTransforms = filter( transformsTo, ( transform ) => {
const possibleTransforms = transformsTo.filter( ( transform ) => {
return (
transform && isPossibleTransformForSource( transform, 'to', blocks )
);
Expand Down Expand Up @@ -338,7 +327,7 @@ export const isContainerGroupBlock = ( name ) =>
* @return {Array} Block types that the blocks argument can be transformed to.
*/
export function getPossibleBlockTransformations( blocks ) {
if ( isEmpty( blocks ) ) {
if ( ! blocks.length ) {
return [];
}

Expand Down Expand Up @@ -418,7 +407,7 @@ export function getBlockTransforms( direction, blockTypeOrName ) {
transforms.supportedMobileTransforms &&
Array.isArray( transforms.supportedMobileTransforms );
const filteredTransforms = usingMobileTransformations
? filter( transforms[ direction ], ( t ) => {
? transforms[ direction ].filter( ( t ) => {
if ( t.type === 'raw' ) {
return true;
}
Expand All @@ -431,7 +420,7 @@ export function getBlockTransforms( direction, blockTypeOrName ) {
return true;
}

return every( t.blocks, ( transformBlockName ) =>
return t.blocks.every( ( transformBlockName ) =>
transforms.supportedMobileTransforms.includes(
transformBlockName
)
Expand Down Expand Up @@ -459,7 +448,7 @@ function maybeCheckTransformIsMatch( transform, blocks ) {
if ( typeof transform.isMatch !== 'function' ) {
return true;
}
const sourceBlock = first( blocks );
const sourceBlock = blocks[ 0 ];
const attributes = transform.isMultiBlock
? blocks.map( ( block ) => block.attributes )
: sourceBlock.attributes;
Expand All @@ -477,7 +466,7 @@ function maybeCheckTransformIsMatch( transform, blocks ) {
* @return {?Array} Array of blocks or null.
*/
export function switchToBlockType( blocks, name ) {
const blocksArray = castArray( blocks );
const blocksArray = Array.isArray( blocks ) ? blocks : [ blocks ];
const isMultiBlock = blocksArray.length > 1;
const firstBlock = blocksArray[ 0 ];
const sourceName = firstBlock.name;
Expand Down Expand Up @@ -514,7 +503,7 @@ export function switchToBlockType( blocks, name ) {
let transformationResults;

if ( transformation.isMultiBlock ) {
if ( has( transformation, '__experimentalConvert' ) ) {
if ( '__experimentalConvert' in transformation ) {
transformationResults =
transformation.__experimentalConvert( blocksArray );
} else {
Expand All @@ -523,7 +512,7 @@ export function switchToBlockType( blocks, name ) {
blocksArray.map( ( currentBlock ) => currentBlock.innerBlocks )
);
}
} else if ( has( transformation, '__experimentalConvert' ) ) {
} else if ( '__experimentalConvert' in transformation ) {
transformationResults =
transformation.__experimentalConvert( firstBlock );
} else {
Expand All @@ -544,7 +533,9 @@ export function switchToBlockType( blocks, name ) {

// If the transformation function returned a single object, we want to work
// with an array instead.
transformationResults = castArray( transformationResults );
transformationResults = Array.isArray( transformationResults )
? transformationResults
: [ transformationResults ];

// Ensure that every block object returned by the transformation has a
// valid block type.
Expand All @@ -562,8 +553,7 @@ export function switchToBlockType( blocks, name ) {
return transformationResults;
}

const hasSwitchedBlock = some(
transformationResults,
const hasSwitchedBlock = transformationResults.some(
( result ) => result.name === name
);

Expand Down Expand Up @@ -608,7 +598,7 @@ export const getBlockFromExample = ( name, example ) => {
return createBlock(
name,
example.attributes,
map( example.innerBlocks, ( innerBlock ) =>
( example.innerBlocks ?? [] ).map( ( innerBlock ) =>
getBlockFromExample( innerBlock.name, innerBlock )
)
);
Expand Down