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

silverwind · 2016-08-08T14:05:22Z

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

jasnell · 2016-08-08T14:28:38Z

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.

jasnell · 2016-08-08T14:29:35Z

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

cjihrig · 2016-08-08T14:31:17Z

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'

silverwind · 2016-08-08T14:55:19Z

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?

cjihrig · 2016-08-08T14:58:35Z

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

jbergstroem · 2016-08-08T15:08:50Z

Windows fail looks unrelated (Jenkins -- connection aborted)

silverwind · 2016-08-08T15:13:21Z

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

silverwind · 2016-08-08T15:19:54Z

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

silverwind · 2016-08-08T16:59:11Z

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?

jasnell · 2016-08-08T17:06:26Z

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.

silverwind · 2016-08-08T17:20:57Z

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

silverwind · 2016-08-08T17:38:46Z

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

silverwind · 2016-08-08T17:41:26Z

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

jasnell · 2016-08-08T17:57:58Z

s/options/default options

jasnell · 2016-08-08T17:58:39Z

LGTM with a nit if CI is green.

cjihrig · 2016-08-08T18:11:37Z

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

cjihrig · 2016-08-08T18:11:50Z

LGTM pending CI.

silverwind · 2016-08-08T18:46:06Z

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

Trott · 2016-08-08T21:26:18Z

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

evanlucas · 2016-08-09T10:55:18Z

LGTM with Trott's nit fixed

silverwind · 2016-08-09T10:58:07Z

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

jasnell · 2016-08-09T14:32:00Z

still LGTM :-)

silverwind · 2016-08-09T17:49:18Z

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/

jasnell · 2016-08-09T17:54:59Z

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#8013Fixes: 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: #8013Fixes: #7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

silverwind · 2016-08-09T18:54:03Z

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: #8013Fixes: #7566 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Evan Lucas <evanlucas@me.com>

silverwind · 2016-08-09T19:00:57Z

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: #8013Fixes: #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 2dcd462Compare August 8, 2016 17:46

silverwind changed the title ~~util: add inspect.config~~util: add inspect.defaultOptionsAug 8, 2016

jasnell reviewed Aug 8, 2016
View reviewed changes

Comment threaddoc/api/util.md Outdated
jasnellAug 8, 2016
Copy link
Copy Markdown
Member
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/options/default options

cjihrig reviewed Aug 8, 2016
View reviewed changes

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

Trott reviewed Aug 8, 2016
View reviewed changes

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

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

silverwind force-pushed the inspect-config branch from c0dcd92 to cfbb09bCompare 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

Uh oh!

Uh oh!

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!

TrottAug 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!

Reviewers

Assignees

Labels

silverwind commented Aug 8, 2016•
edited
Loading

silverwind commented Aug 8, 2016•
edited
Loading

silverwind commented Aug 8, 2016•
edited
Loading

TrottAug 8, 2016•
edited
Loading

silverwind commented Aug 9, 2016•
edited
Loading

silverwind commented Aug 9, 2016•
edited
Loading