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
Reduce dependency on lodash functions: values, extends #11798
Reduce dependency on lodash functions: values, extends #11798
Conversation
(NB: Using |
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 2b03b2e:
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25613/ |
❓ Looks like I've introduced a performance regression here somehow; the CI unit tests appear to be timing out. |
This reverts commit abdd147.
… difference per https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Map )" This reverts commit a4035c8.
I'm pretty confused about how/where I might have introduced the performance regression here - I'd appreciate any ideas and suggestions on how to track that down :) |
Could be that the for (const key of Object.keys(obj)) {
if (Object.prototype.hasOwnProperty.call(obj, key)) {
map.set(key, obj[key]);
}
} You could remove the |
…all" This reverts commit 491c093.
Thanks @AviVahl - the timeouts during continuous integration had been occurring before the |
…; access performance appears much improved for Object property access
@jayaddison Sorry for the late reply. The performance regression, as I can observe, is not introduced by One of key differences between In the following example, the for loop is infinite because we keep adding new key to the map. (0, -1, 0, -1 ...) though the original map has only one element ( 0 => undefined ). const map = new Map([[0]]);
for (const k of map.keys()) {
a.delete(k);
a.set(~k, undefined);
} Now let's take a look at our codebase, babel/packages/babel-plugin-transform-block-scoping/src/index.js Lines 526 to 541 in bea1b56
hangs because in every loop To mitigate we can take a snapshot of all the keys in [...outsideRefs.keys()] |
Brilliant, thanks @JLHwung - oddly an infinite loop was my initial guess for the CI problems but I couldn't figure out where that might have been happening, your explanation makes complete sense. It looks like there are now some legitimate unit test failures here; I'm finishing up for the day but will take a look at those soon (and will finally make another effort to get unit tests running correctly locally). |
@jayaddison I am really appreciated for your efforts!
I believe they are due to
|
Glad to help - and thanks, yep - that was it! |
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.
Nice work, thanks.
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!
As mentioned by @hzoo in #11790 (comment), we can remove the (only) use of
lodash.values
within the codebase with a slightly longer-form but logically-equivalent iteration over the target object properties followed by retrieval of each object value.lodash/values
is replaced byObject.keys
in combination withmap
and object property access-by-keylodash/extend
is replaced by use of native JavaScript objecty property iteration