Revert 17907: util: change inspect depth default

[addaleax]

• 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: npm: last nightly / v8-canary make npm update unusable #19405
  Refs: util: change inspect depth default #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]

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

[jasnell]

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]

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]

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]

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]

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

[mcollina]

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

[apapirovski]

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]

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

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

[mcollina]

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]

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

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

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

Ok that works On Apr 14, 2018 14:48, "Anna Henningsen" <[email protected]> wrote: @jasnell <https://github.com/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 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#20017 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAa2eVL0drGkZSZNXtwZqGPUnn13tqz8ks5tom6XgaJpZM4TUG5b> .

[BridgeAR]

Just marking this explicit PR as blocked for now.

[jasnell]

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

[addaleax]

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

[BridgeAR]

Landed in f145a53...5096e24.

I changed the test title as agreed upon with @addaleax