Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 styleaddSaveProps()
#45014Lodash: Refactor away from
_.omit()
in styleaddSaveProps()
#45014Changes from 1 commit
9589ffe
ae0d16e
87c6c55
a691f88
4e3c1f9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
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.
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 otherwisewill 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.
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.
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.
Thanks! I looked at the changes and continue to approve this 👍
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.
Thank you for double checking 🙌
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.
Can
path
also be an array, or is it guaranteed to be a string?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.
path
can be a string, an array, an array of strings, and an array of arrays.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.
If I pass
{ spacing: [ 'spacing', 'blockGap' ] }
asskipPaths
, then:skipSerialization
istrue
, theomitStyle
call will removespacing
andblockGap
fieldsskipSerialization
is[ 'feature' ]
, theomitStyle
a few lines later will remove thespacing.blockGap.feature
fieldThat seems wrong, we should always be removing
spacing.blockGap
, and never treat them separately.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.
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 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 checkskipSerializationPathsEdit
andskipSerializationPathsSave
, 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, alltypography
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 thestyle.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then why would
skipPaths
values be arrays? If the intent is to always omit only a single path, theskipPaths
would be aRecord< string, string >
, like:but in fact it is a
Record< string, string[] >
, like: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:would remove them all.
But the second
omit
call, theomit( style, [ [ ...path, feature ] ] )
, usespath
with different semantics. For our spacing example, that could lead to callingomit
as:which is a nonsense combination, it won't omit anything.
The Gallery block has spacing support defined as:
So, when calling
addSaveProps
on such a Gallery block, thespacing.blockGap
style won't be removed although it should be?@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 butaddSaveProps
, which is a reason why it's not interesting to land it separately without addressing issues.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.
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 aboolean
orarray
. 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 simplyspacing
, which would return that entire supports configobject
.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 anarray
.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
typography
banner like they are now.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
true
or anarray
.In this case, the
blockGap
style is being removed both in the edit and save contexts due to the normalspacing
block support config in theskipPaths
. 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 toomit( style, [ [ 'spacing', 'blockGap' ] ] )
.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.
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 inaddSaveProps
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.
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 🙂
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.
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?
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.
This indicates that
path
must be an array of path items, never a string, but theomitStyle( 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.
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?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.
omitStyle
is a new function -- which backward compatibility do we need to keep? TheskipPaths
argument ofaddSaveProps
?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.
Yes. Essentially, the function
omitStyle()
is used via theaddSaveProps
function and theblocks.getSaveContent.extraProps
filter, and theskipPaths
could technically be anything that we used to previously support with_.omit()
. Let me know if that makes sense.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.
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 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.
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.
Good idea indeed. I've added a test in a691f88.