-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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 _.omit()
in style addSaveProps()
#45014
Changes from all commits
9589ffe
ae0d16e
87c6c55
a691f88
4e3c1f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,6 @@ | ||
/** | ||
* External dependencies | ||
*/ | ||
import { omit } from 'lodash'; | ||
import classnames from 'classnames'; | ||
|
||
/** | ||
|
@@ -134,6 +133,126 @@ const skipSerializationPathsSave = { | |
*/ | ||
const renamedFeatures = { gradients: 'gradient' }; | ||
|
||
/** | ||
* A utility function used to remove one or more paths from a style object. | ||
* Works in a way similar to Lodash's `omit()`. See unit tests and examples below. | ||
* | ||
* It supports a single string path: | ||
* | ||
* ``` | ||
* omitStyle( { color: 'red' }, 'color' ); // {} | ||
* ``` | ||
* | ||
* or an array of paths: | ||
* | ||
* ``` | ||
* omitStyle( { color: 'red', background: '#fff' }, [ 'color', 'background' ] ); // {} | ||
* ``` | ||
* | ||
* It also allows you to specify paths at multiple levels in a string. | ||
* | ||
* ``` | ||
* omitStyle( { typography: { textDecoration: 'underline' } }, 'typography.textDecoration' ); // {} | ||
* ``` | ||
* | ||
* You can remove multiple paths at the same time: | ||
* | ||
* ``` | ||
* omitStyle( | ||
* { | ||
* typography: { | ||
* textDecoration: 'underline', | ||
* textTransform: 'uppercase', | ||
* } | ||
* }, | ||
* [ | ||
* 'typography.textDecoration', | ||
* 'typography.textTransform', | ||
* ] | ||
* ); | ||
* // {} | ||
* ``` | ||
* | ||
* You can also specify nested paths as arrays: | ||
* | ||
* ``` | ||
* omitStyle( | ||
* { | ||
* typography: { | ||
* textDecoration: 'underline', | ||
* textTransform: 'uppercase', | ||
* } | ||
* }, | ||
* [ | ||
* [ 'typography', 'textDecoration' ], | ||
* [ 'typography', 'textTransform' ], | ||
* ] | ||
* ); | ||
* // {} | ||
* ``` | ||
* | ||
* With regards to nesting of styles, infinite depth is supported: | ||
* | ||
* ``` | ||
* omitStyle( | ||
* { | ||
* border: { | ||
* radius: { | ||
* topLeft: '10px', | ||
* topRight: '0.5rem', | ||
* } | ||
* } | ||
* }, | ||
* [ | ||
* [ 'border', 'radius', 'topRight' ], | ||
* ] | ||
* ); | ||
* // { border: { radius: { topLeft: '10px' } } } | ||
* ``` | ||
* | ||
* The third argument, `preserveReference`, defines how to treat the input style object. | ||
* It is mostly necessary to properly handle mutation when recursively handling the style object. | ||
* Defaulting to `false`, this will always create a new object, avoiding to mutate `style`. | ||
* However, when recursing, we change that value to `true` in order to work with a single copy | ||
* of the original style object. | ||
* | ||
* @see https://lodash.com/docs/4.17.15#omit | ||
* | ||
* @param {Object} style Styles object. | ||
* @param {Array|string} paths Paths to remove. | ||
* @param {boolean} preserveReference True to mutate the `style` object, false otherwise. | ||
* @return {Object} Styles object with the specified paths removed. | ||
*/ | ||
export function omitStyle( style, paths, preserveReference = false ) { | ||
if ( ! style ) { | ||
return style; | ||
} | ||
|
||
let newStyle = style; | ||
if ( ! preserveReference ) { | ||
newStyle = JSON.parse( JSON.stringify( style ) ); | ||
} | ||
|
||
if ( ! Array.isArray( paths ) ) { | ||
paths = [ paths ]; | ||
} | ||
|
||
paths.forEach( ( path ) => { | ||
if ( ! Array.isArray( path ) ) { | ||
path = path.split( '.' ); | ||
} | ||
|
||
if ( path.length > 1 ) { | ||
const [ firstSubpath, ...restPath ] = path; | ||
omitStyle( newStyle[ firstSubpath ], [ restPath ], true ); | ||
} else if ( path.length === 1 ) { | ||
delete newStyle[ path[ 0 ] ]; | ||
} | ||
} ); | ||
Comment on lines
+245
to
+251
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @tyxla: I have no idea if this code path is hot enough for this to make a difference in performance, but — just in case — there is this alternative to recursion (passes all tests): if (! obj) return obj
if (! Array.isArray(paths)) paths = [paths]
const copy = JSON.parse(JSON.stringify(obj));
for (let path of paths) {
if (! Array.isArray(path)) path = path.split('.')
let target = copy;
// Walk up to the last branch, stopping before the leaf
for (let i = 0; i < path.length - 1; i++) {
target = target[path[i]];
}
delete target[path[path.length - 1]];
}
return copy; As a side benefit, it simplifies the interface by removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for the suggestion, @mcsf! That seems to be the constant argument between implementing an algorithm to be recursive or iterative. I did have something like that but found the recursive version a bit more readable. Performance-wise, my observations are that both solutions will be close enough, with a difference of around roughly 10%. I'm happy to alter it to iterative if the recursive version doesn't look good to you though! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No worries, I trust your judgment! It was something I quickly drew up in between tasks, and I thought I'd preserve it in this discussion before I deleted the file. |
||
|
||
return newStyle; | ||
} | ||
|
||
/** | ||
* Override props assigned to save component to inject the CSS variables definition. | ||
* | ||
|
@@ -159,13 +278,13 @@ export function addSaveProps( | |
const skipSerialization = getBlockSupport( blockType, indicator ); | ||
|
||
if ( skipSerialization === true ) { | ||
style = omit( style, path ); | ||
style = omitStyle( style, path ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If I pass
That seems wrong, we should always be removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I looked at the original code and couldn't fully understand why there was a need for a special treatment. However, from what I understand, this isn't something that's coming from this PR, so I believe it makes sense that we handle it in a different PR. Or am I misunderstanding you? What do you think? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't have full context on the original implementation around I believe they were designed that way specifically to align with the fact that each block support (border, color, spacing, typography etc) corresponds with a single top-level path within the style object. That leads me to conclude that there wasn't the intent to omit multiple style paths in a single call here, as proposed. The initial call to omit a style occurs when an entire block support has its serialization skipped via a simple In the latter case, we want I could well be misunderstanding your point @jsnajdr but hopefully the above helps clarify things some. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Then why would skipPaths = {
'spacing': 'spacing.blockGap'
} but in fact it is a skipPaths = {
'spacing': [ 'spacing.blockGap' ]
} which IMO clearly shows that there can be potentially multiple paths to skip. The paths = skipPaths[ 'spacing' ];
style = omit( style, paths ); would remove them all. But the second omit( style, [ [ 'spacing.blockGap', 'blockGap' ] ] ) which is a nonsense combination, it won't omit anything. The Gallery block has spacing support defined as:
So, when calling
@tyxla It seems that the pre-existing code has a bug, caused by incorrect usage of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think you might be right about a bug, but maybe not with the use of It appears to be an issue with the check for skipping serialization or the config for skipping the blockGap support in The The indicator used to retrieve the block support config, when processing that block gap entry from I'd need to test that further, but it appears the block gap support wouldn't be being skipped for save at all, as that skip serialization value wouldn't be cc/ @andrewserong for your thoughts and additional background here.
Good point. Looking back through the file history here it looks like they were arrays due to some of the typography styles originally not being included under a single They could probably be changed to strings not arrays now.
A call as suggested here wouldn't occur as the skip serialization retrieved from the block supports wouldn't be
In this case, the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Keeping the scope for this PR purely to the replacement of the Lodash Happy to leave that call to @tyxla though 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now when we reached some agreement that there is indeed something wrong with the underlying code, and it's on someone's radar, my objections are no longer so strong 🙂 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks, everyone for the thoughts and research put into this discussion. @aaronrobertshaw basically said it here:
This aligns with my sentiment, and I do think we should try our best to keep changes focused on one thing at a time. What if there were 1000 bugs in this component? Should we fix them all in a single PR? I don't think so. That doesn't mean that this shouldn't be addressed in a follow-up, though. I do feel like @aaronrobertshaw, @andrewserong, and @ramonjd are better equipped to address this one, but I'm happy to help with reviews and testing. Any objections? |
||
} | ||
|
||
if ( Array.isArray( skipSerialization ) ) { | ||
skipSerialization.forEach( ( featureName ) => { | ||
const feature = renamedFeatures[ featureName ] || featureName; | ||
style = omit( style, [ [ ...path, feature ] ] ); | ||
style = omitStyle( style, [ [ ...path, feature ] ] ); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This indicates that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree. Still the original There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Yes. Essentially, the function |
||
} ); | ||
} | ||
} ); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import { applyFilters } from '@wordpress/hooks'; | |
/** | ||
* Internal dependencies | ||
*/ | ||
import { getInlineStyles } from '../style'; | ||
import { getInlineStyles, omitStyle } from '../style'; | ||
|
||
describe( 'getInlineStyles', () => { | ||
it( 'should return an empty object when called with undefined', () => { | ||
|
@@ -202,3 +202,208 @@ describe( 'addSaveProps', () => { | |
} ); | ||
} ); | ||
} ); | ||
|
||
describe( 'omitStyle', () => { | ||
it( 'should remove a single path', () => { | ||
const style = { color: '#d92828', padding: '10px' }; | ||
const path = 'color'; | ||
const expected = { padding: '10px' }; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should remove multiple paths', () => { | ||
const style = { color: '#d92828', padding: '10px', background: 'red' }; | ||
const path = [ 'color', 'background' ]; | ||
const expected = { padding: '10px' }; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should remove nested paths when specified as a string', () => { | ||
const style = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
textTransform: 'uppercase', | ||
}, | ||
}; | ||
const path = 'typography.textTransform'; | ||
const expected = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should remove nested paths when specified as an array', () => { | ||
const style = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
textTransform: 'uppercase', | ||
}, | ||
}; | ||
const path = [ [ 'typography', 'textTransform' ] ]; | ||
const expected = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should remove multiple nested paths', () => { | ||
const style = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
textTransform: 'uppercase', | ||
}, | ||
}; | ||
const path = [ | ||
[ 'typography', 'textTransform' ], | ||
'typography.textDecoration', | ||
]; | ||
const expected = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: {}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should remove paths with different nesting', () => { | ||
const style = { | ||
color: { | ||
text: '#d92828', | ||
}, | ||
typography: { | ||
textDecoration: 'underline', | ||
textTransform: 'uppercase', | ||
}, | ||
}; | ||
const path = [ | ||
'color', | ||
[ 'typography', 'textTransform' ], | ||
'typography.textDecoration', | ||
]; | ||
const expected = { | ||
typography: {}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should support beyond 2 levels of nesting when passed as a single string', () => { | ||
const style = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
topRight: '0.5rem', | ||
}, | ||
}, | ||
}; | ||
const path = 'border.radius.topRight'; | ||
const expected = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
}, | ||
}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should support beyond 2 levels of nesting when passed as array of strings', () => { | ||
const style = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
topRight: '0.5rem', | ||
}, | ||
}, | ||
}; | ||
const path = [ 'border.radius.topRight' ]; | ||
const expected = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
}, | ||
}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should support beyond 2 levels of nesting when passed as array of arrays', () => { | ||
const style = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
topRight: '0.5rem', | ||
}, | ||
}, | ||
}; | ||
const path = [ [ 'border', 'radius', 'topRight' ] ]; | ||
const expected = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
}, | ||
}, | ||
}; | ||
|
||
expect( omitStyle( style, path ) ).toEqual( expected ); | ||
} ); | ||
|
||
it( 'should ignore a nullish style object', () => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I like this PR! Would it worth adding test to see what happens when there is no property match for a given path? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The extra test sounds like a good idea, even if just to protect against regressions for future refactors. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good idea indeed. I've added a test in a691f88. |
||
expect( omitStyle( undefined, 'color' ) ).toEqual( undefined ); | ||
expect( omitStyle( null, 'color' ) ).toEqual( null ); | ||
} ); | ||
|
||
it( 'should ignore a missing object property', () => { | ||
const style1 = { typography: {} }; | ||
expect( omitStyle( style1, 'color' ) ).toEqual( style1 ); | ||
|
||
const style2 = { color: { text: '#d92828' } }; | ||
expect( omitStyle( style2, 'color.something' ) ).toEqual( style2 ); | ||
|
||
const style3 = { | ||
border: { | ||
radius: { | ||
topLeft: '10px', | ||
topRight: '0.5rem', | ||
}, | ||
}, | ||
}; | ||
expect( | ||
omitStyle( style3, [ [ 'border', 'radius', 'bottomLeft' ] ] ) | ||
).toEqual( style3 ); | ||
} ); | ||
|
||
it( 'should ignore an empty array path', () => { | ||
const style = { typography: {}, '': 'test' }; | ||
|
||
expect( omitStyle( style, [] ) ).toEqual( style ); | ||
expect( omitStyle( style, [ [] ] ) ).toEqual( style ); | ||
} ); | ||
} ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While creating a brand new
omitStyle
function, can be limit thepaths
argument to always be either one path, or always an array of multiple paths?The fact that the element of the
paths
array can also be an array makes the API rather confusing.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can, however, this would require some modifications to the original code that uses
_.omit()
in the first place. Also, since this is exposed to theblocks.getSaveContent.extraProps
filter, we'd have to provide full backward compatibility.The existing code relies on supporting multiple paths and being able to specify them both as strings and arrays. So the API here strives to maintain backward compatibility while improving docs and providing unit tests to ensure the behavior is more transparent than before.
Note that this function is exported solely to allow it to be unit-tested. It should be for internal usage only and not part of the public API.