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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>

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 <[email protected]>
  Reviewed-By: Matteo Collina <[email protected]>
  Reviewed-By: James M Snell <[email protected]>
  Reviewed-By: John-David Dalton <[email protected]>
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
@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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
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 <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
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