Reduce dependency on lodash functions: values, extends

[jayaddison]

Q	A
Patch: Bug Fix?	No
Major: Breaking Change?	No
Minor: New Feature?	No
Tests Added + Pass?	No
Any Dependency Changes?	No
License	MIT

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 by Object.keys in combination with map and object property access-by-key
• lodash/extend is replaced by use of native JavaScript objecty property iteration

[jayaddison]

(NB: Using Object.values was considered as an alternative during #11790 but was discounted since that prototype method isn't reliably available until Node 7.x)

[codesandbox-ci]

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:

Sandbox	Source
babel-repl-custom-plugin	Configuration
babel-plugin-multi-config	Configuration

[babel-bot]

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/25613/

[jayaddison]

❓ Looks like I've introduced a performance regression here somehow; the CI unit tests appear to be timing out.

[jayaddison]

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 :)

[AviVahl]

Could be that the Maps are costing way more than objects if they contain low number of items.
Also, in:

  for (const key of Object.keys(obj)) {
    if (Object.prototype.hasOwnProperty.call(obj, key)) {
      map.set(key, obj[key]);
    }
  }

You could remove the if and just set the key, as Object.keys returns own enumerable keys. The Object.prototype.hasOwnProperty lookup itself on every iteration is also costly (if not removed, at least destructured once outside the function).

[jayaddison]

Thanks @AviVahl - the timeouts during continuous integration had been occurring before the if condition was introduced, so I'm not sure that it'll turn out to be the cause, but it's good to know that it's redundant code and can be removed -- I've gone ahead and reverted it.

[JLHwung]

@jayaddison Sorry for the late reply. The performance regression, as I can observe, is not introduced by Map but an implicit infinite loop.

One of key differences between Object.keys() and Map.prototype.keys() is that the latter returns an iterator, which means if you add new element to the Map in a loop, the iterator will eventually visit the new element.

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

	for (const name of outsideRefs.keys()) {
	const id = outsideRefs.get(name);
	if (
	this.scope.hasGlobal(id.name) ||
	this.scope.parentHasBinding(id.name)
	) {
	outsideRefs.delete(id.name);
	this.letReferences.delete(id.name);
	this.scope.rename(id.name);
	this.letReferences.set(id.name, id);
	outsideRefs.set(id.name, id);
	}
	}

hangs because in every loop scope.rename() generates a new name, assigns to id.name and this name is added into outsideRefs. Since scope.rename() avoids name conflicts, the loop will never terminate.

To mitigate we can take a snapshot of all the keys in outsideRefs

[...outsideRefs.keys()]

[jayaddison]

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).

[JLHwung]

@jayaddison I am really appreciated for your efforts!

It looks like there are now some legitimate unit test failures here

I believe they are due to

babel/packages/babel-plugin-transform-block-scoping/src/tdz.js

Line 23 in e498bee

const declared = state.letReferences[node.name];

[jayaddison]

@jayaddison I am really appreciated for your efforts!

It looks like there are now some legitimate unit test failures here

I believe they are due to

babel/packages/babel-plugin-transform-block-scoping/src/tdz.js

Line 23 in e498bee

const declared = state.letReferences[node.name];

Glad to help - and thanks, yep - that was it!

[JLHwung]

Nice work, thanks.

[nicolo-ribaudo]

Thanks!