Skip to content

/node

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Revert 17907: util: change inspect depth default #20017

Closed
wants to merge 3 commits into from

Conversation

Reviewers

@Trott Trott

@TimothyGu TimothyGu

@cjihrig cjihrig

@joyeecheung joyeecheung

@BridgeAR BridgeAR

Assignees
No one assigned
Labels
dont-land-on-v10.x dont-land-on-v8.x repl util
Projects
  
Don't land (ever) in v10.x
Milestone
No milestone
10 participants
@addaleax @jasnell @Trott @mcollina @apapirovski @BridgeAR @cjihrig @TimothyGu @joyeecheung @nodejs-github-bot
@addaleax
Copy link
Member

commented Apr 13, 2018

  • Revert "util: change util.inspect depth default"

    This reverts commit b994b8e.

    This caused regressions in ecosystem code. While the change originally
    was semver-major and could be postponed until after Node.js 10,
    I think reverting it is a good choice at this point.

    Also, I personally do not think defaulting to a shallow inspect
    is a bad thing at all – quite the opposite: It makes util.inspect()
    give an overview of an object, rather than providing a full
    display of its contents. Changing the depth default to infinity
    fundamentally changed the role that util.inspect() plays,
    and makes output much more verbose and thus at times unusable
    for console.log()-style debugging.

    Fixes: #19405
    Refs: #17907

  • Revert "util: change %o depth default"

    This reverts commit 8f15309.

(#19994 also has a Fixes: tag for #19405, but it’s independent from this PR and is not uncontroversial)

  • 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

@addaleax addaleax requested review from BridgeAR and nodejs/tsc Apr 13, 2018

@nodejs-github-bot nodejs-github-bot added repl util labels Apr 13, 2018

@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 13, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14272/
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 13, 2018

While this option definitely needs to stay on the table, I'm not yet ready to agree that this is the right thing to do... for a couple of reasons:

  1. The issue with npm update illustrates a fundamental weakness of util.inspect() with overly large objects. The npm code takes an object that with a depth of 8 generates a string that is over 6.6 MB in size and over 81k lines long. Any object, even with a depth of 2, could end up with precisely the same issue. The problem is not so much the default depth but the depth-first string-and-array-munging and memory-eating algorithm that is used to generate the string. We should investigate a much more efficient algorithm.

  2. The issue in user-code can be mitigated with a semver-patch fix to explicitly set the depth to 2 as opposed to relying on the default.

  3. Sending multi-megabyte large objects to a sync function for serialization is never a great idea.

I'm perfectly fine with not including the original commit in 10.0.0 but before we revert we should look at alternatives.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 13, 2018

We should investigate a much more efficient algorithm.

Yes, and I like the suggestion of a maximum string length that @BridgeAR brought up – That’s not an easy thing to do, though.

Also, from a purely pragmatical point of view, the current situation would be that if we don’t land both #17907 and a revert on v10.x, there are going to be other semver-major changes that would be help up because of merge conflicts (like WeakMap/WeakSet support).

The issue in user-code can be mitigated with a semver-patch fix to explicitly set the depth to 2 as opposed to relying on the default.

Yea, I don’t think relying on something that has been the default for literally 8+ years qualifies as an “issue in user-code” in any way.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 13, 2018

Also: Even if we do the maximum string length approach or something like that, I still think this is the right thing to do (for the reasons given in the commit message).
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 13, 2018

As I said, I'm all for not including this in 10.0.0. I'd like to just do a bit more investigation of other options before we revert.
@Trott

Trott approved these changes Apr 13, 2018
View changes

@Trott

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 13, 2018

CI: https://ci.nodejs.org/job/node-test-pull-request/14273/
@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 14, 2018

👍 on not including the change of default in 10, but let's wait a bit more before landing this.
@apapirovski

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 14, 2018 •
edited

Instead of a revert, could we just back this out of v10 and have it be scheduled to land in v11? Or do we want each major to represent the state of master at the related point in time?
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 14, 2018

@apapirovski Yes, backing it out is the current plan.

Also, as said above, there are other factors that played into the decision to open a revert (unblocking other PRs that wouldn’t go into 10.x + the change in semantics not being a good one).
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 14, 2018

but let's wait a bit more before landing this.

@mcollina Can you give a ballpark number for the time frame you had in mind? (And, ideally, a reason would be nice as well…)
@TimothyGu

TimothyGu approved these changes Apr 14, 2018
View changes

@mcollina

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 14, 2018

After 10 goes out and we can discuss about it with less urgency? There is a lot of traffic on a lot of issues atm.
Sincerely, I’ve seen this API being misused and causing a lot of production issues, and not being really useful.
I think that printing full depth is actually a good improvement in usability, as I never founs the output particularly useful in complex cases.

I think something should be done about it. This might be the path to go, and then we do something else instead.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 14, 2018

@mcollina I’m sorry, I don’t mean to imply that landing this PR would mean an end of the discussion. I do agree that util.inspect() output could often be improved.

But, unfortunately, there is urgency in the sense that we do need this revert to land before Node 10, either on master or on v10.x-staging. We can’t just rebase out the commits, there have been over 1000 commits since and there would be far too many merge conflicts or commits that we’d have to skip (I tried it).
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 14, 2018

@addaleax ... Can you open a separate revert PR against v10.x-staging? I'll get it landed on Tuesday when I do the next round of updates on that branch.
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 14, 2018

@jasnell That also gives merge conflicts because v10.x-staging isn’t quite caught up to master yet, so if I were to open a revert “backport” PR now we’d just have to take care of those later again … let me know when you’ve updated the branch and I can open a PR then, if that’s ok with you
@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 14, 2018

@cjihrig

cjihrig approved these changes Apr 14, 2018
View changes

@joyeecheung

joyeecheung approved these changes Apr 15, 2018
View changes

@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 16, 2018

Just marking this explicit PR as blocked for now.

@BridgeAR BridgeAR added the blocked label Apr 16, 2018

@jasnell jasnell added blocked and removed blocked labels Apr 16, 2018

@jasnell

This comment has been minimized.

Sign in to view
Copy link
Member

commented Apr 16, 2018

@addaleax .... I just updated v10.x-staging and this now lands clean there. I'll open a second PR.

@jasnell jasnell referenced this pull request Apr 16, 2018

Closed

[10.x] Revert 17907: util: change inspect depth default #20089

4 of 4 tasks complete
@addaleax

This comment has been minimized.

Sign in to view
Copy link
Member Author

commented Apr 17, 2018

@BridgeAR I think this is no longer blocked after #20089, if I understood your intention with adding the label correctly?
@BridgeAR

This comment has been minimized.

Sign in to view
Copy link
Member

commented May 11, 2018

Landed in f145a53...5096e24.

I changed the test title as agreed upon with @addaleax

@BridgeAR BridgeAR closed this May 11, 2018

targos added a commit that referenced this pull request May 12, 2018

@addaleax @targos
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: #20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Verified
This commit was signed with a verified signature.
@targos targos Michaël Zasso
GPG key ID: 770F7A9A5AE15600 Learn about signing commits
de34cfa

shisama added a commit to shisama/node that referenced this pull request May 13, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
d16ab60

shisama added a commit to shisama/node that referenced this pull request May 13, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
5f2da80

shisama added a commit to shisama/node that referenced this pull request May 13, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
dabf40f

@addaleax addaleax referenced this pull request May 14, 2018

Merged

v10.2.0 proposal #20724

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
4518bf6

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
8232a38

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
549b847

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
e019979

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
43ff786

shisama added a commit to shisama/node that referenced this pull request May 30, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
f5bbbc2

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
ea21b54

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
db73739

shisama added a commit to shisama/node that referenced this pull request Jun 5, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
fb33aaa

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
e2210e8

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
43b328e

shisama added a commit to shisama/node that referenced this pull request Jun 9, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
e71f144

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
87e7fac

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
46e5145

shisama added a commit to shisama/node that referenced this pull request Jun 11, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
02c7f28

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
d33d2ff

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
c3be15e

shisama added a commit to shisama/node that referenced this pull request Jun 12, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
ec10489

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@addaleax @shisama
Revert "util: change %o depth default" 
This reverts commit 8f15309.

PR-URL: nodejs#20017
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
41f86c6

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@addaleax @shisama
Revert "util: change util.inspect depth default" 
This reverts commit b994b8e.

This caused regressions in ecosystem code. While the change originally
was semver-major and could be postponed until after Node.js 10,
I think reverting it is a good choice at this point.

Also, I personally do not think defaulting to a shallow inspect
is a bad thing at all – quite the opposite: It makes `util.inspect()`
give an overview of an object, rather than providing a full
display of its contents. Changing the `depth` default to infinity
fundamentally changed the role that `util.inspect()` plays,
and makes output much more verbose and thus at times unusable
for `console.log()`-style debugging.

PR-URL: nodejs#20017
Fixes: nodejs#19405
Refs: nodejs#17907
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
dc99bd0

shisama added a commit to shisama/node that referenced this pull request Jun 17, 2018

@addaleax @shisama
test: make sure linked lists are inspectable with defaults 
Make sure that a long singly-linked list can be passed
to `util.inspect()` without causing a stack overflow.

PR-URL: nodejs#20017
Reviewed-By: Rich Trott <rtrott@gmail.com>
Reviewed-By: Tiancheng "Timothy" Gu <timothygu99@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
04b9616

@targos targos added this to Don't land (for now) in v10.x Sep 23, 2018

@targos targos moved this from Don't land (for now) to Don't land (ever) in v10.x Sep 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.