Lodash: Refactor away from _.omit() in style addSaveProps()
#45014
Conversation
tyxla
added
[Type] Performance
tyxla
requested review from
ockham,
youknowriad,
sgomes,
aristath,
jsnajdr,
swissspidy,
glendaviesnz,
ramonjd,
tellthemachines,
chad1008,
andrewserong,
ntsekouras,
fabiankaegy and
aaronrobertshaw
tyxla
force-pushed
the
refactor/add-save-props-omit
branch
from
fd484d1
to
9589ffe
Compare
|
Size Change: +1.45 kB (0%) Total Size: 1.27 MB
ℹ️ View Unchanged
|
|
I have to admit that I don't understand the reasoning for this PR. Why change a util that is already loaded and always will be (lodash is loaded as a separate script in WP) with a custom utility that we'd have to maintain.
|
| * @see https://lodash.com/docs/4.17.15#omit | ||
| * | ||
| * @param {Object} style Styles object. | ||
| * @param {Array|string} paths Paths to remove. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -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.
Can path also be an array, or is it guaranteed to be a string?
There was a problem hiding this comment.
path can be a string, an array, an array of strings, and an array of arrays.
There was a problem hiding this comment.
If I pass { spacing: [ 'spacing', 'blockGap' ] } as skipPaths, then:
- if
skipSerializationistrue, theomitStylecall will removespacingandblockGapfields - if
skipSerializationis[ 'feature' ], theomitStylea few lines later will remove thespacing.blockGap.featurefield
That seems wrong, we should always be removing spacing.blockGap, and never treat them separately.
There was a problem hiding this comment.
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.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
skipPathsvalues 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
omitcall, theomit( style, [ [ ...path, feature ] ] ), usespathwith different semantics. For our spacing example, that could lead to callingomitas:
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' ] ] ).
There was a problem hiding this comment.
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 🙂
There was a problem hiding this comment.
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.
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 ] ] ); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree. Still the original omit() supported a single string. Would you recommend breaking backward compatibility in that case?
There was a problem hiding this comment.
omitStyle is a new function -- which backward compatibility do we need to keep? The skipPaths argument of addSaveProps?
There was a problem hiding this comment.
The
skipPathsargument ofaddSaveProps?
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.
I think the reasoning for removing Lodash from Gutenberg, although the |
|
Thanks for chiming in @youknowriad - with regards to why we're removing Lodash from Gutenberg, there are a bunch of rationales discussed in the links I've mentioned in the PR description. They're a great read and cover the state of things really well in my opinion. On top of that, I agree and recognize that in the short term, the result is a few bytes on top of what we already had, but we're doing this with a few end goals in mind: Removing While this won't impact WP itself yet, since Not using AFAIK, the current usage of Developer experience, code readability and quality One of my primary focuses in working on Gutenberg on top of performance is improving the developer experience, and part of that is exploring ways to make code clearer and easier to read. This PR is a clear example of how using Lodash in certain places can make code difficult to read and understand, and conceal underlying functionality. The original |
|
I really want to reinforce @tyxla's point that Plus, even in the context of WP core, we can't assume that a library will already be loaded, and that this will always be the case in the future. Even the ubiquitous |
|
Can we assume that the collection of all these small utils we'll be duplicating is smaller than the lodash script is self by a significant order of magnitude. Also, can't we just import lodash/omit and bundle it instead of maintaining our own utility? I'm still very skeptical. I'm one of the persons that would encourage packages usage outside of WP and in different context but it still remains marginal to be honest compared to its potential. |
That's my assumption, yes, and it mirrors how the Lodash migration has been going so far. I don't think we'll end up duplicating too many of those. We've gotten rid of a big chunk of Lodash already and I don't see much duplication around the codebase. I'm also positive that things will get better as more proposals TC39 proposals get accepted, standardized, and implemented by browsers. A few years ago, it was unthinkable to write good code without a library like Lodash, and nowadays, Lodash helper functions are almost unnecessary. Just imagine what it would mean for the possibilities of JavaScript a few years from now. Note that what we're doing here is not really duplication; the substitute code is much simpler, shorter, more targeted and adapted to our specific needs, more readable, better documented, and better tested - I think we need to consider those important factors too.
Sure, we can. However, it's worth mentioning that it still pulls ~10% of the entire Lodash as a dependency (see here). That doesn't only make the bundle size way larger; it also makes the code less predictable, because as I stated above, it's not immediately clear how
Anyway, I'd love to hear more about your opinion. I'm curious how all the trade-offs we've listed above are not enough to convince you 😉 |
| expect( omitStyle( style, path ) ).toEqual( expected ); | ||
| } ); | ||
|
|
||
| it( 'should ignore a nullish style object', () => { |
There was a problem hiding this comment.
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.
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.
Good idea indeed. I've added a test in a691f88.
There was a problem hiding this comment.
Thanks for tackling the removal of Lodash dependencies @tyxla 👍
Personally, I'm in favour of the removal of omit here, given the arguments presented so far for doing so. Happy to let others make a final call on that, as I don't feel too strongly about it.
This PR tests well for me:
✅ Unit tests are passing
✅ Skipping of entire block supports or their individual features works
✅ Skipped serialization was working for static blocks e.g. Group, and dynamic blocks e.g. Search.
From what I could work out the blockGap related functionality was still working but it might be worth double checking with @andrewserong on that front.
| expect( omitStyle( style, path ) ).toEqual( expected ); | ||
| } ); | ||
|
|
||
| it( 'should ignore a nullish style object', () => { |
There was a problem hiding this comment.
The extra test sounds like a good idea, even if just to protect against regressions for future refactors.
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com>
|
Thanks, everyone for the great feedback, insights, and thorough testing! I think I've addressed all of it, and it's now ready for another look. I agree with @jsnajdr that the |
| const [ firstSubpath, ...restPath ] = path; | ||
| omitStyle( newStyle[ firstSubpath ], [ restPath ], true ); | ||
| } else { | ||
| delete newStyle[ path ]; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thanks! I looked at the changes and continue to approve this 👍
There was a problem hiding this comment.
Thank you for double checking 🙌
| if ( path.length > 1 ) { | ||
| const [ firstSubpath, ...restPath ] = path; | ||
| omitStyle( newStyle[ firstSubpath ], [ restPath ], true ); | ||
| } else if ( path.length === 1 ) { | ||
| delete newStyle[ path[ 0 ] ]; | ||
| } | ||
| } ); |
There was a problem hiding this comment.
@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 preserveReference.
There was a problem hiding this comment.
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.
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.
What?
This PR removes Lodash's
_.omit()from the styleaddSavePropshook in favor of a custom alternative utility functionWhy?
Lodash is known to unnecessarily inflate the bundle size of packages, and in most cases, it can be replaced with native language functionality. See these for more information and rationale:
@wordpress/api-fetchpackage haslodashas a dependency #39495This is technically the most complex remaining use of
_.omit()and I believe that the way Lodash handles arguments in a different way can very easily be concealed, which could lead to subtle bugs.How?
We're creating a custom utility function that handles the omission of styles recursively and still supports all use cases that the Lodash function used to. We're adding thorough documentation and unit tests to ensure that the current way it works is more transparent than it was.
Testing Instructions
npm run test:unit packages/block-editor/src/hooks/test/style.js