Replace lodash.assign with vanilla JS. #21054
Conversation
ZebulanStanphill
added
[Type] Code Quality
|
Size Change: +68 B (0%) Total Size: 856 kB
ℹ️ View Unchanged
|
|
The failing unit test illustrates one additional benefit of Lodash 😄 (It tolerates nullish values better than native But in general, I can get on board with this effort. A couple things to consider:
|
ZebulanStanphill
force-pushed
the
update/replace-lodash-assign-with-vanilla-js
branch
from
a80435a
to
389d8d2
Compare
|
(Maybe I should start running the tests before pushing... but then again, they all end up having to get run again anyway, so what's the point? 😛) All good points, @aduth! Yeah, I'm mainly doing this for the sake of 3rd party package usage and speed improvements. I'll make sure to check the performance comparisons. 👍 I have now switched to an approach using object spread syntax. In my testing, this appears to be even faster than Unfortunately, there are still some test failures: e2e failures this time. But as far as I can tell, they are completely unrelated. Any ideas? |
Huh, that's Interesting, and quite unexpected to me. As the one who originally implemented the original code, I remember explicitly doing it this way in mind of the fact that an object spread is effectively the same as: Object.assign( {}, a, b );...contrasted with as it was chosen to be implemented: Object.assign( a, b );To me, I would think the fact that assigning the values from sources of both Then again, since it's all processed by some combination of Babel and core-js (not actually native), it's hard to accurately measure. |
|
Actually, I wasn't even testing through Babel compilation; I was comparing self-made tests on https://jsbench.me/. Specifically, I was comparing these two: const settings = {};
settings.attributes = {};
Object.assign(settings.attributes, {thing: {donut: 'cake'}});const settings = {};
settings.attributes = {
...settings.attributes,
thing: {donut: 'cake'}
};I checked, and if |
|
After restarting e2e tests several times, the same ones keep failing, but I haven't got a clue as to why. I wonder if my optimizations have revealed a race condition or something... 😕 Any help would be greatly appreciated. |
|
Is it the same error each time? From the current one, it appears to be one which has been frequently failing lately: #21052. It might just be bad luck combined with a particularly flakey test. I'll restart the build. Separately, I'd also like to find some time this week to properly debug / fix these current intermittent failures. |
ZebulanStanphill
force-pushed
the
update/replace-lodash-assign-with-vanilla-js
branch
from
389d8d2
to
f9ea821
Compare
|
I did some further testing, and I discovered that the second test I made is only faster in Firefox; it is slower in Chrome. I have now discovered an approach that is more verbose, but it is the fastest I've tried on both Chrome and Firefox. It also happens to use very basic JavaScript that probably won't end up using any I have rebased this branch and updated it to use this new approach. The tests that were failing before are no longer failing, but now a different test is failing. 😕 |
|
While I think it's always a good idea to have performance in mind, I also think it's important to balance the benefit in mind of (a) the human time-cost of benchmarking and (b) the maintainability impact of a more complex implementation. And by benefit, we should consider what the real-world impact is. If we're talking about code which will run a dozen times during the average lifecycle of a page session, I don't personally think it's worth getting too caught up in the minutia of ops/sec of various implementations†, especially if there's one option which is more obviously simple. Which is to say that, between the options: const alignAttribute = {
type: 'string',
};
if (
typeof settings.attributes === 'object' &&
settings.attributes !== null
) {
settings.attributes.align = alignAttribute;
} else {
settings.attributes = {
align: alignAttribute,
};
}...and: settings.attributes = {
...settings.attributes,
align: alignAttribute,
};I'd generally be more in favor of the second, given the circumstances. † To be clear, I don't mean to say it's wasted time. I think it can be useful knowledge to keep in mind for any implementation, especially when working in more "hot" code paths than what we're dealing with here. |
ZebulanStanphill
force-pushed
the
update/replace-lodash-assign-with-vanilla-js
branch
from
fa6559b
to
22d88e0
Compare
|
Good point; I've reverted to the spread approach. (I've also squashed a few commits and rebased the branch again.) |
ZebulanStanphill
force-pushed
the
update/replace-lodash-assign-with-vanilla-js
branch
from
22d88e0
to
ef5ee3d
Compare
ZebulanStanphill
force-pushed
the
update/replace-lodash-assign-with-vanilla-js
branch
from
ef5ee3d
to
fe86a1e
Compare
ZebulanStanphill
deleted the
update/replace-lodash-assign-with-vanilla-js
branch
Description
This PR replaces all uses of lodash.assign in the codebase with the standard JS
Object.assign. The only exception to this are some ES5 documentation examples which I left unchanged sinceObject.assignwasn't introduced until ES6.This is the first of several PRs I plan to make in order to reduce our usage of
lodashwhenever there is a simple vanilla JS equivalent, in the hopes of removing it as a dependency from several WP packages.