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.

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 _.set() in core-data #48784

Merged
merged 2 commits into from
Mar 13, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
5 changes: 3 additions & 2 deletions packages/core-data/src/queried-data/selectors.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,12 +3,12 @@
*/
import createSelector from 'rememo';
import EquivalentKeyMap from 'equivalent-key-map';
import { set } from 'lodash';

/**
* Internal dependencies
*/
import getQueryParts from './get-query-parts';
import { setNestedValue } from '../utils';

/**
* Cache of state keys to EquivalentKeyMap where the inner map tracks queries
Expand Down Expand Up @@ -70,7 +70,8 @@ function getQueriedItemsUncached( state, query ) {
field.forEach( ( fieldName ) => {
value = value[ fieldName ];
} );
set( filteredItem, field, value );

setNestedValue( filteredItem, field, value );
}
} else {
// If expecting a complete item, validate that completeness, or
Expand Down
9 changes: 6 additions & 3 deletions packages/core-data/src/selectors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,6 @@
* External dependencies
*/
import createSelector from 'rememo';
import { set } from 'lodash';

/**
* WordPress dependencies
Expand All @@ -17,7 +16,11 @@ import deprecated from '@wordpress/deprecated';
import { STORE_NAME } from './name';
import { getQueriedItems } from './queried-data';
import { DEFAULT_ENTITY_KEY } from './entities';
import { getNormalizedCommaSeparable, isRawAttribute } from './utils';
import {
getNormalizedCommaSeparable,
isRawAttribute,
setNestedValue,
} from './utils';
import type * as ET from './entity-types';

// This is an incomplete, high-level approximation of the State type.
Expand Down Expand Up @@ -336,7 +339,7 @@ export const getEntityRecord = createSelector(
field.forEach( ( fieldName ) => {
value = value[ fieldName ];
} );
set( filteredItem, field, value );
setNestedValue( filteredItem, field, value );
}
return filteredItem as EntityRecord;
}
Expand Down
1 change: 1 addition & 0 deletions packages/core-data/src/utils/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,3 +6,4 @@ export { default as onSubKey } from './on-sub-key';
export { default as replaceAction } from './replace-action';
export { default as withWeakMapCache } from './with-weak-map-cache';
export { default as isRawAttribute } from './is-raw-attribute';
export { default as setNestedValue } from './set-nested-value';
33 changes: 33 additions & 0 deletions packages/core-data/src/utils/set-nested-value.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
/**
* Sets the value at path of object.
* If a portion of path doesn’t exist, it’s created.
* Arrays are created for missing index properties while objects are created
* for all other missing properties.
*
* This function intentionally mutates the input object.
*
* Inspired by _.set().
*
* @see https://lodash.com/docs/4.17.15#set
*
* @param {Object} object Object to modify
* @param {Array} path Path of the property to set.
* @param {*} value Value to set.
*/
export default function setNestedValue( object, path, value ) {
path.reduce( ( acc, key, idx ) => {
if ( acc[ key ] === undefined ) {
if ( Number.isInteger( path[ idx + 1 ] ) ) {
acc[ key ] = [];
} else {
acc[ key ] = {};
}
}
if ( idx === path.length - 1 ) {
acc[ key ] = value;
}
return acc[ key ];
}, object );

return object;
}
51 changes: 51 additions & 0 deletions packages/core-data/src/utils/test/set-nested-value.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
/**
* Internal dependencies
*/
import setNestedValue from '../set-nested-value';

describe( 'setNestedValue', () => {
it( 'should return the same object unmodified if path is an empty array', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably add some tests to ensure that it behaves correctly if input is not an object, e.g. null or undefined. I'll leave it to your judgement what "behaves correctly" should mean; lodash simply does nothing, but throwing an error may be a good alternative as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good point. Happy to alter it to do nothing as well - it's a simple condition. I've done it in 37cf182 where I also added some additional unit tests.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks great, thank you!

const input = { x: 'y' };
const result = setNestedValue( input, [], 123 );

expect( result ).toBe( input );
expect( result ).toEqual( { x: 'y' } );
} );

it( 'should set values at deep level', () => {
const input = { x: { y: { z: 123 } } };
const result = setNestedValue( input, [ 'x', 'y', 'z' ], 456 );

expect( result ).toEqual( { x: { y: { z: 456 } } } );
} );

it( 'should create nested objects if necessary', () => {
const result = setNestedValue( {}, [ 'x', 'y', 'z' ], 123 );

expect( result ).toEqual( { x: { y: { z: 123 } } } );
} );

it( 'should create nested arrays when keys are numeric', () => {
const result = setNestedValue( {}, [ 'x', 0, 'z' ], 123 );

expect( result ).toEqual( { x: [ { z: 123 } ] } );
} );

it( 'should keep remaining properties unaffected', () => {
const input = { x: { y: { z: 123, z1: 'z1' }, y1: 'y1' }, x1: 'x1' };
const result = setNestedValue( input, [ 'x', 'y', 'z' ], 456 );

expect( result ).toEqual( {
x: { y: { z: 456, z1: 'z1' }, y1: 'y1' },
x1: 'x1',
} );
} );

it( 'should intentionally mutate the original object', () => {
const input = { x: 'y' };
const result = setNestedValue( input, [ 'x' ], 'z' );

expect( result ).toBe( input );
expect( result ).toEqual( { x: 'z' } );
} );
} );