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 _.omit() in style addSaveProps() #45014

Merged
merged 5 commits into from
Oct 19, 2022
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
125 changes: 122 additions & 3 deletions packages/block-editor/src/hooks/style.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
/**
* External dependencies
*/
import { omit } from 'lodash';
import classnames from 'classnames';

/**
Expand Down Expand Up @@ -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',
* }
* },
* [
tyxla marked this conversation as resolved.
Show resolved Hide resolved
* 'typography.textDecoration',
* 'typography.textTransform',
* ]
* );
* // {}
* ```
*
* You can also specify nested paths as arrays:
*
* ```
* omitStyle(
* {
* typography: {
* textDecoration: 'underline',
* textTransform: 'uppercase',
* }
* },
* [
tyxla marked this conversation as resolved.
Show resolved Hide resolved
* [ 'typography', 'textDecoration' ],
* [ 'typography', 'textTransform' ],
* ]
* );
* // {}
* ```
*
* With regards to nesting of styles, infinite depth is supported:
*
* ```
* omitStyle(
* {
* border: {
* radius: {
* topLeft: '10px',
* topRight: '0.5rem',
* }
* }
* },
* [
tyxla marked this conversation as resolved.
Show resolved Hide resolved
* [ '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.
Copy link
Member

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 the paths 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.

Copy link
Member Author

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 the blocks.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.

* @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 {
delete newStyle[ path ];
Copy link
Member

Choose a reason for hiding this comment

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

This is funny because it should be delete newStyle[ path[ 0 ] ], but it works anyway because [ 'x' ].toString() is still 'x'.

There should be also some check for path.length === 0 because otherwise

omit( { a: 1, '': 2 }, [ [] ] )

will be { a: 1 } while the correct result is the original object unchanged, and _.omit doesn't change the object either.

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 eye, @jsnajdr! Both have been addressed in 4e3c1f9 and I've also added a unit test to cover the scenario you mentioned.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks! I looked at the changes and continue to approve this 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

Thank you for double checking 🙌

}
} );

return newStyle;
}

/**
* Override props assigned to save component to inject the CSS variables definition.
*
Expand All @@ -159,13 +278,13 @@ export function addSaveProps(
const skipSerialization = getBlockSupport( blockType, indicator );

if ( skipSerialization === true ) {
style = omit( style, path );
style = omitStyle( style, path );
Copy link
Member

Choose a reason for hiding this comment

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

Can path also be an array, or is it guaranteed to be a string?

Copy link
Member Author

Choose a reason for hiding this comment

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

path can be a string, an array, an array of strings, and an array of arrays.

Copy link
Member

Choose a reason for hiding this comment

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

If I pass { spacing: [ 'spacing', 'blockGap' ] } as skipPaths, then:

  • if skipSerialization is true, the omitStyle call will remove spacing and blockGap fields
  • if skipSerialization is [ 'feature' ], the omitStyle a few lines later will remove the spacing.blockGap.feature field

That seems wrong, we should always be removing spacing.blockGap, and never treat them separately.

Copy link
Member Author

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't have full context on the original implementation around skipPaths but when you check skipSerializationPathsEdit and skipSerializationPathsSave, their path values are all single element arrays.

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 true value for its __experiementalSkipSerialization flag. For example, all typography styles. The next call is only to omit a single feature for a block support e.g. only skip text decoration but still serialize all other typography supports.

In the latter case, we want style.typography.textDecoration to be removed but the style.typography to remain.

I could well be misunderstanding your point @jsnajdr but hopefully the above helps clarify things some.

Copy link
Member

Choose a reason for hiding this comment

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

That leads me to conclude that there wasn't the intent to omit multiple style paths in a single call

Then why would skipPaths values be arrays? If the intent is to always omit only a single path, the skipPaths would be a Record< string, string >, like:

skipPaths = {
  'spacing': 'spacing.blockGap'
}

but in fact it is a Record< string, string[] >, like:

skipPaths = {
  'spacing': [ 'spacing.blockGap' ]
}

which IMO clearly shows that there can be potentially multiple paths to skip. The skipPaths structure is designed to support that. And indeed a call like:

paths = skipPaths[ 'spacing' ];
style = omit( style, paths );

would remove them all.

But the second omit call, the omit( style, [ [ ...path, feature ] ] ), uses path with different semantics. For our spacing example, that could lead to calling omit as:

omit( style, [ [ 'spacing.blockGap', 'blockGap' ] ] )

which is a nonsense combination, it won't omit anything.

The Gallery block has spacing support defined as:

supports: {
  spacing: {
    /* ... */
    __experimentalSkipSerialization: [ "blockGap" ],
  }
}

So, when calling addSaveProps on such a Gallery block, the spacing.blockGap style won't be removed although it should be?

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.

@tyxla It seems that the pre-existing code has a bug, caused by incorrect usage of omit. That seems very relevant to the current PR, and should get some attention. Otherwise the PR is kind of "garbage in, garbage out", not very valuable. Also, the PR doesn't modify anything else but addSaveProps, which is a reason why it's not interesting to land it separately without addressing issues.

Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that the pre-existing code has a bug, caused by incorrect usage of omit

I think you might be right about a bug, but maybe not with the use of omit.

It appears to be an issue with the check for skipping serialization or the config for skipping the blockGap support in skipSerializationPathsSave.

The __experimentalSkipSerialization flags are meant to be a boolean or array. The block gap support looks like it was trying to always skip its serialization from being saved into block markup, likely due to that fact it was injected server-side.

The indicator used to retrieve the block support config, when processing that block gap entry from skipSerializationPathsSave, was simply spacing, which would return that entire supports config object.

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 true or an array.

cc/ @andrewserong for your thoughts and additional background here.

Then why would skipPaths values be arrays?

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 typography banner like they are now.

They could probably be changed to strings not arrays now.

But the second omit call, the omit( style, [ [ ...path, feature ] ] ), uses path with different semantics. For our spacing example, that could lead to calling omit as:

omit( style, [ [ 'spacing.blockGap', 'blockGap' ] ] )

A call as suggested here wouldn't occur as the skip serialization retrieved from the block supports wouldn't be true or an array.

So, when calling addSaveProps on such a Gallery block, the spacing.blockGap style won't be removed although it should be?

In this case, the blockGap style is being removed both in the edit and save contexts due to the normal spacing block support config in the skipPaths. When processing the skip paths, the indicator for the spacing support, ${ SPACING_SUPPORT_KEY }.__experimentalSkipSerialization, would return the array [ "blockGap" ]. Which would then mean a call to omit( style, [ [ 'spacing', 'blockGap' ] ] ).

Copy link
Contributor

Choose a reason for hiding this comment

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

Otherwise the PR is kind of "garbage in, garbage out", not very valuable. Also, the PR doesn't modify anything else but addSaveProps, which is a reason why it's not interesting to land it separately without addressing issues.

Keeping the scope for this PR purely to the replacement of the Lodash omit function sounds like a win. I don't think it hurts to move the discussion on the logic contained in addSaveProps to its own dedicated PR rather than hijack this PR further.

Happy to leave that call to @tyxla though 🙂

Copy link
Member

Choose a reason for hiding this comment

The 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 🙂

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, everyone for the thoughts and research put into this discussion.

@aaronrobertshaw basically said it here:

Keeping the scope for this PR purely to the replacement of the Lodash omit function sounds like a win. I don't think it hurts to move the discussion on the logic contained in addSaveProps to its own dedicated PR rather than hijack this PR further.

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 ] ] );
Copy link
Member

Choose a reason for hiding this comment

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

This indicates that path must be an array of path items, never a string, but the omitStyle( style, path ) above indicates otherwise: that it's either a string or an array of paths (not an array of one path's items). Confusing.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. Still the original omit() supported a single string. Would you recommend breaking backward compatibility in that case?

Copy link
Member

Choose a reason for hiding this comment

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

omitStyle is a new function -- which backward compatibility do we need to keep? The skipPaths argument of addSaveProps?

Copy link
Member Author

Choose a reason for hiding this comment

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

The skipPaths argument of addSaveProps?

Yes. Essentially, the function omitStyle() is used via the addSaveProps function and the blocks.getSaveContent.extraProps filter, and the skipPaths could technically be anything that we used to previously support with _.omit(). Let me know if that makes sense.

} );
}
} );
Expand Down
180 changes: 179 additions & 1 deletion packages/block-editor/src/hooks/test/style.js
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -202,3 +202,181 @@ 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 paths with different nesting', () => {
tyxla marked this conversation as resolved.
Show resolved Hide resolved
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', () => {
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

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 idea indeed. I've added a test in a691f88.

expect( omitStyle( undefined, 'color' ) ).toEqual( undefined );
expect( omitStyle( null, 'color' ) ).toEqual( null );
} );
} );