GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — util: change inspect depth default by BridgeAR · Pull Request #17907 · nodejs/node

Conversation

BridgeAR commented Dec 28, 2017

Copy link

Copy Markdown

Member

Right now the defaults for util.inspect are often not ideal. This is a first step to improve those. I tried not to break to much and keep the current behavior for the repl and console.

Refs: #12693

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

Affected core subsystem(s)

util, console

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

nodejs-github-bot added console

Issues and PRs related to the console subsystem.

repl

Issues and PRs related to the REPL subsystem.

util Issues and PRs related to the built-in util module. labels

Dec 28, 2017

BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label

Dec 28, 2017

lpinca reviewed

Dec 29, 2017

View reviewed changes

Comment thread

lib/util.js Outdated

lpinca Dec 29, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason for not using _extend?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR Dec 29, 2017

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The type check has to be done again in that case.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

lpinca Dec 29, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I doubt it will add a noticeable overhead but yes it makes sense.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

TimothyGu Dec 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually care about the performance of inspection? I thought we didn’t.
Same concern here as @lpinca. This is quite literally reinventing the wheel, and type checks for object are quite fast in modern V8.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR Jan 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated the use _extend as it will still be faster than Object.assign

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR Jan 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

About the performance - just recently I heard that inspect came up in a flamegraph. Seems like people use it a lot. I am aware that setting the default options will not be a big thing but the first implementation (not changing inspect for console.*) used it a lot and that is why I decided to optimize it as well.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

silverwind commented Dec 29, 2017

Copy link

Copy Markdown

Contributor

Not sure I like the complexity introduced here. Given that this is already flagged as semver-major, why not update it to 5 (I'd leave that value open to discussion, but it's certainly more useful than the current 2) everywhere via new defaults?

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR commented Dec 29, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member Author

@silverwind I do not see a big difference between 5 and unlimited. Most entries are not that deeply nested and if they are, you likely want to see everything. To prevent huge diffs, the max array entries option is way more efficient as this would otherwise be more difficult to handle.

About the complexity: I am fine with showing everything as a new default for console.* and this would remove the complexity again. But it would be a more significant change and I tried to minimize the breaking change.

For the repl it might be best to stay with the current default though. The reason is that some modules like fs have lots of nested elements and it would be be verbose otherwise.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell approved these changes

Dec 29, 2017

View reviewed changes

BridgeAR commented Dec 29, 2017

Copy link

Copy Markdown

Member Author

@jasnell what do you think about removing the limit for console.* as well? I guess most people would actually profit from that.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

jasnell commented Dec 29, 2017

Copy link

Copy Markdown

Member

Changing the default for console.* should be ok given that this is semver-major

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR added 3 commits

December 30, 2017 02:29


          streams: add custom inspect to BufferList

94b7b01

Currently inspecting the BufferList can result a maximum call stack
size error. Adding a individual inspect function prevents this.


          util: change util.inspect depth default

2d845cf

The current default is not ideal in most use cases. Therefore it is
changed to showing unlimited depth in case util.inspect is called
directly. The default is kept as before for console.log and similar.

Using console.dir will now show a depth of up to five and
console.assert / console.trace will show a unlimited depth.


          util: improve setting default options

Object.assign is currently very slow. Using Object.keys is much
faster in v8 6.3.

BridgeAR force-pushed the change-inspect-defaults branch from 2bb38d6 to 2721767 Compare

December 30, 2017 01:36

BridgeAR commented Dec 30, 2017

Copy link

Copy Markdown

Member Author

I changed the code to default to unlimited for the console functions as well.
To make that work I had to add a custom inspect function to the BufferList.

CITGM https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1179/
CI https://ci.nodejs.org/job/node-test-pull-request/12350/

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

TimothyGu reviewed

Dec 30, 2017

View reviewed changes

Comment thread

lib/internal/streams/BufferList.js Outdated

		}

		[customInspectSymbol]() {
		return `${this.constructor.name} { length: ${this.length} }`;

TimothyGu Dec 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could return { __proto__: Object.getPrototypeOf(this), length: this.length } which would also give coloring etc. for free.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR Jan 2, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using __proto__ will actually result in a `call stack size exceeded error. I decided to call inspect directly instead to highlight the color in case it is needed.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

lib/util.js Outdated

TimothyGu Dec 30, 2017

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually care about the performance of inspection? I thought we didn’t.
Same concern here as @lpinca. This is quite literally reinventing the wheel, and type checks for object are quite fast in modern V8.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

starkwang mentioned this pull request

Dec 30, 2017

Some jenkins jobs failed to push state. nodejs/github-bot#163

Closed


          fixup address comments

606f06c

BridgeAR commented Jan 2, 2018

Copy link

Copy Markdown

Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

BridgeAR added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed author ready PRs with CI started, the required approvals, and no outstanding review comments. labels

Jan 2, 2018

BridgeAR requested a review from a team

January 2, 2018 01:42

mcollina approved these changes

Jan 2, 2018

View reviewed changes

mcollina left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

targos suggested changes

Jan 2, 2018

View reviewed changes

targos left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please change the documentation of util.inspect (and add an entry to its history)?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

addaleax reviewed

Jan 2, 2018

View reviewed changes

Comment thread

lib/internal/streams/BufferList.js Outdated

		return ret;
		}

		[customInspectSymbol]() {

addaleax Jan 2, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’d just use util.inspect.custom here, it saves people looking at this code one round-trip to another file

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR Jan 3, 2018

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

BridgeAR added 2 commits

January 3, 2018 16:45


          fixup update documentation

3ca34ca


          util: change %o depth default

8ab8726

Since the default for depth is changed to `Infinity` it is logical
to change the %o default to the same as well.

Using %o with `util.format` will now always print the whole object.

BridgeAR commented Jan 3, 2018

Copy link

Copy Markdown

Member Author

I updated the documentation and added the history entry. I also changed the %o default of util.format to also use Infinity as default.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

targos approved these changes

Jan 7, 2018

View reviewed changes

Comment thread

doc/api/util.md Outdated

		* `depth` {number} Specifies the number visible nested Objects in an `object`.
		This is useful to minimize the inspection output for large complicated
		objects. To make it recurse indefinitely pass `null` or `Infinity`. Defaults
		to `null`.

targos Jan 7, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion: Infinity instead of null to match the changelog?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions


          fixup

e220598

targos removed the dont-land-on-v10.x label

Jun 8, 2018

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Sep 24, 2018


          util: change util.inspect depth default

b6fa43d

The current default is not ideal in most use cases. Therefore it is
changed to inspect objects to a maximum depth of 20 in case
util.inspect is called with it's defaults. The default is kept at 2
when using console.log() and similar in the repl.

PR-URL: nodejs#17907
Refs: nodejs#12693

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Sep 24, 2018


          util: change %o depth default

fdfcd24

Since the default for depth is changed to `20` it is logical
to change the %o default as well. It will now always use the
default depth.

PR-URL: nodejs#17907
Refs: nodejs#12693

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Oct 2, 2018


          util: change util.inspect depth default

ac7450a

The current default is not ideal in most use cases. Therefore it is
changed to inspect objects to a maximum depth of 20 in case
util.inspect is called with it's defaults. The default is kept at 2
when using console.log() and similar in the repl.

PR-URL: nodejs#17907
Refs: nodejs#12693

PR-URL: nodejs#22846
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>

BridgeAR added a commit to BridgeAR/node that referenced this pull request

Oct 2, 2018


          util: change %o depth default

1a1fe53

Since the default for depth is changed to `20` it is logical
to change the %o default as well. It will now always use the
default depth.

PR-URL: nodejs#17907
Refs: nodejs#12693

PR-URL: nodejs#22846
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Denys Otrishko <[email protected]>
Reviewed-By: Roman Reiss <[email protected]>

BridgeAR deleted the change-inspect-defaults branch

April 1, 2019 23:38

Reviewers

addaleax

addaleax left review comments

lpinca

lpinca left review comments

mcollina

mcollina approved these changes

jasnell

jasnell approved these changes

+2 more reviewers

TimothyGu

TimothyGu left review comments

targos

targos approved these changes

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

console

Issues and PRs related to the console subsystem.

repl

Issues and PRs related to the REPL subsystem.

semver-major

PRs that contain breaking changes and should be released in the next major version.

util

Issues and PRs related to the built-in util module.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

16 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

BridgeAR commented Dec 28, 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!

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!

silverwind commented Dec 29, 2017

Uh oh!

BridgeAR commented Dec 29, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BridgeAR commented Dec 29, 2017

Uh oh!

jasnell commented Dec 29, 2017

Uh oh!

BridgeAR commented Dec 30, 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 Jan 2, 2018

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

targos left a comment

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 Jan 3, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

16 participants

BridgeAR commented Dec 29, 2017 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.