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 "util: change util.inspect depth default" #24326

Closed
wants to merge 1 commit into
base: master
from

Conversation

@devsnek
Copy link
Member

devsnek commented Nov 12, 2018

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

@devsnek devsnek requested a review from BridgeAR Nov 12, 2018

Revert "util: change util.inspect depth default"
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 devsnek:revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch from 4ef10a3 to bde132b Nov 12, 2018

@devsnek devsnek changed the title revert c7450a, 1a1fe53 Revert "util: change util.inspect depth default" Nov 12, 2018

@devsnek

This comment has been minimized.

@targos

targos approved these changes Nov 13, 2018

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Nov 13, 2018

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.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 13, 2018

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

@Trott

This comment has been minimized.

Copy link
Member

Trott commented Nov 15, 2018

Landed in 7082c61

@Trott Trott closed this Nov 15, 2018

Trott added a commit to Trott/io.js that referenced this pull request Nov 15, 2018

Revert "util: change util.inspect depth default"
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 <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

@devsnek devsnek deleted the devsnek:revert/ac7450a09a4c167cd43c14d7c88721d22f077529 branch Nov 15, 2018

@devsnek devsnek removed the author ready label Nov 15, 2018

BridgeAR added a commit that referenced this pull request Nov 15, 2018

Revert "util: change util.inspect depth default"
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: #24326
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Franziska Hinkelmann <franziska.hinkelmann@gmail.com>
Reviewed-By: Michaël Zasso <targos@protonmail.com>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: John-David Dalton <john.david.dalton@gmail.com>

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    [#24059](#24059)
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    [#24326](#24326)
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected.
    [#24322](#24322)
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    [#23822](#23822)
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    [#23987](#23987),
    [#24348](#24348)
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      [#24034](https.//github.com//pull/24034)
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    [#24300](#24300)
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    [#24170](#24170)
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    [#24136](#24136)

BridgeAR added a commit that referenced this pull request Nov 15, 2018

2018-11-15, Version 11.2.0 (Current)
Notable changes:

* deps:
  * A new experimental HTTP parser (`llhttp`) is now supported.
    #24059
* timers:
  * Fixed an issue that could cause setTimeout to stop working as
    expected. #24322
* util:
  * The `util.inspect` `depth` option default is reset to 2.
    #24326
* Windows
  * A crashing process will now show the names of stack frames if the
    node.pdb file is available.
    #23822
  * Continued effort to improve the installer's new stage that installs
    native build tools.
    #23987,
    #24348
  * child_process:
    * On Windows the `windowsHide` option default was restored to
      `false`. This means `detached` child processes and GUI apps will
      once again start in a new window.
      #24034
* Added new collaborators:
  * [oyyd](https://github.com/oyyd) - Ouyang Yadong.
    #24300
  * [psmarshall](https://github.com/psmarshall) - Peter Marshall.
    #24170
  * [shisama](https://github.com/shisama) - Masashi Hirano.
    #24136

@BridgeAR BridgeAR referenced this pull request Nov 15, 2018

Merged

Release proposal: v11.2 #24350

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Nov 15, 2018

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.

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 15, 2018

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

@BridgeAR

This comment has been minimized.

Copy link
Member

BridgeAR commented Nov 15, 2018

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.

@targos

This comment has been minimized.

Copy link
Member

targos commented Nov 18, 2018

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

@devsnek

This comment has been minimized.

Copy link
Member

devsnek commented Nov 18, 2018

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

@targos targos added this to Backport requested in v11.x Nov 18, 2018

@richardlau richardlau referenced this pull request Dec 5, 2018

Closed

doc: mention util depth default change #24805

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 5, 2018

2018-12-06, Version 11.4.0 (Current), @BridgeAR
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 5, 2018

2018-12-06, Version 11.4.0 (Current), @BridgeAR
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 referenced this pull request Dec 5, 2018

Merged

v11.4.0 proposal #24854

4 of 4 tasks complete

BridgeAR added a commit that referenced this pull request Dec 5, 2018

2018-12-06, Version 11.4.0 (Current), @BridgeAR
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

2018-12-06, Version 11.4.0 (Current), @BridgeAR
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

2018-12-06, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

2018-12-07, Version 11.4.0 (Current)
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

@targos targos moved this from Backport requested to Backported in v11.x Dec 7, 2018

@kapouer

This comment has been minimized.

Copy link
Contributor

kapouer commented Dec 7, 2018

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

refack added a commit to refack/node that referenced this pull request Jan 14, 2019

2018-12-07, Version 11.4.0 (Current)
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment