GV

GeezerVison

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

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Revert "util: change util.inspect depth default" by devsnek · Pull Request #24326 · nodejs/node

Conversation

@devsnek

@devsnek devsnek commented Nov 12, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member

This reverts commit ac7450a.

It has caused breakage in logging to existing apps, and even
something as simple as console.log(require) will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying while debugging.

cc @nodejs/util

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

Sorry, something went wrong.

Uh oh!

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

Lewdcario, iCrawl, bdistin, xiaobotdev, appellation, Darqam, kyranet, DevYukine, and bricss reacted with hooray emoji
All reactions
  • 🎉 9 reactions
@devsnek
devsnek requested a review from BridgeAR November 12, 2018 21:29
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot nodejs-github-bot added repl Issues and PRs related to the REPL subsystem. util Issues and PRs related to the built-in util module. labels Nov 12, 2018
@devsnek
Revert "util: change util.inspect depth default"
bde132b
This reverts commit ac7450a.

This fully reverts the changes to util.inspect depth.

It has caused breakage in logging to existing apps, and even
something as simple as `console.log(require)` will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying.
@devsnek
devsnek force-pushed the revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch from 4ef10a3 to bde132b Compare November 12, 2018 21:53
@devsnek devsnek changed the title revert c7450a, 1a1fe53 Revert "util: change util.inspect depth default" Nov 12, 2018
@devsnek

devsnek commented Nov 12, 2018

Copy link
Copy Markdown
Member Author

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

All reactions

Sorry, something went wrong.

Uh oh!

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

addaleax
addaleax approved these changes Nov 12, 2018
View reviewed changes
cjihrig
cjihrig approved these changes Nov 12, 2018
View reviewed changes
@devsnek devsnek added the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 12, 2018
fhinkel
fhinkel approved these changes Nov 12, 2018
View reviewed changes
targos
targos approved these changes Nov 13, 2018
View reviewed changes
@BridgeAR

BridgeAR commented Nov 13, 2018

Copy link
Copy Markdown
Member

I was not aware that #23350 never landed. Shall we go for that instead or shall we revert everything fully first? I am fine either way as I intend to do a breadth wide inspection instead of the current inspection which allows to check for the actual lines being printed and therefore have a reasonable way to limit the output. The implementation is just not that trivial.

All reactions

Sorry, something went wrong.

Uh oh!

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

@devsnek

devsnek commented Nov 13, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR I'd prefer it go all the way back to 2, and then we can work our way up if that is deemed the best direction.

All reactions

Sorry, something went wrong.

Uh oh!

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

BridgeAR
BridgeAR approved these changes Nov 14, 2018
View reviewed changes
jdalton
jdalton approved these changes Nov 14, 2018
View reviewed changes
@Trott

Trott commented Nov 15, 2018

Copy link
Copy Markdown
Member

Landed in 7082c61

All reactions

Sorry, something went wrong.

Uh oh!

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

@Trott Trott closed this Nov 15, 2018
Trott pushed a commit to Trott/io.js that referenced this pull request Nov 15, 2018
@devsnek @Trott
Revert "util: change util.inspect depth default"
7082c61
This reverts commit ac7450a.

This fully reverts the changes to util.inspect depth.

It has caused breakage in logging to existing apps, and even
something as simple as `console.log(require)` will cause >1m freezes.

I've heard nothing but negative feedback (seriously not a single
person has expressed anything positive about this change) and
personally i find this change extremely annoying.

PR-URL: nodejs#24326
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Franziska Hinkelmann <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: John-David Dalton <[email protected]>
@devsnek
devsnek deleted the revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch November 15, 2018 04:16
@devsnek devsnek removed the author ready PRs with CI started, the required approvals, and no outstanding review comments. label Nov 15, 2018
@BridgeAR BridgeAR mentioned this pull request Nov 15, 2018
Release proposal: v11.2 #24350
Merged
@BridgeAR BridgeAR added the notable-change PRs with changes that should be highlighted in changelogs. label Nov 15, 2018
@BridgeAR

BridgeAR commented Nov 15, 2018

Copy link
Copy Markdown
Member

It would have been good to include the other commit from the original PR as well. Otherwise the default depth for the %o and %O formatter falls back to 2 instead of 4 as before.

jdalton reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@devsnek

devsnek commented Nov 15, 2018 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

@BridgeAR I left that commit out on purpose, as it wasn't breaking anything and seems reasonable to not override anything.

All reactions

Sorry, something went wrong.

Uh oh!

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

@devsnek

devsnek commented Nov 15, 2018

Copy link
Copy Markdown
Member Author

@BridgeAR then revert it i guess? regardless of that, this revert was approved and merged and should be in a release

All reactions

Sorry, something went wrong.

Uh oh!

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

@BridgeAR

BridgeAR commented Nov 15, 2018

Copy link
Copy Markdown
Member

It was my intention to release it but it broke tests without the other commit backported as well. That's why I pulled it out again as I did not want to block the release. So there is no disagreement here.

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos targos added dont-land-on-v6.x labels Nov 18, 2018
@targos

targos commented Nov 18, 2018

Copy link
Copy Markdown
Member

What's the next step here? Open a backport PR for v11.x that includes a revert of the other commit?

All reactions

Sorry, something went wrong.

Uh oh!

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

@targos targos added the backport-requested-v11.x label Nov 18, 2018
@devsnek

devsnek commented Nov 18, 2018

Copy link
Copy Markdown
Member Author

i'm not really sure. apparently some tests are failing on 11.x that aren't failing on master 🤷‍♂️

All reactions

Sorry, something went wrong.

Uh oh!

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

@richardlau richardlau mentioned this pull request Dec 5, 2018
doc: mention util depth default change #24805
Closed
4 tasks done
@BridgeAR BridgeAR added backported-to-v11.x and removed backport-requested-v11.x labels Dec 5, 2018
BridgeAR added a commit that referenced this pull request Dec 5, 2018
@BridgeAR
2018-12-06, Version 11.4.0 (Current), @BridgeAR
32ee780
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655
@BridgeAR BridgeAR mentioned this pull request Dec 5, 2018
v11.4.0 proposal #24854
Merged
4 tasks done
BridgeAR added a commit that referenced this pull request Dec 5, 2018
@BridgeAR
2018-12-06, Version 11.4.0 (Current), @BridgeAR
b0273f1
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655
BridgeAR added a commit that referenced this pull request Dec 6, 2018
@BridgeAR
2018-12-06, Version 11.4.0 (Current), @BridgeAR
2e6e799
Notable Changes:

* util:
  The inspection `depth` default is now back at 2.
  #24326
* Added new collaborator:
  [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
  #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 6, 2018
@BridgeAR
2018-12-06, Version 11.4.0 (Current)
758cb07
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
4f1297f
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
BridgeAR added a commit that referenced this pull request Dec 7, 2018
@BridgeAR
2018-12-07, Version 11.4.0 (Current)
bcef949
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    #23708
  * The inspection `depth` default is now back at 2.
    #24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    #23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    #24739
* readline:
  * The `readline` module now supports async iterators.
    #23916
* repl:
  * The multiline history feature is removed.
    #24804
* tls:
  * Added min/max protocol version options.
    #24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. #24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    #23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    #24677
  * The install-tools scripts or now included in the dist.
    #24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    #24655

PR-URL: #24854
This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#261
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#262
Closed
@kapouer

kapouer commented Dec 7, 2018

Copy link
Copy Markdown
Contributor

Did anyone actually checked the claim that util.inspect(require, {depth: 20}) took one minute ?

nmrugg reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

This was referenced Dec 7, 2018
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#263
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#264
Closed
2018-12-07 Version 11.4.0 (Current) @BridgeAR beevelop/docker-android-nodejs#265
Closed
@yous yous mentioned this pull request Jan 7, 2019
Translate Node v11.4.0, v11.5.0, v11.6.0 nodejs/nodejs-ko#791
Merged
refack pushed a commit to refack/node that referenced this pull request Jan 14, 2019
@BridgeAR @refack
2018-12-07, Version 11.4.0 (Current)
5912f6d
Notable Changes:

* console,util:
  * `console` functions now handle symbols as defined in the spec.
    nodejs#23708
  * The inspection `depth` default is now back at 2.
    nodejs#24326
* dgram,net:
  * Added ipv6Only option for `net` and `dgram`.
    nodejs#23798
* http:
  * Chosing between the http parser is now possible per runtime flag.
    nodejs#24739
* readline:
  * The `readline` module now supports async iterators.
    nodejs#23916
* repl:
  * The multiline history feature is removed.
    nodejs#24804
* tls:
  * Added min/max protocol version options.
    nodejs#24405
  * The X.509 public key info now includes the RSA bit size and the
    elliptic curve. nodejs#24358
* url:
  * `pathToFileURL()` now supports LF, CR and TAB.
    nodejs#23720
* Windows:
  * Tools are not installed using Boxstarter anymore.
    nodejs#24677
  * The install-tools scripts or now included in the dist.
    nodejs#24233
* Added new collaborator:
  * [antsmartian](https://github.com/antsmartian) - Anto Aravinth.
    nodejs#24655

PR-URL: nodejs#24854
@inoway46 inoway46 mentioned this pull request Aug 16, 2026
test: format errors with configurable (or hardcoded infinite) depth #65266
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@addaleax addaleax addaleax approved these changes
@cjihrig cjihrig cjihrig approved these changes
@BridgeAR BridgeAR BridgeAR approved these changes
+3 more reviewers
@jdalton jdalton jdalton approved these changes
@fhinkel fhinkel fhinkel approved these changes
@targos 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

notable-change PRs with changes that should be highlighted in changelogs. repl Issues and PRs related to the REPL subsystem. 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.

10 participants

@devsnek @nodejs-github-bot @BridgeAR @Trott @targos @kapouer @jdalton @fhinkel @addaleax @cjihrig
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.