util: add inspect.defaultOptions by silverwind · Pull Request #8013 · nodejs/node · GitHub

Checklist

make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

util

Description of change

Adds the util.inspect.config() method to allow customization of inspect options in cases where util.inspect() is implicitly called by core like in console or util.format.

Fixes: #7566

Perhaps a defaultOptions setter/getter would be better here? Something like...

util.inspect.defaultOptions = options;

// or even...

util.inspect.defaultOptions.maxArrayLength = 1000;

Right, seems cleaner.

Hmm, I think it might be possible to make them plain properties. Would save some time having to compute the getter/setter names at least.

I'm generally +1 on having this, but prefer making it a getter/setter rather than a method.

Shouldn't this check for null?

Right, null is an issue. How about a strict object check instead?

if (Object.prototype.toString.call(options) === '[object Object]') {

In other places in core, I believe we've just been doing options === null || typeof options !== 'object'

Thanks for review. What's the general consensus on util._extend vs Object.assign? Shall we use the former in performance-critical paths, otherwise the latter?

Last I heard, we were still preferring util._extend() for performance reasons. However, I believe Object.assign() is more flexible, so it might be preferable in cases that would require multiple calls to util._extend().

Windows fail looks unrelated (Jenkins -- connection aborted)

Also, not sure what's up with the linter on CI:

+ gmake lint-ci
./node tools/jslint.js -j 1 -f tap -o test-eslint.tap \
    benchmark lib src test tools
gmake: *** [Makefile:677: jslint-ci] Error 1

Definitely passes locally.

Nevermind, there's some lint errors. Fixing them.

Updated to use the util.inspect.defaultOptions property instead of a function.

I sealed the object to prevent addition/deletion of options.
Currently, the property is writable: false to prevent shenanigans like defaultOptions = null which would break inspect. The downside of this is that we can't set using a object, just through individual properties, one at a time. Thoughts about writeable, @jasnell?

Using a getter/setter would help protect against things like defaultOptions = null, just have the setter function check to see if the input is valid and ignore it if it is not.

I'm not convinced that sealing the object is necessary but it's likely harmless.

@jasnell Sealing for example prevents delete defaultOptions.depth for of the current solution without accessors.

Supporting both defaultOptions = {} and defaultOptions.property = true through getters/setters would introduce quite a big amount of code. I'm not sure if it's really that common to set all properties at once. In my case, I'd only like to change depth and maxArrayLength.

@jasnell I've switched to accessors now. The solution turned out shorter than I thought and should satisfy all cases. I'd like to keep the properties sealed to prevent some misuse like deleting, and so I can Object.assign without checking for property existance in the setter.

CI: https://ci.nodejs.org/job/node-test-pull-request/3574/

s/options/default options

LGTM with a nit if CI is green.

Tiny nit, but could you capitalize "set" here and in the comment below.

LGTM pending CI.

CI is green except a hanging Windows box. Giving this PR a bit of time for others to weight in.

Since this is a property and not a function, it doesn't really "return" anything, right? It just is a value. I'd be inclined to delete the line entirely. I don't think it actually adds any information. (I also think it's misleading. If you don't set it, it's undefined and not an object.)

(Whoops, the parenthetical is wrong, ignore it. Also, uh, yeah, looks like I'm commenting on an out-of-date diff. SMH. Sorry.)

LGTM with Trott's nit fixed

Nit addressed. One more CI: https://ci.nodejs.org/job/node-test-pull-request/3589/

still LGTM :-)

Noticed a bug where the REPL module called to inspect using the legacy form with undefined properties:

util.inspect(obj, undefined, undefined, true)

The undefined properties were overriding the defaults in the Object.assign call. I fixed the issue by not adding undefined props to ctx and added a test for it. Also made it copy opts last, so it matches previous behaviour.

One last CI:

https://ci.nodejs.org/job/node-test-pull-request/3598/
https://ci.nodejs.org/job/node-test-pull-request/3599/
https://ci.nodejs.org/job/node-test-pull-request/3600/

oh, good catch @silverwind

Adds util.inspect.defaultOptions which allows customization of the default util.inspect options, which is useful for functions like console.log or util.format which implicitly call into util.inspect. PR-URL: nodejs#8013 Fixes: nodejs#7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Adds util.inspect.defaultOptions which allows customization of the default util.inspect options, which is useful for functions like console.log or util.format which implicitly call into util.inspect. PR-URL: #8013 Fixes: #7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

One unrelated CI failure on Windows. Landed in d9ef3e1 1a6a69a.

Adds util.inspect.defaultOptions which allows customization of the default util.inspect options, which is useful for functions like console.log or util.format which implicitly call into util.inspect. PR-URL: #8013 Fixes: #7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

There was a silly typo in the error message. I --force-with-lease pushed 1a6a69a to correct it.

Adds util.inspect.defaultOptions which allows customization of the default util.inspect options, which is useful for functions like console.log or util.format which implicitly call into util.inspect. PR-URL: #8013 Fixes: #7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) #7983 and #7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) #7811 and #7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) #7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) #7942 * repl: The REPL now supports editor mode. (Prince J Wesley) #7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) #8013 Refs: #8020 PR-URL: #8070

Notable changes: * build: zlib symbols and additional OpenSSL symbols are now exposed on Windows platforms. (Alex Hultman) nodejs#7983 and nodejs#7576 * child_process, cluster: Forked child processes and cluster workers now support stdio configuration. (Colin Ihrig) nodejs#7811 and nodejs#7838 * child_process: argv[0] can now be set to arbitrary values in spawned processes. (Pat Pannuto) nodejs#7696 * fs: fs.ReadStream now exposes the number of bytes it has read so far. (Linus Unnebäck) nodejs#7942 * repl: The REPL now supports editor mode. (Prince J Wesley) nodejs#7275 * util: inspect() can now be configured globally using util.inspect.defaultOptions. (Roman Reiss) nodejs#8013 Refs: nodejs#8020 PR-URL: nodejs#8070

silverwind added util Issues and PRs related to the built-in util module. semver-minor PRs that contain new features and should be released in the next minor version. labels Aug 8, 2016

silverwind mentioned this pull request Aug 8, 2016

Changing default options for util.inspect #7566

Closed

jasnell reviewed Aug 8, 2016
View reviewed changes

cjihrig reviewed Aug 8, 2016
View reviewed changes

silverwind force-pushed the inspect-config branch from ab34c96 to 2dcd462 Compare August 8, 2016 17:46

silverwind changed the title util: add inspect.config util: add inspect.defaultOptions Aug 8, 2016

jasnell reviewed Aug 8, 2016
View reviewed changes

cjihrig reviewed Aug 8, 2016
View reviewed changes

silverwind force-pushed the inspect-config branch from 769d558 to 01ef515 Compare August 8, 2016 18:30

Trott reviewed Aug 8, 2016
View reviewed changes

silverwind force-pushed the inspect-config branch from cc911f7 to 9942438 Compare August 9, 2016 17:51

silverwind force-pushed the inspect-config branch from 9942438 to c0dcd92 Compare August 9, 2016 18:06

silverwind force-pushed the inspect-config branch from c0dcd92 to cfbb09b Compare August 9, 2016 18:52

silverwind closed this Aug 9, 2016

cjihrig mentioned this pull request Aug 10, 2016

v6.4.0 Release Proposal #8020

Closed

cjihrig mentioned this pull request Aug 11, 2016

v6.4.0 Proposal #8070

Merged

black-snow mentioned this pull request Aug 18, 2016

Console.log crashes fs-jetpack szwacz/fs-jetpack#29

Closed

eduardomourar mentioned this pull request Oct 16, 2020

Increase default util inspect depth for objects aws-cloudformation/cloudformation-cli-typescript-plugin#27

Merged

util: add inspect.defaultOptions by silverwind · Pull Request #8013 · nodejs/node · GitHub

Uh oh!

Uh oh!

util: add inspect.defaultOptions - #8013

util: add inspect.defaultOptions#8013silverwind wants to merge 1 commit intonodejs:masternodejs/node:masterfrom silverwind:inspect-configsilverwind/node:inspect-configCopy head branch name to clipboard

Conversation

silverwind commented Aug 8, 2016

Checklist

Affected core subsystem(s)

Description of change

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!

jasnell commented Aug 8, 2016

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!

silverwind commented Aug 8, 2016

Uh oh!

cjihrig commented Aug 8, 2016

Uh oh!

jbergstroem commented Aug 8, 2016

Uh oh!

silverwind commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 8, 2016

Uh oh!

silverwind commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

silverwind commented Aug 8, 2016

Uh oh!

silverwind commented Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasnell commented Aug 8, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cjihrig commented Aug 8, 2016

Uh oh!

silverwind commented Aug 8, 2016

Uh oh!

Trott Aug 8, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

evanlucas commented Aug 9, 2016

Uh oh!

silverwind commented Aug 9, 2016

Uh oh!

jasnell commented Aug 9, 2016

Uh oh!

silverwind commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasnell commented Aug 9, 2016

Uh oh!

silverwind commented Aug 9, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silverwind commented Aug 9, 2016

Uh oh!

util: add inspect.defaultOptions#8013
silverwind wants to merge 1 commit into
nodejs:masterfrom
silverwind:inspect-config

silverwind commented Aug 8, 2016 •
edited
Loading

silverwind commented Aug 8, 2016 •
edited
Loading

silverwind commented Aug 8, 2016 •
edited
Loading

Trott Aug 8, 2016 •
edited
Loading

silverwind commented Aug 9, 2016 •
edited
Loading

silverwind commented Aug 9, 2016 •
edited
Loading