Skip to content
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

[v10.x backport] util: handle null prototype on inspect #23655

Closed

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