lib: add console.{group,groupCollapsed,groupEnd}() #1727

imyller · 2015-05-18T11:40:12Z

Implements nested console groups to match browser console API:

console.group()
console.groupCollapsed()
console.groupEnd()

Notes:

Starting a new group prefixes each line of output with two-spaces per group/indent level.
console API documentation is updated to include new functions.
Test case test-console-group.js is included.
console.groupCollapsed() is aliased to console.group() with a wrapper function so that external gui debuggers can handle it correctly. For example node-inspector.

Implements nested console groups to match browser API: * `console.group()` * `console.groupCollapsed()` * `console.groupEnd()` Note: `console.groupCollapsed()` is functionally equivalent to `console.group()` but is included for compatibility.

imyller · 2015-05-18T11:40:40Z

Referencing #1716

targos · 2015-05-18T12:27:41Z

lib/console.js

@@ -33,7 +35,8 @@ function Console(stdout, stderr) {
 }

 Console.prototype.log = function() {
-  this._stdout.write(util.format.apply(this, arguments) + '\n');
+  this._stdout.write(util.format.apply(this, arguments).replace(/^/gm,
+          Array(this._group * 2 + 1).join(' ')) + '\n');


What about ' '.repeat(this._group * 2) ?

I just wanted to keep console as lightweight and performant as possible.

String.prototype.repeat is bit slower than Array.join method I chose.

See http://jsperf.com/faster-string-repeat/15

I do agree that ' '.repeat() is more readable in code.

If you want performance, define this._group as string.
in group function do this._group += ' '
in this function do:

var data = util.format.apply(this, arguments); if (this._group.length) data = data.replace(/^/gm, this._group); this._stdout.write(data + '\n');

Please add a check that grouping is used before allocating all these arrays and strings

I also think it's better to just write the indentation, instead of concatenating it and then writing it anyway

This jsperf is using a shim for String.prototype.repeat. Native is faster
Anyway both show that Array#join is the slowest of all

Thank you for reviewing and providing input. I tried to be minimal and not introduce lot of new code and variables to core lib.

I'll update the PR soon based on your comments.

@3y3 For tracking multiple levels of grouping I'd still keep numeric _group but generate a fixed prefix string which would be conditionally concatenated as @petkaantonov suggested. This adds variables and code, but I think it pleases everyone?

3y3 · 2015-05-18T14:26:19Z

How about using extended utf characters here?

console.group('Test group level 1');
console.log(1);
console.group('Test group level 2');
console.log(2);
console.log(3);
console.log({a: 1, b: 2});
console.groupEnd();
console.log(4);
console.groupEnd();

▼ Test group level 1
│ 1
│ ▼ Test group level 2
│ │ 2
│ │ 3
│ │ {
│ │   a: 1,
│ │   b: 2
│ │ }
│ └
│ 4
└

it is easier to read

imyller · 2015-05-18T15:03:39Z

I hope to keep this PR simple and just add the base functions for console grouping with minimal output formatting.

My personal opinion is that introducing extended UTF chars and lot of additional formatting can be done if core team so accepts, but I wouldn't push it here in this PR. There is also risk that it will break something that depends on simplified console output.

Also I'd keep the new _group property as a numeric value holding the nested group-level so that those who want to override console can use it in their own implementation as-is. If _group becomes a string holding an arbitrary prefix "text" it most likely can not be used at all by some implementations.

Fishrock123 · 2015-05-18T15:13:19Z

Does this affect console.log that could be called from somewhere elsewhere while group() is on?

to match browser console API:

I'm really not sure that is what we are or want to be doing, as exists in discussion surrounding things like workers, url, etc.

silverwind · 2015-05-18T15:23:16Z

I'm also not too fond of using unicode in terminals. A lot of terminals still have trouble with that.

3y3 · 2015-05-18T15:30:01Z

I'm also not too fond of using unicode in terminals. A lot of terminals still have trouble with that.

I agree about terminals. But I also say, that space char works bad if we inspect objects, which have their own space indentation

imyller · 2015-05-18T15:31:58Z

I've now adjusted the PR according to input received. Still tried to keep modifications to console.js minimal.

@petkaantonov there is now conditional insertion of prefix if grouping is not used
@targos Prefix is now generated with String.prototype.repeat
@3y3 The prefix is no longer generated for each log output separately but is instead cached to _prefix property.

Thank you for your input.

imyller · 2015-05-18T15:37:14Z

@3y3 Your point is valid, but do understand that I tried to keep prefix generation consistent in group and groupEnd without adding some hidden _generatePrefix function that would NOT most definitely be accepted in core lib.

ChALkeR · 2015-05-18T15:46:55Z

@3y3 I would argue against unnecessary string concatenation (that you use in your code), even so minor. But that doesn't matter in this case, the performance effect of this is negligable.

3y3 · 2015-05-18T15:49:33Z

But that doesn't matter in this case, the performance effect of this is negligable.

This is not only for performance, but also for readability.

imyller · 2015-05-18T15:52:27Z

With this PR, I'm at all cost avoiding any kind of rewrite of console.js.
This is just about adding basic console grouping with minimal modifications while respecting both the structure and intent of the original code.

Sure, I'd like to rewrite the console and add all bells and whistles right away, but I'm almost sure that it would never be accepted to Node core lib from single pull-request (or even two).

So, if you want more this and that; more descriptive, but longer, code; I can surely offer such modifications, but I personally think that it will not be what lib maintainers want to bring to Stability level 2 core lib.

I'm just trying to keep this minimal :)

ChALkeR · 2015-05-18T15:54:43Z

This is not only for performance, but also for readability.

Keeping the variable/line count minimal improves the readability better.

ChALkeR · 2015-05-18T15:56:37Z

lib/console.js

+Console.prototype.groupEnd = function() {
+  if ( this._group > 0 ) {
+    this._group--;
+    this._prefix = (this._group ? '  '.repeat(this._group) : '');


Is a special case really needed here? ' '.repeat(0) === ''.

Tried to avoid extra function call and use single '' const that optimizes better than generating a new one for each groupEnd.

If you feel this complicates code too much, this conditional can be removed as you suggested.

I think this is right place for this line:

if (_group` == 0) //do nothing else //modify something.

And about:

this._prefix = (this._group ? ' '.repeat(this._group) : '');

generate some levels of indentation
or

this._prefix = this._prefix.slice(0, -2);

trim two characters from indentation level

Oh sorry I missed that where cheched

this._group ? ' '.repeat(this._group) : ''

so if rewrite it with if statement:

if (this._group) ' '.repeat(this._group) else { // here is unreachable code }

I agree with @ChALkeR

@3y3 So to summarize:

You want to get rid of the ( a ? b : c ) notation and replace with if else structures

Use += ' ' concatenation in group() to modify _prefix string

Use String.slice in groupEnd() to modify _prefix string

You have a valid approach, but here I did not choose that. Lets give the maintainers some time to comment on this and give us direction :)

Propose this as final solution for this block:

Console.prototype.groupEnd = function() { if (this._group) { this._group--; this._prefix = this._prefix.slice(0, -2); } }

You want to get rid of the ( a ? b : c ) notation and replace with if else structures

No I only say, that your ( a ? b : c ) notation has unreachable part.

Look at my example.

No I only say, that your ( a ? b : c ) notation has unreachable part.

Actually, It does not have unreachable part, but I do see an extra > 0 comparison that could be left out completely for optimization. Can you please define what is the unreachable part you see.

(this._group ? ' '.repeat(this._group) : ''); evaluates to '' empty string const only if _group has just been decreased to zero. Otherwise it just regenerates the _prefix with new string. It does not use slice, but it matches the code group uses for generating the prefix.

I'm just trying to avoid avoiding unnecessary string concatenations and multiple methods in functions to generate the prefix. I chose String.prototype.repeat instead of string concatenation and String.prototype.slice.

rvagg · 2015-05-18T17:24:26Z

I'm not sure we want to slavishly re-implement all of the browser console functionality and my inclination at this stage is to say this should be something you install from npm rather than putting in core

imyller · 2015-05-18T17:31:07Z

Thanks @rvagg

This is the kind of input and discussion the pull-request needs 👍

I'm not qualified to review if the feature is right for the core lib, but I've offered a minimalistic implementation here as a base for discussion.

silverwind · 2015-05-18T19:15:29Z

slavishly re-implement all of the browser console

As I see it, we're on the same boat as browsers when it comes to console. People will be expecting that we provide the same APIs as browsers do, and it'd further help JS along on the way to platform-independent code. Also, tools like node-inspector need us to at least provide the functions they can hook into, so the end result is the same as in Chrome DevTools.

3y3 · 2015-05-18T19:22:51Z

need us to at least provide the functions they can hook into

It's not true for node-inspector =)
Node Inspector can implement this feature by itself (we inject a lot of code in live process).
But may be it's truth for someone else.

silverwind · 2015-05-18T19:27:01Z

Node Inspector can implement this feature by itself

But I guess no one would use it if it doesn't work without the inspector attached 😉

rvagg · 2015-05-18T19:57:55Z

As I see it, we're on the same boat as browsers when it comes to console

This is not strictly correct: we have npm, browsers have no means to easily extend tooling like this so usually just bundle all the things which we explicitly don't do. Want a fancier devtools environment? Use node-inspector, replpad, whatever. Want more fanciness in your logging? npm install consolegroup (doesn't exist? make it!).

imyller · 2015-05-18T20:43:29Z

I'm just saying that JavaScript needs a Standard Library; and if there ever is going to be one, Node needs to ship with it.

console is a good example of this. There are as many takes on what console means as there are JS platforms. All vaguely similar, but not enough to write portable code with.

It just might be so that Node should not be chasing the others and this PR deserves to be buried. .. but that and the general policies about what to include and what not are up to the maintainers of Node to decide.

Fishrock123 · 2015-05-18T21:34:24Z

I'm just saying that JavaScript needs a Standard Library; and if there ever is going to be one, Node needs to ship with it.

It definitely doesn't need it, there are plenty of people who advocate a smaller core both in core and outside.

I'll pull up some articles / talks on why when I am able to, however there is a general goal in core to keep core lean and empower users d solutions; so far, it has worked very well.

imyller · 2015-05-18T21:55:52Z

All good points. Trust me, I'm one of the outside proponents of keeping the core lean.

Lean core, good package management and a solid platform agnostic Standard Library would be great combo for the JS. My main point was that we have the first two, but not the third.

Here however, core has already chosen to implement console but has since left it lacking functionality compared to other JS implementations. I think the justification for accepting this PR would be to catch up with those. On the other hand keeping core lean and rejecting "browserification" are good reasons not to accept this PR; both sides have their point. Luckily I'm not one making the decision ;)

Yet, if adding features to the console is not desired going forward, one might ask why include console (by that name) at all if the goal is not to have implementation of the console users coming from other corners of JS world are expecting.

silverwind · 2015-05-19T04:54:16Z

we have npm, browsers have no means to easily extend tooling like this

Most well-written modules that don't depend on platform-api work in both browser and node nowadays, and I think reusable code is clearly the future where modules should be going. My point in this case is console is historically something the environment provides, not the ecosystem.

yosuke-furukawa · 2015-05-19T07:53:27Z

I totally agree that we should get closer to browser API.
Our current console is different from Devtool WG standard.

my opinion and current console status is written here.
#1716 (comment)

my concern is to spread the friction between node/io.js API and browser API ...
This is not only console, we should get closer to browser API to learn easily and improve reusability.

imyller mentioned this pull request May 18, 2015

Implement console.group #1716

Closed

Improve console.markdown groupEnd() docs

f67438e

targos reviewed May 18, 2015
View reviewed changes

Fishrock123 added semver-minor PRs that contain new features and should be released in the next minor version. feature request Issues that request new features to be added to Node.js. console Issues and PRs related to the console subsystem. labels May 18, 2015

lib: improve console grouping formatting performance

9a3a885

ChALkeR reviewed May 18, 2015
View reviewed changes

lib: console: small clean ups

fd0e2f2

brendanashworth removed the feature request Issues that request new features to be added to Node.js. label May 19, 2015

yosuke-furukawa mentioned this pull request May 19, 2015

Isomorphism nodejs/NG#17

Open

imyller closed this May 26, 2015

imyller deleted the master branch May 26, 2015 18:55

imyller mentioned this pull request May 26, 2015

lib: add console.{group,groupCollapsed,groupEnd}() #1799

Closed

snyk-bot mentioned this pull request Jan 20, 2023

[Snyk] Upgrade commander from 2.11.0 to 9.4.1 Supermajestcd/node#3

Open

lib: add console.{group,groupCollapsed,groupEnd}() #1727

lib: add console.{group,groupCollapsed,groupEnd}() #1727

Conversation

imyller commented May 18, 2015

imyller commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

3y3 commented May 18, 2015

imyller commented May 18, 2015

Fishrock123 commented May 18, 2015

silverwind commented May 18, 2015

3y3 commented May 18, 2015

imyller commented May 18, 2015

imyller commented May 18, 2015

ChALkeR commented May 18, 2015

3y3 commented May 18, 2015

imyller commented May 18, 2015

ChALkeR commented May 18, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rvagg commented May 18, 2015

imyller commented May 18, 2015

silverwind commented May 18, 2015

3y3 commented May 18, 2015

silverwind commented May 18, 2015

rvagg commented May 18, 2015

imyller commented May 18, 2015

Fishrock123 commented May 18, 2015

imyller commented May 18, 2015

silverwind commented May 19, 2015

yosuke-furukawa commented May 19, 2015