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

Trott · 2017-09-26T21:18:13Z

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

vsemozhetbyt · 2017-09-26T21:28:45Z

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.

addaleax · 2017-09-26T21:41:24Z

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.

Trott · 2017-09-27T03:03:48Z

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

evanlucas · 2017-09-27T16:07:22Z

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

Trott · 2017-09-28T03:26:24Z

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.

jasnell

+1 as a docs-only deprecation

refack · 2017-09-28T14:05:30Z

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)

ljharb · 2017-09-29T16:10:34Z

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

jasnell · 2017-09-29T16:12:35Z

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

addaleax · 2017-09-29T20:29:09Z

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.

refack · 2017-09-29T21:35:44Z

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

BridgeAR · 2017-09-30T15:05:57Z

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


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?

BridgeAR · 2017-10-02T03:15:15Z

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>

MylesBorins · 2017-10-03T17:54:15Z

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

MylesBorins · 2017-10-03T17:57:20Z

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

vitaly-t · 2017-10-17T21:28:02Z

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?

addaleax · 2017-10-17T21:32:20Z

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

vitaly-t · 2017-10-18T05:49:56Z

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

Works:

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

Doesn't work:

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

Node.js v8.7.0

ljharb · 2017-10-18T05:54:23Z

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

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

vitaly-t · 2017-10-18T05:57:35Z

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

addaleax · 2017-10-18T07:54:10Z

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)

gibfahn · 2018-01-15T22:27:42Z

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

ljharb · 2018-01-15T22:30:58Z

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

gibfahn · 2018-01-15T23:49:54Z

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

ljharb · 2018-01-15T23:56:33Z

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 37b9faaCompare 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 5dd64efCompare September 29, 2017 23:31

squash: suggestion from addaleax
1a791e8

Trott force-pushed the address-15549 branch from 5dd64ef to 1a791e8Compare 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

Uh oh!

Uh oh!

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

Labels

Projects

Milestone

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