Skip to content

Conversation

@antsmartian
Copy link
Contributor

@antsmartian antsmartian commented Oct 14, 2018

This makes sure the  prototype is always detected properly.

PR-URL: https://github.com/nodejs/node/pull/22331
Fixes: https://github.com/nodejs/node/issues/22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

Backport : https://github.com/nodejs/node/pull/22331/files

  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added util Issues and PRs related to the built-in util module. v10.x labels Oct 14, 2018
@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from 92dc0a6 to e983968 Compare October 14, 2018 15:16
@antsmartian
Copy link
Contributor Author

antsmartian commented Oct 14, 2018

The build is failing (https://travis-ci.com/nodejs/node/jobs/151670153) because WeakMap and BigUint64Array had different OP on 10.x. We need the following change:

diff --git a/test/parallel/test-util-inspect.js b/test/parallel/test-util-inspect.js
index b74fe00a61..f12b49c073 100644
--- a/test/parallel/test-util-inspect.js
+++ b/test/parallel/test-util-inspect.js
@@ -1678,8 +1678,8 @@ util.inspect(process);
   [new Map([[1, 2]]), '[Map: null prototype] { 1 => 2 }'],
   [new Promise((resolve) => setTimeout(resolve, 10)),
    '[Promise: null prototype] { <pending> }'],
-  [new WeakSet(), '[WeakSet: null prototype] { <items unknown> }'],
-  [new WeakMap(), '[WeakMap: null prototype] { <items unknown> }'],
+  [new WeakSet(), '[WeakSet: null prototype] { [items unknown] }'],
+  [new WeakMap(), '[WeakMap: null prototype] { [items unknown] }'],
   [new Uint8Array(2), '[Uint8Array: null prototype] [ 0, 0 ]'],
   [new Uint16Array(2), '[Uint16Array: null prototype] [ 0, 0 ]'],
   [new Uint32Array(2), '[Uint32Array: null prototype] [ 0, 0 ]'],
@@ -1688,8 +1688,8 @@ util.inspect(process);
   [new Int32Array(2), '[Int32Array: null prototype] [ 0, 0 ]'],
   [new Float32Array(2), '[Float32Array: null prototype] [ 0, 0 ]'],
   [new Float64Array(2), '[Float64Array: null prototype] [ 0, 0 ]'],
-  [new BigInt64Array(2), '[BigInt64Array: null prototype] [ 0n, 0n ]'],
-  [new BigUint64Array(2), '[BigUint64Array: null prototype] [ 0n, 0n ]'],
+  [new BigInt64Array(2), '[BigInt64Array: null prototype] [ 0, 0 ]'],
+  [new BigUint64Array(2), '[BigUint64Array: null prototype] [ 0, 0 ]'],
   [new ArrayBuffer(16), '[ArrayBuffer: null prototype] ' +
    '{ byteLength: undefined }'],
   [new DataView(new ArrayBuffer(16)),

If I add this change, that should go as different commit or to be combined in the same commit? cc @BridgeAR @targos. Thanks!

@targos
Copy link
Member

targos commented Oct 14, 2018

You can combine in the same commit. Thanks for doing that!

@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from e983968 to 98c716d Compare October 15, 2018 01:21
@antsmartian
Copy link
Contributor Author

Thanks @targos. I have taken care those changes.

@BridgeAR
Copy link
Member

CI https://ci.nodejs.org/job/node-test-pull-request/17944/

@antsmartian
Copy link
Contributor Author

hmmm, looks like conflict. Will get rebased with master.

  This makes sure the  prototype is always detected properly.

  PR-URL: nodejs#22331
  Fixes: nodejs#22141
  Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
  Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
  Reviewed-By: James M Snell <jasnell@gmail.com>
  Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@antsmartian antsmartian force-pushed the backport-22331-to-v10.x branch from 98c716d to a39f341 Compare October 30, 2018 15:34
@antsmartian
Copy link
Contributor Author

@BridgeAR I have now rebased with staging branch, can you have a look at it and merge if all good please?

@BethGriggs
Copy link
Member

Another CI: https://ci.nodejs.org/job/node-test-pull-request/18278/

@MylesBorins
Copy link
Contributor

rebuild of OSX which failed due to infra: https://ci.nodejs.org/job/node-test-commit-osx/22376/

MylesBorins pushed a commit that referenced this pull request Nov 12, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
@MylesBorins
Copy link
Contributor

landed in 3df083c

@antsmartian
Copy link
Contributor Author

@MylesBorins Thanks!

rvagg pushed a commit that referenced this pull request Nov 28, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
MylesBorins pushed a commit that referenced this pull request Nov 29, 2018
This makes sure the  prototype is always detected properly.

Backport-PR-URL: #23655
PR-URL: #22331
Fixes: #22141
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Matteo Collina <matteo.collina@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

util Issues and PRs related to the built-in util module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants