Skip to content

deps: update V8 to 7.0#22754

Closed
targos wants to merge 8 commits intonodejs:masterfrom
targos:v8-7.0
Closed

deps: update V8 to 7.0#22754
targos wants to merge 8 commits intonodejs:masterfrom
targos:v8-7.0

Conversation

@targos
Copy link
Member

@targos targos commented Sep 7, 2018 •
edited
Loading

ETA: Oct 16th, 2018

Issues to fix:

@targos targos added this to the 11.0.0 milestone Sep 7, 2018
@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. v8 engine Issues and PRs related to the V8 dependency. labels Sep 7, 2018
@targos targos removed the build Issues and PRs related to build files or the CI. label Sep 7, 2018
@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented Sep 7, 2018

@targos build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/826/pipeline

@targos targos added the semver-major PRs that contain breaking changes and should be released in the next major version. label Sep 7, 2018
@targos
Copy link
Member Author

targos commented Sep 7, 2018 •
edited
Loading

CI: https://ci.nodejs.org/job/node-test-pull-request/17080/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1672/

@targos targos added the blocked PRs that are blocked by other issues or PRs. label Sep 7, 2018
@vsemozhetbyt
Copy link
Contributor

vsemozhetbyt commented Sep 7, 2018

Hello, stable Array.prototype.sort().

@targos
Copy link
Member Author

targos commented Sep 8, 2018

This version is also affected by nodejs/node-v8#77

/cc @refack who's been working on it.

@targos
Copy link
Member Author

targos commented Sep 9, 2018

Pushed temporary fixes for nodejs/node-v8#77 and nodejs/node-v8#78.

CI: https://ci.nodejs.org/job/node-test-pull-request/17087/
V8: https://ci.nodejs.org/job/node-test-commit-v8-linux/1675/

@AyushG3112
Copy link
Contributor

AyushG3112 commented Sep 9, 2018

Just confirming, this will release with nodejs 11 right?

@targos
Copy link
Member Author

targos commented Sep 9, 2018

@AyushG3112 That's the plan

@targos
Copy link
Member Author

targos commented Sep 9, 2018

@nodejs/platform-smartos There is a small error with src/v8ustack.d:

01:17:47 dtrace: failed to compile script src/v8ustack.d: line 402: failed to resolve V8DBG_CLASS_SHAREDFUNCTIONINFO__FUNCTION_IDENTIFIER_OR_DEBUG_INFO__OBJECT: Unknown variable name

@targos
Copy link
Member Author

targos commented Sep 9, 2018

There are missing postmortem metadata constants:

01:24:58     AssertionError [ERR_ASSERTION]: Missing constants: v8dbg_class_SharedFunctionInfo__function_identifier_or_debug_info__Object,v8dbg_class_SharedFunctionInfo__script__Object

@targos
Copy link
Member Author

targos commented Sep 9, 2018

@nodejs/platform-ppc something related to WASM seems broken:

https://ci.nodejs.org/job/node-test-commit-v8-linux/1675/

@targos
Copy link
Member Author

targos commented Sep 12, 2018

Ping @nodejs/platform-smartos @nodejs/platform-ppc

@mhdawson
Copy link
Member

mhdawson commented Sep 13, 2018

@john-yan Please look at the ppc issue and comment here.

@john-yan
Copy link

john-yan commented Sep 13, 2018

Hello Michael, I am aware of the errors and trying to fix asap

@john-yan
Copy link

john-yan commented Sep 13, 2018

Hello @targos , the ppc Atomic errors shown in https://ci.nodejs.org/job/node-test-commit-v8-linux/1675/#showFailuresLink are skipped by https://chromium-review.googlesource.com/c/v8/v8/+/1217003 as I64Atomic operations are not yet supported on the platform.

@targos targos force-pushed the v8-7.0 branch 2 times, most recently from 761f10e to fe5cc29 Compare September 17, 2018 10:42
@targos
Copy link
Member Author

targos commented Sep 17, 2018

Thanks @john-yan, I just included the changes.

CI: https://ci.nodejs.org/job/node-test-pull-request/17231/
V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/1688/

@targos
Copy link
Member Author

targos commented Sep 17, 2018

@cjihrig maybe you can have a look at the postmortem issues?

There is a small error with src/v8ustack.d:

01:17:47 dtrace: failed to compile script src/v8ustack.d: line 402: failed to resolve V8DBG_CLASS_SHAREDFUNCTIONINFO__FUNCTION_IDENTIFIER_OR_DEBUG_INFO__OBJECT: Unknown variable name

There are missing postmortem metadata constants:

01:24:58     AssertionError [ERR_ASSERTION]: Missing constants: v8dbg_class_SharedFunctionInfo__function_identifier_or_debug_info__Object,v8dbg_class_SharedFunctionInfo__script__Object

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2018

I'll take a look.

@cjihrig
Copy link
Contributor

cjihrig commented Sep 17, 2018

@targos please cherry pick cjihrig@3ea5acc for the test fix and cjihrig@38a738d for SmartOS compilation.

@targos
Copy link
Member Author

targos commented Sep 17, 2018

@cjihrig Done. Thanks a lot!

@targos
Copy link
Member Author

targos commented Sep 17, 2018

Only nodejs/node-v8#78 remains. /cc @ofrobots

@joyeecheung joyeecheung mentioned this pull request Sep 18, 2018
Closed
jasnell added a commit that referenced this pull request Oct 21, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
ab20ac4 
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
jasnell added a commit that referenced this pull request Oct 22, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
cea049b 
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](#21914)
devsnek pushed a commit to devsnek/node that referenced this pull request Oct 23, 2018
@jasnell
2018-10-23, Version 11.0.0 (Current)
3516052 
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[nodejs#22617](nodejs#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [nodejs#21316](nodejs#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [nodejs#21649](nodejs#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [nodejs#20442](nodejs#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [nodejs#22754](nodejs#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [nodejs#22146](nodejs#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[nodejs#20735](nodejs#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [nodejs#20270](nodejs#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [nodejs#22951](nodejs#22951)
* Internal
  * Windows performance-counter support has been removed.
    [nodejs#22485](nodejs#22485)
  * The `--expose-http2` command-line option has been removed.
    [nodejs#20887](nodejs#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [nodejs#20002](nodejs#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [nodejs#22281](nodejs#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [nodejs#22756](nodejs#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [nodejs#21914](nodejs#21914)
@refack refack mentioned this pull request Nov 5, 2018
Closed
@Hypnosphi Hypnosphi mentioned this pull request Nov 5, 2018
Merged
@refack refack added landed and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. labels Nov 6, 2018
@richardlau richardlau mentioned this pull request Nov 10, 2018
Closed
@refack
Copy link
Contributor

refack commented Nov 11, 2018

Adding an extra dont-land-on-v10.x due to array.sort implementation change.

@targos
Copy link
Member Author

targos commented Nov 11, 2018

It's Semver-major. There's no risk for it to land on v10.x

@gabrielschulhof gabrielschulhof mentioned this pull request Dec 6, 2018
Open
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 10, 2018
@jasnell @deepak1556
2018-10-23, Version 11.0.0 (Current)
fffd852 
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](nodejs/node#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](nodejs/node#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](nodejs/node#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](nodejs/node#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](nodejs/node#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](nodejs/node#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](nodejs/node#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](nodejs/node#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](nodejs/node#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](nodejs/node#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](nodejs/node#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](nodejs/node#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](nodejs/node#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](nodejs/node#21914)
deepak1556 pushed a commit to electron/node that referenced this pull request Dec 19, 2018
@jasnell @deepak1556
2018-10-23, Version 11.0.0 (Current)
0b2f9b4 
Notable changes:

* Build
  * FreeBSD 10 is no longer supported.[#22617](nodejs/node#22617)
* `child_process`
  * The default value of the `windowsHide` option has been changed
    to `true`. [#21316](nodejs/node#21316)
* `console`
  * `console.countReset()` will emit a warning if the timer
    being reset does not exist. [#21649](nodejs/node#21649)
  * `console.time()` will no longer reset a timer if it already
    exists. [#20442](nodejs/node#20442)
* Dependencies
  * V8 has been updated to 7.0.
    [#22754](nodejs/node#22754)
* `fs`
  * The `fs.read()` method now requires a callback.
    [#22146](nodejs/node#22146)
  * The previously deprecated `fs.SyncWriteStream` utility has been
    removed.[#20735](nodejs/node#20735)
* `http`
  * The `http`, `https`, and `tls` modules now use the WHATWG URL parser
    by default. [#20270](nodejs/node#20270)
* General
  * Use of `process.binding()` has been deprecated. Userland code using
    `process.binding()` should re-evaluate that use and begin migrating. If
    there are no supported API alternatives, please open an issue in the
    Node.js GitHub repository so that a suitable alternative may be discussed.
  * An experimental implementation of `queueMicrotask()` has been added.
    [#22951](nodejs/node#22951)
* Internal
  * Windows performance-counter support has been removed.
    [#22485](nodejs/node#22485)
  * The `--expose-http2` command-line option has been removed.
    [#20887](nodejs/node#20887)
* Timers
  * Interval timers will be rescheduled even if previous interval threw
    an error. [#20002](nodejs/node#20002)
* `util`
  * The WHATWG `TextEncoder` and `TextDecoder` are now globals.
    [#22281](nodejs/node#22281)
  * `util.inspect()` output size is limited to 128 MB by default.
    [#22756](nodejs/node#22756)
  * A runtime warning will be emitted when `NODE_DEBUG` is set for
    either `http` or `http2`. [#21914](nodejs/node#21914)
@orangejulius orangejulius mentioned this pull request Jan 14, 2019
Merged
@JamieMagee JamieMagee mentioned this pull request Apr 16, 2019
Merged
rarkins pushed a commit to renovatebot/renovate that referenced this pull request Apr 16, 2019
@JamieMagee @rarkins
fix: nodejs 11 support (#3549)
05a46dd 
Due to an update in the v8 runtime, Node.js `Array.prototype.sort()` is now stable (See [here](nodejs/node#22754 (comment))).

These changes allow for tests to pass on both Node.js 10 and 11.

Fixes #3445
@Hakerh400 Hakerh400 mentioned this pull request Jul 26, 2019
Closed
@nolleto nolleto mentioned this pull request Oct 21, 2019
Merged
@slowbackspace slowbackspace mentioned this pull request Oct 31, 2019
Closed
das7pad added a commit to das7pad/web-sharelatex that referenced this pull request Nov 5, 2019
@das7pad
[ProjectController] fix the sorting of project entities for node v11+
85dc1bf 
For the signature `Array.prototype.sort((a, b) => rt)`
 `rt` encodes the sorting positions as follows [1]:
 - `rt < 0`: a should go first
 - `rt = 0`: same position
 - `rt > 0`: b should go first

Node version 11 bundles an updated v8 engine [2]. One of the changes is
 a different sorting algorithm for Array.prototype.sort.
 Quicksort was replaced by Timsort[3], a stable sorting algorithm.
Node v12 (new LTS) has the same updated behaviour.

For an arbitrary sorted array every comparision would yield 0 or 1.
Our comparision function using `rt = a > b`, is not sufficient anymore,
 as it would yield the signature of a sorted array and no changes are
 made to the sequence accordingly.
The fix is simple: adjust the return value of 0 to -1.

There should be only one entity per unique path, so we can omit the rt=0
 case.

Here is a minimal demo of the unit test

  test/unit/src/Project/ProjectControllerTests.js
  "ProjectController"
  -> "projectEntitiesJson"
  -> "should produce a list of entities"

For future reference I kept the nvm output referencing the used node
 version (Running node ...) in place.

Using the current sorting function
```
$ nvm run v10 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v10.17.0 (npm v6.12.1)
comparing /things/b.txt /main.tex true
comparing /things/b.txt /things/a.txt true
comparing /main.tex /things/a.txt false
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]

$ nvm run v11 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v11.15.0 (npm v6.12.1)
comparing /main.tex /things/b.txt false
comparing /things/a.txt /main.tex true
[ { path: '/things/b.txt', type: 'doc' },
  { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' } ]
```

Using the adjusted return value
```
$ nvm run v10 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path ? 1 : -1
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v10.17.0 (npm v6.12.1)
comparing /things/b.txt /main.tex 1
comparing /things/b.txt /things/a.txt 1
comparing /main.tex /things/a.txt -1
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]

$ nvm run v11 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path ? 1 : -1
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v11.15.0 (npm v6.12.1)
comparing /main.tex /things/b.txt -1
comparing /things/a.txt /main.tex 1
comparing /things/a.txt /things/b.txt -1
comparing /things/a.txt /main.tex 1
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]
```

---
[1] https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#Description
[2] nodejs/node#22754
[3] https://chromium-review.googlesource.com/c/v8/v8/+/1186801

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
@das7pad das7pad mentioned this pull request Nov 5, 2019
Closed
das7pad added a commit to das7pad/web-sharelatex that referenced this pull request Nov 20, 2019
@das7pad
[ProjectController] fix the sorting of project entities for node v11+
633c7a6 
For the signature `Array.prototype.sort((a, b) => rt)`
 `rt` encodes the sorting positions as follows [1]:
 - `rt < 0`: a should go first
 - `rt = 0`: same position
 - `rt > 0`: b should go first

Node version 11 bundles an updated v8 engine [2]. One of the changes is
 a different sorting algorithm for Array.prototype.sort.
 Quicksort was replaced by Timsort[3], a stable sorting algorithm.
Node v12 (new LTS) has the same updated behaviour.

For an arbitrary sorted array every comparision would yield 0 or 1.
Our comparision function using `rt = a > b`, is not sufficient anymore,
 as it would yield the signature of a sorted array and no changes are
 made to the sequence accordingly.
The fix is simple: adjust the return value of 0 to -1.

There should be only one entity per unique path, so we can omit the rt=0
 case.

Here is a minimal demo of the unit test

  test/unit/src/Project/ProjectControllerTests.js
  "ProjectController"
  -> "projectEntitiesJson"
  -> "should produce a list of entities"

For future reference I kept the nvm output referencing the used node
 version (Running node ...) in place.

Using the current sorting function
```
$ nvm run v10 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v10.17.0 (npm v6.12.1)
comparing /things/b.txt /main.tex true
comparing /things/b.txt /things/a.txt true
comparing /main.tex /things/a.txt false
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]

$ nvm run v11 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v11.15.0 (npm v6.12.1)
comparing /main.tex /things/b.txt false
comparing /things/a.txt /main.tex true
[ { path: '/things/b.txt', type: 'doc' },
  { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' } ]
```

Using the adjusted return value
```
$ nvm run v10 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path ? 1 : -1
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v10.17.0 (npm v6.12.1)
comparing /things/b.txt /main.tex 1
comparing /things/b.txt /things/a.txt 1
comparing /main.tex /things/a.txt -1
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]

$ nvm run v11 -e 'console.log(
[{ path: "/things/b.txt", type: "doc" },
 { path: "/main.tex", type: "doc" },
 { path: "/things/a.txt", type: "file" }
].sort((a, b) => {
  const rt = a.path > b.path ? 1 : -1
  console.log("comparing", a.path, b.path, rt)
  return rt}))'
Running node v11.15.0 (npm v6.12.1)
comparing /main.tex /things/b.txt -1
comparing /things/a.txt /main.tex 1
comparing /things/a.txt /things/b.txt -1
comparing /things/a.txt /main.tex 1
[ { path: '/main.tex', type: 'doc' },
  { path: '/things/a.txt', type: 'file' },
  { path: '/things/b.txt', type: 'doc' } ]
```

---
[1] https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Array/sort#Description
[2] nodejs/node#22754
[3] https://chromium-review.googlesource.com/c/v8/v8/+/1186801

Signed-off-by: Jakob Ackermann <das7pad@outlook.com>
@owaiswiz owaiswiz mentioned this pull request Mar 28, 2020
Closed
MunifTanjim added a commit to Synor/synor that referenced this pull request Nov 17, 2020
@MunifTanjim
test(core): update utils/sort expected result
13cd663 
Align with stable `Array.prototype.sort` in Node.js 12.x

References:
  - https://v8.dev/features/stable-sort
  - nodejs/node#22754 (comment)
@MunifTanjim MunifTanjim mentioned this pull request Nov 17, 2020
Merged
MunifTanjim added a commit to Synor/synor that referenced this pull request Nov 17, 2020
@MunifTanjim
test(core): update utils/sort expected result
3a0aef1 
Align with stable `Array.prototype.sort` in Node.js 12.x

References:
  - https://v8.dev/features/stable-sort
  - nodejs/node#22754 (comment)
MunifTanjim added a commit to Synor/synor that referenced this pull request Nov 17, 2020
@MunifTanjim
test(core): update utils/sort expected result
c6cc6e2 
Align with stable `Array.prototype.sort` in Node.js 12.x

References:
  - https://v8.dev/features/stable-sort
  - nodejs/node#22754 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mcollina mcollina mcollina approved these changes

@jasnell jasnell jasnell approved these changes

@cjihrig cjihrig cjihrig approved these changes

+2 more reviewers

@ofrobots ofrobots ofrobots approved these changes

@refack refack refack approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

semver-major PRs that contain breaking changes and should be released in the next major version. v8 engine Issues and PRs related to the V8 dependency.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.