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 away from _.cloneDeep() #43631

Merged
merged 2 commits into from
Aug 30, 2022
Merged

Lodash: Refactor away from _.cloneDeep() #43631

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
7c00ce0
Lodash: Refactor away from _.cloneDeep()
tyxla Aug 26, 2022
367e3ce
Document deep cloning to make the intent clearer
tyxla Aug 29, 2022
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
1 change: 1 addition & 0 deletions .eslintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,7 @@ module.exports = {
'capitalize',
'chunk',
'clamp',
'cloneDeep',
'compact',
'concat',
'countBy',
Expand Down
24 changes: 11 additions & 13 deletions packages/block-editor/src/components/block-list/test/block-list-context.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { cloneDeep } from 'lodash';

/**
* Internal dependencies
*/
Expand All @@ -19,6 +14,9 @@ import {
ROOT_LEVEL_ID,
} from './fixtures/block-list-context.native';

// Deep clone an object to avoid mutating it later.
const cloneObject = ( obj ) => JSON.parse( JSON.stringify( obj ) );

describe( 'findBlockLayoutByClientId', () => {
it( "finds a block's layout data at root level", () => {
const { findBlockLayoutByClientId } = DEFAULT_BLOCK_LIST_CONTEXT;
Expand Down Expand Up @@ -66,7 +64,7 @@ describe( 'findBlockLayoutByClientId', () => {
describe( 'deleteBlockLayoutByClientId', () => {
it( "deletes a block's layout data at root level", () => {
const { findBlockLayoutByClientId } = DEFAULT_BLOCK_LIST_CONTEXT;
const defaultBlockLayouts = cloneDeep( BLOCKS_LAYOUTS_DATA );
const defaultBlockLayouts = cloneObject( BLOCKS_LAYOUTS_DATA );
const currentBlockLayouts = deleteBlockLayoutByClientId(
defaultBlockLayouts,
ROOT_LEVEL_ID
Expand All @@ -82,7 +80,7 @@ describe( 'deleteBlockLayoutByClientId', () => {

it( "deletes a nested block's layout data with inner blocks", () => {
const { findBlockLayoutByClientId } = DEFAULT_BLOCK_LIST_CONTEXT;
const defaultBlockLayouts = cloneDeep( BLOCKS_LAYOUTS_DATA );
const defaultBlockLayouts = cloneObject( BLOCKS_LAYOUTS_DATA );
const currentBlockLayouts = deleteBlockLayoutByClientId(
defaultBlockLayouts,
NESTED_WITH_INNER_BLOCKS_ID
Expand All @@ -98,7 +96,7 @@ describe( 'deleteBlockLayoutByClientId', () => {

it( "deletes a deep nested block's layout data", () => {
const { findBlockLayoutByClientId } = DEFAULT_BLOCK_LIST_CONTEXT;
const defaultBlockLayouts = cloneDeep( BLOCKS_LAYOUTS_DATA );
const defaultBlockLayouts = cloneObject( BLOCKS_LAYOUTS_DATA );
const currentBlockLayouts = deleteBlockLayoutByClientId(
defaultBlockLayouts,
DEEP_NESTED_ID
Expand All @@ -120,7 +118,7 @@ describe( 'updateBlocksLayouts', () => {
findBlockLayoutByClientId,
updateBlocksLayouts,
} = DEFAULT_BLOCK_LIST_CONTEXT;
const currentBlockLayouts = cloneDeep( blocksLayouts );
const currentBlockLayouts = cloneObject( blocksLayouts );
const BLOCK_CLIENT_ID = PARAGRAPH_BLOCK_LAYOUT_DATA.clientId;

updateBlocksLayouts( currentBlockLayouts, PARAGRAPH_BLOCK_LAYOUT_DATA );
Expand All @@ -142,7 +140,7 @@ describe( 'updateBlocksLayouts', () => {
const { findBlockLayoutByClientId, updateBlocksLayouts } =
DEFAULT_BLOCK_LIST_CONTEXT;
const currentBlockLayouts = {
current: cloneDeep( BLOCKS_LAYOUTS_DATA ),
current: cloneObject( BLOCKS_LAYOUTS_DATA ),
};
const PARENT_BLOCK_CLIENT_ID = GROUP_BLOCK_LAYOUT_DATA.clientId;

Expand Down Expand Up @@ -181,7 +179,7 @@ describe( 'updateBlocksLayouts', () => {
const { findBlockLayoutByClientId, updateBlocksLayouts } =
DEFAULT_BLOCK_LIST_CONTEXT;
const currentBlockLayouts = {
current: cloneDeep( BLOCKS_LAYOUTS_DATA ),
current: cloneObject( BLOCKS_LAYOUTS_DATA ),
};

// Add block layout data to it's parents inner blocks
Expand All @@ -207,7 +205,7 @@ describe( 'updateBlocksLayouts', () => {
const { findBlockLayoutByClientId, updateBlocksLayouts } =
DEFAULT_BLOCK_LIST_CONTEXT;
const currentBlockLayouts = {
current: cloneDeep( BLOCKS_LAYOUTS_DATA ),
current: cloneObject( BLOCKS_LAYOUTS_DATA ),
};

updateBlocksLayouts( currentBlockLayouts, {
Expand All @@ -227,7 +225,7 @@ describe( 'updateBlocksLayouts', () => {
const { findBlockLayoutByClientId, updateBlocksLayouts } =
DEFAULT_BLOCK_LIST_CONTEXT;
const currentBlockLayouts = {
current: cloneDeep( BLOCKS_LAYOUTS_DATA ),
current: cloneObject( BLOCKS_LAYOUTS_DATA ),
};

updateBlocksLayouts( currentBlockLayouts, {
Expand Down
22 changes: 7 additions & 15 deletions packages/block-library/src/utils/migrate-font-family.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/**
* External dependencies
*/
import { cloneDeep } from 'lodash';

/**
* Internal dependencies
*/
Expand All @@ -21,17 +16,14 @@ export default function ( attributes ) {
return attributes;
}

// Clone first so when we delete the fontFamily
// below we're not modifying the original
// attributes. Because the deprecation may be discarded
// we don't want to alter the original attributes.
const atts = cloneDeep( attributes );
const fontFamily = atts.style.typography.fontFamily.split( '|' ).pop();
delete atts.style.typography.fontFamily;
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 not sure I can spot where, in the new version of the code, the fontFamily property is deleted from attributes.style.typography ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for asking - we're doing this just on the next line - by doing:

const { fontFamily, ...typography } = attributes.style.typography;

we pick all the attributes.style.typography props in the typography constant, with the exception of fontFamily, and pass them later to the new attributes.style.typography.

FWIW this is the technique we've been using to refactor much of the existing _.omit() instances throughout Gutenberg so far.

atts.style = cleanEmptyObject( atts.style );
const { fontFamily, ...typography } = attributes.style.typography;

return {
...atts,
fontFamily,
...attributes,
style: cleanEmptyObject( {
...attributes.style,
typography,
} ),
fontFamily: fontFamily.split( '|' ).pop(),
};
}
9 changes: 5 additions & 4 deletions packages/components/src/ui/context/context-system-provider.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { isEqual, merge, cloneDeep } from 'lodash';
import { isEqual, merge } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -66,17 +66,18 @@ function useContextSystemBridge( { value } ) {
// `parentContext` will always be memoized (i.e., the result of this hook itself)
// or the default value from when the `ComponentsContext` was originally
// initialized (which will never change, it's a static variable)
// so this memoization will prevent `merge` and `cloneDeep` from rerunning unless
// so this memoization will prevent `merge` and `JSON.parse/stringify` from rerunning unless
// the references to `value` change OR the `parentContext` has an actual material change
// (because again, it's guaranteed to be memoized or a static reference to the empty object
// so we know that the only changes for `parentContext` are material ones... i.e., why we
// don't have to warn in the `useUpdateEffect` hook above for `parentContext` and we only
// need to bother with the `value`). The `useUpdateEffect` above will ensure that we are
// correctly warning when the `value` isn't being properly memoized. All of that to say
// that this should be super safe to assume that `useMemo` will only run on actual
// changes to the two dependencies, therefore saving us calls to `merge` and `cloneDeep`!
// changes to the two dependencies, therefore saving us calls to `merge` and `JSON.parse/stringify`!
const config = useMemo( () => {
return merge( cloneDeep( parentContext ), value );
// Deep clone `parentContext` to avoid mutating it later.
return merge( JSON.parse( JSON.stringify( parentContext ) ), value );
}, [ parentContext, value ] );

return config;
Expand Down
8 changes: 5 additions & 3 deletions packages/edit-site/src/components/global-styles/hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* External dependencies
*/
import { get, cloneDeep, set, isEqual } from 'lodash';
import { get, set, isEqual } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -48,7 +48,8 @@ export function useSetting( path, blockName, source = 'all' ) {

const setSetting = ( newValue ) => {
setUserConfig( ( currentConfig ) => {
const newUserConfig = cloneDeep( currentConfig );
// Deep clone `currentConfig` to avoid mutating it later.
const newUserConfig = JSON.parse( JSON.stringify( currentConfig ) );
const pathToSet = PATHS_WITH_MERGE[ path ]
? fullPath + '.custom'
: fullPath;
Expand Down Expand Up @@ -109,7 +110,8 @@ export function useStyle( path, blockName, source = 'all' ) {

const setStyle = ( newValue ) => {
setUserConfig( ( currentConfig ) => {
const newUserConfig = cloneDeep( currentConfig );
// Deep clone `currentConfig` to avoid mutating it later.
const newUserConfig = JSON.parse( JSON.stringify( currentConfig ) );
set(
newUserConfig,
finalPath,
Expand Down