console: implement minimal `console.group()` by Trott · Pull Request #14910 · nodejs/node · GitHub

Node.js exposes console.group() and console.groupEnd() via the
inspector. These functions have no apparent effect when called from
Node.js without the inspector. We cannot easily hide them when Node.js
is started without the inspector because we support opening the
inspector during runtime via inspector.port().

Implement a minimal console.group()/console.groupEnd(). More
sophisticated implementations are possible, but they can be done in
userland and/or features can be added to this at a later time. (It lacks
the label argument to console.group() right now, for example. How to
handle label, or even whether to handle it, may become
a bikeshed discussion. Landing a minimal implementation first avoids the
pitfall of that discussion or a similar discussion delaying the
implementation indefinitely.)

Refs: #12675
Fixes: #1716

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)

console

Thanks for working on this. I noticed that browsers also have groupCollapsed() which has the property of hiding everything entered after it unless the user manually expands the group. Due to Console's nature, I say we just make it an alias of group().

The state should be per-Console.

Fixed, thanks.

This browsers this also prints whatever's passed to group:

IMO it should be something like:

Console.prototype.group = function group(...args) {
  this.log(...args);
  this.groupIndent++;
};

(I used log() as that's supposed to be the same level of severity as group() according to https://console.spec.whatwg.org/#loglevel-severity.)

An argument passed to console.group() gets used as a label. It's not obvious to me what the best way to distinguish labels from the rest of the output is. However, omitting it entirely is probably not the way to go, so I'll add something like this.

OK, done. (I still need to document it.)

Documented.

Two spaces instead of a tab? Would look more like util.indent output.

That's how I did it at first but figured a tab would allow people to configure the indentation without us having to add configuration in our code. You raise a good point. Consistency is good, I'll change it to two spaces.

Changed, thanks.

Missing common.restoreStderr()?

Oops, yes, thanks, will fix that.

Fixed, thanks.

I'm not overly in love with this approach, especially given that it's trivial to break the visual formatting using new lines. e.g. console.group(); console.log('hello\nworld')' console.groupEnd(). While I'd be quite happy to have an implementation on this, if we do something and get it wrong it would be a semver-major to fix it. Not sure what else to suggest tho

I'm not overly in love with this approach, especially given that it's trivial to break the visual formatting using new lines

I thought about that but figured that could be a patch-level fix or a minor-level feature enhancement at a later time if it was really necessary. I'm not sure how much this feature is going to get used, to be honest.

And I suspect starting with a full-blown implementation will be riskier than starting with a minimal implementation.

I'm OK with deciding that we don't actually need to implement console.group() and let it be a no-op and people can implement it in the ecosystem if they want it. I'd want there to be consensus, though, that we should close #12675 and #1716 (perhaps after documenting that it's a no-op).

.groupCollapsed() added as an alias for .group(). I think this is ready for another look.

Code changes look fine, I'm not going to object to this landing. Would like further @nodejs/ctc input tho.

LGTM

I'm not in the CTC but this LGTM.

Nit: kGroupIndent for consistency with the kCounts above.

Ideally this should probably be "grouped" with kCounts above.

Node.js exposes `console.group()` and `console.groupEnd()` via the inspector. These functions have no apparent effect when called from Node.js without the inspector. We cannot easily hide them when Node.js is started without the inspector because we support opening the inspector during runtime via `inspector.port()`. Implement a minimal `console.group()`/`console.groupEnd()`. More sophisticated implementations are possible, but they can be done in userland and/or features can be added to this at a later time. (It lacks the `label` argument to `console.group()` right now, for example. How to handle `label`, or even whether to handle it, may become a bikeshed discussion. Landing a minimal implementation first avoids the pitfall of that discussion or a similar discussion delaying the implementation indefinitely.) Refs: nodejs#12675 Fixes: nodejs#1716

Rebased, addressed @TimothyGu's comments, force-pushed.

This could use some more @nodejs/ctc review. Seems like the right thing to do to me, but I want to make sure we have consensus on that. Otherwise, I'll go the document-that-it-doesn't-do-anything route.

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

Landed in c40229a

Node.js exposes `console.group()` and `console.groupEnd()` via the inspector. These functions have no apparent effect when called from Node.js without the inspector. We cannot easily hide them when Node.js is started without the inspector because we support opening the inspector during runtime via `inspector.port()`. Implement a minimal `console.group()`/`console.groupEnd()`. More sophisticated implementations are possible, but they can be done in userland and/or features can be added to this at a later time. `console.groupCollapsed()` is implemented as an alias for `console.group()`. PR-URL: nodejs#14910 Fixes: nodejs#1716 Ref: nodejs#12675 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

@BridgeAR I think @jasnell's point is that in this implementation, this code:

console.log('not indented');
console.group();
console.log('indented\nshould also be indented');

...results in this output:

not indented
  indented
should also be indented

...whereas the expected output should be:

not indented
  indented
  should also be indented

That said, that shortcoming was a choice. I'll create a known_issues test for the problem and also the one you pointed out too.

For comparison the current output in Node.js 8.4.0:

not indented
indented
should also be indented

@Trott I think it is good to add the functionality itself and I think we could just open a new PR to fix the indentation before the next release. WDYT?

I'll add dont-land labels that can be removed once your fixes/improvements land.

(And again, if anyone thinks this ought to be reverted instead, I'm OK with that too.)

@Trott right, I forgot that util.format does not escape the strings when not passed to util.inspect.

#14999 for the multiline fix. (Thanks, @BridgeAR, for the "here's how you should do it" info. :-D )

Node.js exposes `console.group()` and `console.groupEnd()` via the inspector. These functions have no apparent effect when called from Node.js without the inspector. We cannot easily hide them when Node.js is started without the inspector because we support opening the inspector during runtime via `inspector.port()`. Implement a minimal `console.group()`/`console.groupEnd()`. More sophisticated implementations are possible, but they can be done in userland and/or features can be added to this at a later time. `console.groupCollapsed()` is implemented as an alias for `console.group()`. PR-URL: nodejs/node#14910 Fixes: nodejs/node#1716 Ref: nodejs/node#12675 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Is this something we want to land on v8.x?

Is this something we want to land on v8.x?

@MylesBorins Yes. I've removed the dont-land labels. Sorry about not doing that sooner.

Node.js exposes `console.group()` and `console.groupEnd()` via the inspector. These functions have no apparent effect when called from Node.js without the inspector. We cannot easily hide them when Node.js is started without the inspector because we support opening the inspector during runtime via `inspector.port()`. Implement a minimal `console.group()`/`console.groupEnd()`. More sophisticated implementations are possible, but they can be done in userland and/or features can be added to this at a later time. `console.groupCollapsed()` is implemented as an alias for `console.group()`. PR-URL: #14910 Fixes: #1716 Ref: #12675 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

Notable Changes * build: * Snapshots are now re-enabled in V8 #14875 * console: * Implement minimal `console.group()`. #14910 * deps: * upgrade libuv to 1.14.1 #14866 * update nghttp2 to v1.25.0 #14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. #14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. #15034 * inspector: * Enable async stack traces #13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` #14369 * napi: * implement promise #14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. #14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. #14680 * tls: * multiple PFX in createSecureContext [#14793](#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: #15308

Notable Changes * build: * Snapshots are now re-enabled in V8 nodejs#14875 * console: * Implement minimal `console.group()`. nodejs#14910 * deps: * upgrade libuv to 1.14.1 nodejs#14866 * update nghttp2 to v1.25.0 nodejs#14955 * dns: * Add `verbatim` option to dns.lookup(). When true, results from the DNS resolver are passed on as-is, without the reshuffling that Node.js otherwise does that puts IPv4 addresses before IPv6 addresses. nodejs#14731 * fs: * add fs.copyFile and fs.copyFileSync which allows for more efficient copying of files. nodejs#15034 * inspector: * Enable async stack traces nodejs#13870 * module: * Add support for ESM. This is currently behind the `--experimental-modules` flag and requires the .mjs extension. `node --experimental-modules index.mjs` nodejs#14369 * napi: * implement promise nodejs#14365 * os: * Add support for CIDR notation to the output of the networkInterfaces() method. nodejs#14307 * perf_hooks: * An initial implementation of the Performance Timing API for Node.js. This is the same Performance Timing API implemented by modern browsers with a number of Node.js specific properties. The User Timing mark() and measure() APIs are implemented, as is a Node.js specific flavor of the Frame Timing for measuring event loop duration. nodejs#14680 * tls: * multiple PFX in createSecureContext [nodejs#14793](nodejs#14793) * Added new collaborators: * BridgeAR – Ruben Bridgewater PR-URL: nodejs#15308

Trott added the console Issues and PRs related to the console subsystem. label Aug 18, 2017

Trott force-pushed the console-group branch from ae9d954 to b905408 Compare August 18, 2017 05:57

This was referenced Aug 18, 2017

console: console exposing non-implemented methods #12675

Closed

Implement console.group #1716

Closed

TimothyGu reviewed Aug 18, 2017

View reviewed changes

lpinca reviewed Aug 18, 2017

View reviewed changes

Trott force-pushed the console-group branch 3 times, most recently from 519a6d1 to 079319c Compare August 21, 2017 20:04

jasnell approved these changes Aug 21, 2017

View reviewed changes

jasnell requested a review from a team August 21, 2017 22:06

mcollina approved these changes Aug 21, 2017

View reviewed changes

TimothyGu approved these changes Aug 22, 2017

View reviewed changes

Trott added 4 commits August 22, 2017 10:30

console: add label feature for group()

e5bbcd2

console: add groupCollapsed()

c1d5bcc

squash: groupIndent -> kgroupIndent

667be1a

Trott force-pushed the console-group branch from 134f9c6 to 667be1a Compare August 22, 2017 17:57

Trott added semver-minor PRs that contain new features and should be released in the next minor version. notable-change PRs with changes that should be highlighted in changelogs. labels Aug 22, 2017

addaleax approved these changes Aug 22, 2017

View reviewed changes

jasnell added the ctc-review label Aug 23, 2017

Trott closed this Aug 24, 2017

Trott added dont-land-on-v4.x and removed ctc-review labels Aug 24, 2017

Trott removed dont-land-on-v8.x labels Sep 10, 2017

MylesBorins mentioned this pull request Sep 10, 2017

v8.5.0 proposal #15308

Merged

MylesBorins mentioned this pull request Jan 15, 2018

Semver Minor Tracking nodejs/Release#298

Closed

gibfahn added the lts-watch-v6.x label Jan 15, 2018

Trott deleted the console-group branch January 13, 2022 22:46

console: implement minimal `console.group()` by Trott · Pull Request #14910 · nodejs/node · GitHub

Uh oh!

Uh oh!

console: implement minimal console.group() - #14910

console: implement minimal console.group()#14910Trott wants to merge 4 commits intonodejs:masternodejs/node:masterfrom Trott:console-groupTrott/io.js:console-groupCopy head branch name to clipboard

Conversation

Trott commented Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

TimothyGu 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!

TimothyGu Aug 18, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

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!

jasnell commented Aug 18, 2017

Uh oh!

Trott commented Aug 18, 2017

Uh oh!

Trott commented Aug 21, 2017

Uh oh!

jasnell left a comment

Choose a reason for hiding this comment

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu 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!

Trott commented Aug 22, 2017

Uh oh!

Trott commented Aug 22, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

BridgeAR commented Aug 24, 2017

Uh oh!

Trott commented Aug 24, 2017

Uh oh!

MylesBorins commented Sep 10, 2017

Uh oh!

Trott commented Sep 10, 2017

Uh oh!

Reviewers

Assignees

Labels

console: implement minimal console.group()#14910
Trott wants to merge 4 commits into
nodejs:masterfrom
Trott:console-group

Trott commented Aug 18, 2017 •
edited
Loading

TimothyGu Aug 18, 2017 •
edited
Loading