Lodash: Refactor away from _.cloneDeep()
#43631
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
tyxla
added
[Type] Performance
tyxla
requested review from
sgomes,
mirka,
aristath,
gziolo,
swissspidy,
ciampo,
jorgefilipecosta,
chad1008,
ntsekouras and
fabiankaegy
|
Size Change: -11 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
ciampo
reviewed
Aug 29, 2022
There was a problem hiding this comment.
Thank you for working on this!
- Since the goal of the JSON parse/stringify technique may not be obvious to all developers, should we add inline comments to explain that aim is to
cloneDeepan object? - This JSON technique errors (or, even worse, clone in an unreliable way) when the object being cloned is not JSON-safe — i.e. it contains
NaNvalues,undefined,Infinity, functions, regexps, maps, sets... Is it not risky to rely on this technique?
| // we don't want to alter the original attributes. | ||
| const atts = cloneDeep( attributes ); | ||
| const fontFamily = atts.style.typography.fontFamily.split( '|' ).pop(); | ||
| delete atts.style.typography.fontFamily; |
There was a problem hiding this comment.
I'm not sure I can spot where, in the new version of the code, the fontFamily property is deleted from attributes.style.typography ?
There was a problem hiding this comment.
Thanks for asking - we're doing this just on the next line - by doing:
const { fontFamily, ...typography } = attributes.style.typography;
we pick all the attributes.style.typography props in the typography constant, with the exception of fontFamily, and pass them later to the new attributes.style.typography.
FWIW this is the technique we've been using to refactor much of the existing _.omit() instances throughout Gutenberg so far.
tyxla
force-pushed
the
refactor/lodash-clone-deep
branch
from
352a714
to
e5361a5
Compare
|
Thanks for your review, @ciampo!
Good call - just did that in e5361a5.
I agree it could be, however, we're not using it in a general place where such can occur, so I believe it's safe. It would be risky if we had extracted it to a utility function and then rely on it to support all kinds of data structures, which we definitely aren't doing here. This is a compromise and I believe it makes sense in this case because I don't see a use for any special data structures in the affected places of the code at this time. And I prefer keeping it simple - we can always work on a more convoluted solution if we need to at some point in the future. |
|
@tyxla : Is there any reason in particular you went with JSON parse/stringify instead of the more immediate and complete |
Yup, admittedly, I was getting some errors and decided to go the easy and safe path, considering that the usages were pretty straightforward. |
|
Also, it's not available in Node before v17 IIRC. |
Build errors, I assume? Usage-wise, it should actually be the other way around;
Oh, that's a problem if it's not being polyfilled there 👍 |
Yes. |
I'm not sure about that, specifically regarding the changes to:
|
To be fair, |
|
The concerns are fair, but as @sgomes brought up, |
tyxla
force-pushed
the
refactor/lodash-clone-deep
branch
from
e5361a5
to
04d96f1
Compare
|
Given that we also have some unit tests in place for the context system provider, let's go ahead with the current approach, keep an eye for any potential regressions, and iterate as needed. |
ciampo
approved these changes
Aug 29, 2022
tyxla
force-pushed
the
refactor/lodash-clone-deep
branch
from
04d96f1
to
367e3ce
Compare
|
Might be worth doing some benchmarks here to see how fast/slow the JSON approach is vs |
Good call - in my few runs the JSON approach was a bit faster, but then |
Do note that these sorts of benchmarks tend to be inappropriate for determining which of the two approaches is faster. Repeatedly running the same code on the same data leads to very different results that don't reflect real world performance, as different optimisations are performed by the browser than in a real-world scenario. In any case, there's generally no need for micro-benchmarks of this kind, unless you're dealing with a known hot path. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
What?
This PR removes the
_.cloneDeep()usage completely and deprecates the function.Why?
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 #39495How?
We're either refactoring to use different means of native functionality (optional chaining) when possible, or using
JSON.parse( JSON.stringify( object ) )to workaround the deep cloning.Testing Instructions
npm run test:unitandnpm run native test.