util: deprecate obj.inspect for custom inspection by Trott · Pull Request #15631 · nodejs/node · GitHub

The existence of obj.inspect() for custom inspection can cause people
to unintentionally break console.log() and friends. This is a
documentation-only deprecation that can hopefully land in 8.x.

Refs: #15549

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util

not sure, but maybe "equivalent but deprecated"?

Yes, you're correct. Even better might be or the equivalent but deprecated which is what I'll switch it to in a minute.

This is a documentation-only deprecation that can hopefully land in 8.x.

Just to give context, the alternative was only introduced a little more than a year ago in 6.4.0. I am perfectly fine with landing this in 8.x but I’d like to wait a little longer until we consider a runtime-deprecation.

Labeling semver-minor as our docs indicate that is the minimum for a doc-only deprecation.

can we check ecosystem usage of this before we land this?

can we check ecosystem usage of this before we land this?

@evanlucas The same question was asked in #15549 and @refack answered (in #15549 (comment)) like this:

@ChALkeR @refack would you be so kind and try finding out how often this might be used?

It's a tricky query to run... preliminary results indicate "no very often".

Compatibility risks are always there, but util.inspect will still give an output for objects after this change, just a less helpful one.

Just a reminder, deprecation is still far from removal (a full cycle needs at least two major versions). And even after a "full deprecation" removal is not automatic. So IMHO if it's a "bad" API that already has a better implementation, deciding to deprecate should be easier.

+1 as a docs-only deprecation

can we check ecosystem usage of this before we land this?

querying for /\binspect\b/ results in more than 50KLOCs. In a partial manual review, I only picked up Q - #15549 (comment)

Can someone confirm; will util.inspect fall back to the string "inspect" if util.inspect.custom is not available? Is there a plan in the future to change this behavior?

Yes, the string inspect will still work. This is a docs only deprecation for now. Which means no runtime changes at all

It might be helpful to note that, if compatibility is desired, having the method present under both names is always going to work?

I like the idea but I'm struggling with how to word it in a way that doesn't encourage people to use both if they're only using the Symbol. We don't want to create more messages if/when we move to a runtime deprecation. Suggestions welcome.

@Trott The thing is, even if we move to a runtime deprecation, the symbol property will be the first one that’s detected, and the string property ignored – so if both are present, the runtime deprecation won’t be emitted… or am I misunderstanding what you’re saying?

@addaleax Good point. I've added some text in a separate commit.

Can someone confirm; will util.inspect fall back to the string "inspect" if util.inspect.custom is not available? Is there a plan in the future to change this behavior?

AFAICT the long term "plan" is to runtime deprecate then remove, but that cycle requires at least two major versions, and future consensus.

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. Refs: nodejs#15549

instead (For backwards compatibility with Node.js prior to version 6.4.0, both may be specified). ?

I think since it's a whole sentence (not just a clause) inside the parentheses the period should be inside.

http://www.thepunctuationguide.com/parentheses.html

Perhaps the parentheses are unnecessary anyway?

Landed in c09c04f

I removed the braces while landing.

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. PR-URL: #15631 Refs: #15549 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

This does not land cleanly on 8.x. Could you please manually backport.

nvm I figured it out ☺️

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. PR-URL: #15631 Refs: #15549 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. PR-URL: nodejs/node#15631 Refs: nodejs/node#15549 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. PR-URL: #15631 Refs: #15549 Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: Luigi Pinca <luigipinca@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>

Notable Changes: * deps: * update npm to 5.4.2 #15600 * upgrade libuv to 1.15.0 #15745 * update V8 to 6.1.534.42 #15393 * dgram: * support for setting dgram socket buffer size #13623 * fs: * add support O_DSYNC file open constant #15451 * util: * deprecate obj.inspect for custom inspection #15631 * tools, build: * there is a fancy new macOS installer #15179 * Added new collaborator * bmeurer - Benedikt Meurer - https://github.com/bmeurer * kfarnung - Kyle Farnung - https://github.com/kfarnung PR-URL: #15762

Notable Changes: * deps: * update npm to 5.4.2 nodejs/node#15600 * upgrade libuv to 1.15.0 nodejs/node#15745 * update V8 to 6.1.534.42 nodejs/node#15393 * dgram: * support for setting dgram socket buffer size nodejs/node#13623 * fs: * add support O_DSYNC file open constant nodejs/node#15451 * util: * deprecate obj.inspect for custom inspection nodejs/node#15631 * tools, build: * there is a fancy new macOS installer nodejs/node#15179 * Added new collaborator * bmeurer - Benedikt Meurer - https://github.com/bmeurer * kfarnung - Kyle Farnung - https://github.com/kfarnung PR-URL: nodejs/node#15762

I saw the change, read documentation and played with the Node and the code, and still cannot quite get it...

In order to provide custom console output for each of my custom types I used to do this:

MyType.prototype.inspect = function(){
    return 'CUSTOM VALUE';
}

How am I to change this code for the latest Node.js?

@vitaly-t basically, whenever you want your custom inspect function and care about older Node versions, also do:

MyType.prototype.inspect = function(){
    return 'CUSTOM VALUE';
};
if (util.inspect.custom) {
  MyType.prototype[util.inspect.custom] = MyType.prototype.inspect;
}

If you do not care about Node < 6.4.0, you can (and probably should) just use

MyType.prototype[util.inspect.custom] = function(){
    return 'CUSTOM VALUE';
};

The reason for this is that it’s hard to tell for Node whether inspect was meant to be a custom inspection function for util.inspect, or somebody accidentally called it that way. With a symbol, that problem goes away.

@addaleax Thank you! But if this is the case, then why the first code below works fine, while the one after doesn't?

Works:

const obj = {
    inspect: () => {
        return 'INSPECT';
    }
};
console.log(obj);
//=> INSPECT

Doesn't work:

const obj = {
    util: {
        inspect: {
            custom: () => {
                return 'INSPECT';
            }
        }
    }
};
console.log(obj);
//=> { util: { inspect: { custom: [Function: custom] } } }

Node.js v8.7.0

@vitaly-t it's require('util').inspect.custom, eg:

const obj = {
  [require('util').inspect.custom]() {
    return 'INSPECT';
  }
};
console.log(obj);

@ljharb Ouch,... tricky! I think documentation needs to get better on this one 😄 Thank you!

Also, I'd like to figure out what that require('util').inspect.custom really returns. I can only see in the console for it saying Symbol(util.inspect.custom). Inspecting the inspection 😄

I think documentation needs to get better on this one 😄

PRs are definitely very welcome!

Also, I'd like to figure out what that require('util').inspect.custom really returns. I can only see in the console for it saying Symbol(util.inspect.custom)

I am not sure if this is helpful or not, but: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Symbol (i.e. there isn’t really much more information contained in the value of util.inspect.custom than what Symbol(util.inspect.custom) says)

Release team were -1 on landing on v6.x, if you disagree let us know.

@gibfahn on landing the deprecation, the new symbol, or either?

@gibfahn on landing the deprecation, the new symbol, or either?

On landing the deprecation, though it wasn't a heavy -1, as with all backports we'd like to hear others' thoughts (we have a week to decide).

Which is the new symbol? I don't see it in this PR.

ah, you're right, it was added in #8174 (and backported to 4.x in #9688) and this is just the deprecation.

nodejs-github-bot added the doc Issues and PRs related to the documentations. label Sep 26, 2017

vsemozhetbyt reviewed Sep 26, 2017

View reviewed changes

vsemozhetbyt approved these changes Sep 26, 2017

View reviewed changes

vsemozhetbyt added the util Issues and PRs related to the built-in util module. label Sep 26, 2017

addaleax approved these changes Sep 26, 2017

View reviewed changes

Trott added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 26, 2017

hiroppy approved these changes Sep 27, 2017

View reviewed changes

BridgeAR approved these changes Sep 27, 2017

View reviewed changes

refack approved these changes Sep 27, 2017

View reviewed changes

Trott added the semver-minor PRs that contain new features and should be released in the next minor version. label Sep 27, 2017

lpinca approved these changes Sep 27, 2017

View reviewed changes

cjihrig approved these changes Sep 27, 2017

View reviewed changes

jasnell approved these changes Sep 28, 2017

View reviewed changes

Trott force-pushed the address-15549 branch from 4c7ecb6 to 37b9faa Compare September 29, 2017 02:51

addaleax reviewed Sep 29, 2017

View reviewed changes

util: deprecate obj.inspect for custom inspection

7b5864e

The existence of `obj.inspect()` for custom inspection can cause people to unintentionally break `console.log()` and friends. This is a documentation-only deprecation that can hopefully land in 8.x. Refs: nodejs#15549

Trott force-pushed the address-15549 branch from 37b9faa to 5dd64ef Compare September 29, 2017 23:31

squash: suggestion from addaleax

1a791e8

Trott force-pushed the address-15549 branch from 5dd64ef to 1a791e8 Compare September 29, 2017 23:32

refack approved these changes Sep 30, 2017

View reviewed changes

BridgeAR reviewed Sep 30, 2017

View reviewed changes

BridgeAR closed this Oct 2, 2017

MylesBorins added backport-requested-v8.x and removed backport-requested-v8.x labels Oct 3, 2017

MylesBorins mentioned this pull request Oct 3, 2017

v8.7.0 proposal #15762

Merged

refack mentioned this pull request Oct 17, 2017

util: allow to disable only custom inspect "named" method #16266

Closed

4 tasks

gibfahn added the dont-land-on-v6.x label Jan 15, 2018

tniessen added the deprecations Issues and PRs related to deprecations. label Sep 7, 2018

Trott deleted the address-15549 branch January 13, 2022 22:46

               function for [`util.inspect()`][] is deprecated. Use [`util.inspect.custom`][]
-              instead.
+              instead. (For backwards compatibility with Node.js prior to version 6.4.0, both
+              may be specified.)

util: deprecate obj.inspect for custom inspection by Trott · Pull Request #15631 · nodejs/node · GitHub

Uh oh!

Uh oh!

util: deprecate obj.inspect for custom inspection - #15631

util: deprecate obj.inspect for custom inspection#15631Trott wants to merge 2 commits intonodejs:masternodejs/node:masterfrom Trott:address-15549Trott/io.js:address-15549Copy head branch name to clipboard

Conversation

Trott commented Sep 26, 2017

Checklist

Affected core subsystem(s)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

addaleax commented Sep 26, 2017

Uh oh!

Trott commented Sep 27, 2017

Uh oh!

evanlucas commented Sep 27, 2017

Uh oh!

Trott commented Sep 28, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Sep 29, 2017

Uh oh!

jasnell commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

refack commented Sep 29, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BridgeAR commented Oct 2, 2017

Uh oh!

MylesBorins commented Oct 3, 2017

Uh oh!

MylesBorins commented Oct 3, 2017

Uh oh!

vitaly-t commented Oct 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 17, 2017

Uh oh!

vitaly-t commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ljharb commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vitaly-t commented Oct 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

addaleax commented Oct 18, 2017

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

ljharb commented Jan 15, 2018

Uh oh!

gibfahn commented Jan 15, 2018

Uh oh!

ljharb commented Jan 15, 2018

Uh oh!

Reviewers

Assignees

util: deprecate obj.inspect for custom inspection#15631
Trott wants to merge 2 commits into
nodejs:masterfrom
Trott:address-15549

refack commented Sep 28, 2017 •
edited
Loading

vitaly-t commented Oct 17, 2017 •
edited
Loading

vitaly-t commented Oct 18, 2017 •
edited
Loading

ljharb commented Oct 18, 2017 •
edited
Loading

vitaly-t commented Oct 18, 2017 •
edited
Loading