util: use a shared symbol for util.inspect.custom #20857

chocolateboy · 2018-05-20T22:15:54Z

Define util.inspect.custom as:

Symbol.for("util.inspect.custom")

rather than:

Symbol("util.inspect.custom")

This allows util.inspect hooks to easily/safely be defined in non-Node.js environments.

The old symbol and the new symbol stringify to the same value, so much of the test code remains the same.

Fixes: #20821

Checklist

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

BridgeAR · 2018-05-20T22:23:17Z

doc/api/util.md

+can be used to define custom inspection functions in any environment.
+
+```js
+const INSPECT = Symbol.for('util.inspect.custom');


Please use lower cased variable names. We do not use capital letters to indicate constants.

BridgeAR · 2018-05-20T22:24:06Z

doc/api/util.md

+* {symbol} that can be used to declare custom inspect functions.
+
+This is a shared symbol, `Symbol.for('util.inspect.custom')`, which
+can be used to define custom inspection functions in any environment.


It might be good to also add a reference to mdn about Symbol.for() but that is not a must.

BridgeAR · 2018-05-20T22:24:33Z

@chocolateboy thanks a lot for working on this! 👍

devsnek · 2018-05-20T22:27:05Z

i think we should namespace any global symbols we create with node, otherwise looks good

chocolateboy · 2018-05-20T22:45:07Z

i think we should namespace any global symbols we create with node

I agree, but I think that should be addressed in a separate issue as there are other symbols which would also need to be updated e.g. util.promisify.custom.

BridgeAR · 2018-05-20T22:47:53Z

doc/api/util.md

+* {symbol} that can be used to declare custom inspect functions.
+
+This is a shared symbol, [`Symbol.for('util.inspect.custom')`], which is registered
+in the [global symbol registry](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for).


Would you be so kind and move the actual link to the bottom of the file? :-) That allows the text to still stay readable.

BridgeAR · 2018-05-20T22:48:37Z

@chocolateboy you mean because the name would change for the symbol?

jasnell · 2018-05-20T22:51:18Z

Marking semver-major because this impacts the global symbol namespace.

BridgeAR · 2018-05-20T22:53:48Z

If we handle this as semver-major, we should be fine to change the symbol name to node:util.inspect.custom right away.

I think it would really be good if this could be backported though... so I wonder if we should really handle this as semver-major.

mscdex · 2018-05-20T22:55:14Z

That last checkbox shouldn't be ticked, the commit messages don't follow the commit message guidelines.

addaleax

Changes to lib/ and test/ look good :)

addaleax · 2018-05-20T23:09:30Z

doc/api/util.md

+
+class Password {
+  constructor(value) {
+    this.value = value || document.getElementById('password').value;


Not really a fan of non-Node.js examples in the Node.js documentation ;)

addaleax · 2018-05-20T23:10:59Z

doc/api/util.md

+
+const password = new Password('r0sebud');
+console.log(password);
+// Prints xxxxxxxx


It might be obvious from context, but when only considering the example, it’s not all that obvious whether this prints xxxxxxxx because of .toString() or because of the [inspect]() method.

Maybe we could change the latter one to something like return `Password <${this.toString()}>`; ?

BridgeAR · 2018-05-20T23:12:10Z

@mscdex the first commit message does follow the commit guidlines. The others are not but it would be squashed anyway.

chocolateboy · 2018-05-20T23:14:54Z

@mscdex

the commit messages don't follow the commit message guidelines.

Is this an issue with the follow-up commits or the first commit?

I can't see anything here about the format of commit messages for review/feedback fixes. I assumed they would be squashed:

Note that multiple commits often get squashed when they are landed

jasnell · 2018-05-20T23:22:56Z

so I wonder if we should really handle this as semver-major

Yes, it should be semver-major.

Any uses of the new symbol would fail on LTS release lines. And if backported, there'd be a breaking difference between releases in the same LTS line

TimothyGu · 2018-05-20T23:20:23Z

doc/api/util.md

+
+This is a shared symbol, `Symbol.for('util.inspect.custom')`, which is
+registered in the [global symbol registry][]. It can be used to define
+custom inspection functions in any environment.


How about:

In addition to being accessible through `util.inspect`, this symbol is [registered globally][global symbol registry], such that it may be obtained through `Symbol.for('util.inspect.custom')`.

I've tweaked the wording here.

TimothyGu · 2018-05-20T23:21:14Z

doc/api/util.md

@@ -2095,6 +2122,7 @@ Deprecated predecessor of `console.log`.
 [`Error`]: errors.html#errors_class_error
 [`Float32Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float32Array
 [`Float64Array`]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Float64Array
+[global symbol registry]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol/for


This should get sorted after all other entries with first letter uppercase, i.e., ASCII-betically.

TimothyGu · 2018-05-20T23:23:35Z

doc/api/util.md

@@ -564,8 +564,8 @@ terminals.

 <!-- type=misc -->

-Objects may also define their own `[util.inspect.custom](depth, opts)` function
-that `util.inspect()` will invoke and use the result of when inspecting the
+Objects may also define their own `[util.inspect.custom](depth, opts)` function,


It would help if there is a link to util.inspect.custom:

Objects may also define their own <code>[[util.inspect.custom][]](depth, opts)</code> function, which ... ... [util.inspect.custom]: #util_util_inspect_custom

Is this the right syntax?

Objects may also define their own <code>[[util.inspect.custom][]](depth, opts)</code> function, which

I can't see any other examples like this.

addaleax · 2018-05-20T23:26:54Z

@jasnell Can you give an example of code that would that would be broken by backporting this to older release lines? I can’t seem to think of anything.

addaleax · 2018-05-20T23:28:45Z

doc/api/util.md

@@ -620,8 +620,35 @@ util.inspect(obj);
 added: v6.6.0
 -->


Oh, almost forgot: Can you add a changes: block here that describes the change in a succinct fashion? (compare e.g. the metadata for crypto.createCipher in crypto.md)

Yup, looks good!

jasnell · 2018-05-21T00:55:00Z

@jasnell Can you give an example of code that would that would be broken by backporting this to older release lines? I can’t seem to think of anything.

I should have said potentially breaking. Marking as major is defensive given that this consumes a symbol from the global namespace. Just need a demonstration that a backport wouldn't break. A good CITGM run should be enough if any of the modules there use the current exported symbol.

Keep in mind, however, that any code written to use the Symbol.for rather than the exported symbol won't work in current release lines. So a note should be added to that effect.

TimothyGu · 2018-05-21T01:27:18Z

test/parallel/test-util-inspect.js

+
+  // util.inspect.custom is a shared symbol which can be accessed as
+  // Symbol.for("util.inspect.custom").
+  const inspect = Symbol.for('util.inspect.custom');


Maybe

assert.strictEqual(util.inspect.custom, inspect);

also?

I could do that, but it feels rather like assert(true === true). If it fails, the other tests will fail. If it passes, the other tests are redundant.

I'd prefer this test over the other ones then.

@chocolateboy it is not like assert(true === true) because it verifies the reference equality and that is the most important part here. So yes, the other tests are redundant in that case but that should already highlight that this test is actually preferable over the others.

@BridgeAR, @TimothyGu

it is not like assert(true === true) because it verifies the reference equality

I'm not clear how that differs from or improves on assert(true === true).

that is the most important part here

I disagree. I think this change violates a tenet of testing: Test behavior, not implementation.

It's possible for:

util.inspect.custom === Symbol.for("util.inspect.custom")

- to be true and for these tests to fail because inspect accidentally or maliciously ends up checking a different symbol (e.g. util.inspect.hook). At the moment, this mismatch would also be caught by the util.inspect.custom tests preceding these tests, but that's just an unintended consequence of the way the tests are (or aren't) organized.

The equality of the references is not the goal: it's a means to an end (using the hook outside of Node.js). If JavaScript supported operator overloading or custom hash codes, they could even be different references. It's the "Does this work outside of Node.js?" that matters, and that's what these tests test.

TimothyGu · 2018-05-21T01:28:11Z

I also think we should namespace the symbol to node:.

devsnek

the symbol needs to be namespaced to node somehow

addaleax · 2018-05-21T14:29:36Z

@nodejs/tsc Can you take a look at this? I think namespacing with node is not a good idea. (i.e. I’m -1 on namespacing to node unless we have a good reason.)

It would be a different story if this was actually about a Node.js-specific concept, but as I understand it the whole point here is to enable libraries to have isomorphic support for this feature in browsers. It’s not like the idea of inspecting JS objects is tied to Node in any way.

(Also /cc @mafintosh whose https://github.com/mafintosh/inspect-custom-symbol package already uses Symbol.for('util.inspect.custom').)

addaleax · 2018-05-21T14:36:37Z

Anyway:

CI: https://ci.nodejs.org/job/node-test-commit/18648/
CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1420/

devsnek · 2018-05-21T14:56:04Z

@addaleax

It would be a different story if this was actually about a Node.js-specific concept

util.inspect is super duper node centric though. what browser mechanism matches it such that we shouldn't namespace this symbol?

addaleax · 2018-05-21T14:59:14Z

It would be a different story if this was actually about a Node.js-specific concept

util.inspect is super duper node centric though.

I mean, yes, our implementation is, but the API conceptually isn’t; and this is modifying a part of the API, not the implementation.

what browser mechanism matches it such that we shouldn't namespace this symbol?

None (and I wouldn’t expect built-in support in browsers either) – what I was trying to say is that this is a missing puzzle piece for people who want to provide a matching library for browsers.

mcollina · 2018-05-21T15:06:16Z

I'm +1 on this being semver-minor because:

> Symbol('util.custom.inspect').toString() === Symbol.for('util.custom.inspect').toString()
true

IMHO we should really discuss a policy for symbols. I might be ok if we want to namespace all of them, and maybe document them all in a single place. Namespacing only one does not resonate with me.

I think we should release it as a minor in 10, and see if there are actual breakages and bake it for LTS for a bit.

devsnek · 2018-05-21T15:06:51Z

what I was trying to say is that this is a missing puzzle piece for people who want to provide a matching library for browsers.

perhaps thats where the miscommunication lies, i wouldn't encourage the community to standardise object inspection around this symbol, unless we name it something a lot more generic (Symbol.for('custom inspect') or something) and even then i wouldn't seek this out as a connection between the ecosystems.

addaleax · 2018-05-21T15:09:36Z

@devsnek I am a bit torn between what you’re suggesting and @mcollina’s comment – being more generic seems fine to me, but I could see how that change might more realistically break somebody’s code than the current version of this patch.

Define `util.inspect.custom` as `Symbol.for("nodejs.util.inspect.custom")` rather than `Symbol("util.inspect.custom")`. This allows `inspect` hooks to easily/safely be defined in non-Node.js environments. Fixes: nodejs#20821 Refs: nodejs#22684 PR-URL: nodejs#20857 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

Define `util.inspect.custom` as `Symbol.for("nodejs.util.inspect.custom")` rather than `Symbol("util.inspect.custom")`. This allows `inspect` hooks to easily/safely be defined in non-Node.js environments. Fixes: #20821 Refs: #22684 Backport-PR-URL: #23039 PR-URL: #20857 Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com> Reviewed-By: Sakthipriyan Vairamani <thechargingvolcano@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * **url** * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * **util** * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * **Windows** * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * **Added new collaborators**: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

Notable changes: * assert * The diff output is now a tiny bit improved by sorting object properties when inspecting the values that are compared with each other. #22788 * cli * The options parser now normalizes `_` to `-` in all multi-word command-line flags, e.g. `--no_warnings` has the same effect as `--no-warnings`. #23020 * Added bash completion for the `node` binary. To generate a bash completion script, run `node --completion-bash`. The output can be saved to a file which can be sourced to enable completion. #20713 * crypto * Added support for PEM-level encryption. #23151 * Added an API asymmetric key pair generation. The new methods `crypto.generateKeyPair` and `crypto.generateKeyPairSync` can be used to generate public and private key pairs. The API supports RSA, DSA and EC and a variety of key encodings (both PEM and DER). #22660 * fs * Added a `recursive` option to `fs.mkdir` and `fs.mkdirSync`. If this option is set to true, non-existing parent folders will be automatically created. #21875 * http2 * Added a `'ping'` event to `Http2Session` that is emitted whenever a non-ack `PING` is received. #23009 * Added support for the `ORIGIN` frame. #22956 * Updated nghttp2 to 1.34.0. This adds RFC 8441 extended connect protocol support to allow use of WebSockets over HTTP/2. #23284 * module * Added `module.createRequireFromPath(filename)`. This new method can be used to create a custom require function that will resolve modules relative to the filename path. #19360 * process * Added a `'multipleResolves'` process event that is emitted whenever a `Promise` is attempted to be resolved multiple times, e.g. if the `resolve` and `reject` functions are both called in a `Promise` executor. #22218 * url * Added `url.fileURLToPath(url)` and `url.pathToFileURL(path)`. These methods can be used to correctly convert between file: URLs and absolute paths. #22506 * util * Added the `sorted` option to `util.inspect()`. If set to `true`, all properties of an object and Set and Map entries will be sorted in the returned string. If set to a function, it is used as a compare function. #22788 * The `util.instpect.custom` symbol is now defined in the global symbol registry as `Symbol.for('nodejs.util.inspect.custom')`. #20857 * Added support for `BigInt` numbers in `util.format()`. #22097 * V8 API * A number of V8 C++ APIs have been marked as deprecated since they have been removed in the upstream repository. Replacement APIs are added where necessary. #23159 * Windows * The Windows msi installer now provides an option to automatically install the tools required to build native modules. #22645 * Workers * Debugging support for Workers using the DevTools protocol has been implemented. #21364 * The public `inspector` module is now enabled in Workers. #22769 * Added new collaborators: * digitalinfinity - Hitesh Kanwathirtha PR-URL: #23313

Ports the `buffer-inspect` test from Node core. Adds a `Buffer.prototype[util.inspect.custom]` method, which is an alias to `Buffer.prototype.inspect`. Node already doesn't have an `.inspect` method on Buffers anymore, but since this module has to work in browsers that do not have Symbols, it seems better to keep it around. In Node, this will use the builtin `util.inspect.custom` symbol. In the browser, it will use `Symbol.for('util.inspect.custom')`. The browser version of `util` will also use the `inspect-custom-symbol` module in the near future. If nodejs/node#20857 gets merged, `Symbol.for('util.inspect.custom')` will be used everywhere and the dependency on `inspect-custom-symbol` could probably be dropped. The motivation for this is API parity and the fact that Node is removing support for the old `.inspect` method: nodejs/node#20722

nodejs-github-bot added the util Issues and PRs related to the built-in util module. label May 20, 2018

BridgeAR approved these changes May 20, 2018

View reviewed changes

BridgeAR reviewed May 20, 2018

View reviewed changes

jasnell added the semver-major PRs that contain breaking changes and should be released in the next major version. label May 20, 2018

addaleax approved these changes May 20, 2018

View reviewed changes

TimothyGu reviewed May 20, 2018

View reviewed changes

addaleax reviewed May 20, 2018

View reviewed changes

TimothyGu approved these changes May 21, 2018

View reviewed changes

thefourtheye approved these changes May 21, 2018

View reviewed changes

devsnek previously requested changes May 21, 2018

View reviewed changes

targos mentioned this pull request Sep 23, 2018

[v10.x] Backport util PRs #23039

Closed

targos added backported-to-v10.x and removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v10.x labels Sep 24, 2018

oyyd mentioned this pull request Sep 26, 2018

[v10.x] Backport assert PRs #23095

Closed

2 tasks

targos mentioned this pull request Oct 7, 2018

Release proposal: v10.12.0 #23313

Merged

ExE-Boss mentioned this pull request Feb 5, 2020

Support util.promisify.custom as a global symbol #31647

Closed

ExE-Boss mentioned this pull request Mar 16, 2020

util: use a global symbol for util.promisify.custom #31672

Closed

4 tasks

davidchambers mentioned this pull request Jun 16, 2021

provide custom inspect behaviour in Node without using require sanctuary-js/sanctuary-def#303

Merged

util: use a shared symbol for util.inspect.custom #20857

util: use a shared symbol for util.inspect.custom #20857

Conversation

chocolateboy commented May 20, 2018 • edited by BridgeAR Loading

Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 20, 2018

devsnek commented May 20, 2018

chocolateboy commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 20, 2018

jasnell commented May 20, 2018

BridgeAR commented May 20, 2018

mscdex commented May 20, 2018

addaleax left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BridgeAR commented May 20, 2018

chocolateboy commented May 20, 2018 • edited Loading

jasnell commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

addaleax commented May 20, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasnell commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chocolateboy May 21, 2018 • edited Loading

Choose a reason for hiding this comment

TimothyGu commented May 21, 2018

devsnek left a comment

Choose a reason for hiding this comment

addaleax commented May 21, 2018

addaleax commented May 21, 2018

devsnek commented May 21, 2018

addaleax commented May 21, 2018

mcollina commented May 21, 2018

devsnek commented May 21, 2018 • edited Loading

addaleax commented May 21, 2018

chocolateboy commented May 20, 2018 •

edited by BridgeAR

Loading

chocolateboy commented May 20, 2018 •

edited

Loading

chocolateboy May 21, 2018 •

edited

Loading

devsnek commented May 21, 2018 •

edited

Loading