Replace lodash 'sortBy' usage with Array.prototype.sort

[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

This is likely a lower-priority change now that lodash 4.17.19 has been released and resolves a vulnerability that caused package audit warnings and noise for many users; that was the original motivation for removing some of these functions as per #11789.

That said, there still seems to be scope for further dependency reduction, so this changeset is an attempt to remove one more lodash reference.

[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 4f5673b:

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/32439/

[jayaddison]

Still puzzling. One possibility is that the sort order may be reversed at the moment, based on some unit test failures where the received statements are flipped around from the expected statements:

Expected: "import _source from \"source\"; _source;"
Received: "_source; import _source from \"source\";"

I'm just doing some sorting via the node shell with lodash.sortBy and Array.prototype.sort to try to determine whether that's the cause. The comparator function could be the source of the trouble (and perhaps just reversing the division operands around is the solution, but I'd like to confirm that first).

[jayaddison]

Yep, the current b - a comparator ordering when used with Array.prototype.sort doesn't match lodash/sortBy; so I think these need to be flipped around. A quick justification:

> sortBy = require('lodash/sortBy')
[Function]
> arr = [{"a": 1}, {"a": -1}, {"a": 3}]
[ { a: 1 }, { a: -1 }, { a: 3 } ]
> sortFn = x => x.a * -1;
[Function: sortFn]
> sortBy(arr, sortFn)
[ { a: 3 }, { a: 1 }, { a: -1 } ]
> arr.sort((a, b) => sortFn(b) - sortFn(a))
[ { a: -1 }, { a: 1 }, { a: 3 } ]
> 

[nicolo-ribaudo]

Thanks for investigating!

[jayaddison]

I'm a bit puzzled about why the latest Travis checks are failing here for Node < v12. There must be a language feature in use that's only supported in later versions?

[coreyfarrell]

I'm a bit puzzled about why the latest Travis checks are failing here for Node < v12. There must be a language feature in use that's only supported in later versions?

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#Browser_compatibility

Apparently Array.prototype.sort wasn't a stable sort before node.js 12. It looks like lodash deals with this by wrapping array items so the internal comparitor has access to each item's initial index.

The following code demonstrates this problem:

const items = [];

for (let i = 0; i < 20; i++) {
  items.push({idx: i, value: 'a'});
}

const original = process.versions.node.split('.')[0] < 12 ? new Map(items.map((v, idx) => [v, idx])) : undefined;

items[10].value = 'b';
items.sort((a, b) => {
  if (a.value < b.value) {
    return -1;
  }

  if (a.value > b.value) {
    return 1;
  }

  // This if statement makes the sort stable for node.js < 12, uses a Map to find original indexes.
  if (original) {
    return original.get(a) - original.get(b);
  }

  return 0;
});

console.log(items);

[jayaddison]

Well, today's turning into a good day for learning software archaeology; thank you @coreyfarrell :)

[coreyfarrell]

@jayaddison I just did some more experimentation with my sort stability demo, found an easier way to get it working for older node. See my previous message I've edited the example code to use a Map when needed. Not sure this will be acceptable to do in the babel code just trying to find a way to prevent unstable sort from being a blocker.

[jayaddison]

@coreyfarrell Nice! Just to rephrase it as I understand it: for Node versions prior to v12, we store a map keyed on each node object and storing the array index for that object as a value. If that map exists during sorting, then it's used as a tie-breaker to ensure stable sort behaviour.

Nice approach. I'll start incorporating that here.

[jayaddison]

(we might also want to keep the 'Sorting with map' section from https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#Browser_compatibility in mind, particularly if any of the performance concerns mentioned there crop up)

[coreyfarrell]

This seems good to me. Only remaining question does the access to process.versions need to guard against running outside Node.js? Maybe (typeof process !== 'undefined' ? process.version.node.split(".")[0] : 0)?

[coreyfarrell]

Suggested change

	: undefined;
	const stabilityMap =
	!nodeVersion || nodeVersion < 12
	: 0;
	const stabilityMap =
	nodeVersion < 12

Feel free to ignore this suggestion, just wanted to point out that the conditional can be simplified if you make browsers produce nodeVersion === 0.

[jayaddison]

Thanks @coreyfarrell - I think I'd prefer to keep the meaning/semantics of nodeVersion closer to the ground truth (i.e. it'll genuinely have an undefined value in non-Node environments), even though I'd admit that it's (hopefully) unlikely that anyone would be line-by-line debugging the value. I'm sure I've probably used tricks like this to simplify my own code elsewhere too, so it's a bit arbitrary of me not to accept. But I think I'd prefer the more verbose condition in this case.

[jayaddison]

NB: There's a change to the semver.intersects logic from semver v6.0.0 onwards; that might be worth looking at, particulary in relation to the comments here:

babel/packages/babel-core/src/transformation/file/file.js

Lines 141 to 158 in 5bbad89

	// semver.intersects() has some surprising behavior with comparing ranges
	// with pre-release versions. We add '^' to ensure that we are always
	// comparing ranges with ranges, which sidesteps this logic.
	// For example:
	//
	// semver.intersects(`<7.0.1`, "7.0.0-beta.0") // false - surprising
	// semver.intersects(`<7.0.1`, "^7.0.0-beta.0") // true - expected
	//
	// This is because the first falls back to
	//
	// semver.satisfies("7.0.0-beta.0", `<7.0.1`) // false - surprising
	//
	// and this fails because a prerelease version can only satisfy a range
	// if it is a prerelease within the same major/minor/patch range.
	//
	// Note: If this is found to have issues, please also revisit the logic in
	// transform-runtime's definitions.js file.
	if (semver.valid(versionRange)) versionRange = `^${versionRange}`;

[jayaddison]

(closing and cleaning up a few stale pull requests; see #11789 (comment) and subsequent comments in case it's useful to re-open from github pr refs at a later date)