Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@imyller
Copy link
Member

commented May 26, 2015

Implements nested console groups. This improves code portability by matching with console object standard proposal and major browsers 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.

(This is resubmit of #1727 to replace invalid source branch)

@Fishrock123

This comment has been minimized.

Copy link
Member

commented May 31, 2015

I'm still not really sure core should add this kind of thing.

@imyller

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2015

What is the policy on closing PRs that are reviewed not to be interesting/relevant to the core?

Does someone from maintainer/collaborator team close the PR or is it up to the PR author?

Either way is fine by me as this PR was offered just in case the consistency with browser JS console/debug tooling is something io.js maintainers end up wanting.

@brendanashworth

This comment has been minimized.

Copy link
Member

commented Jun 1, 2015

@imyller typically, if someone has an opinion on it, discussion ensues. First, we attempt to reach concensus in the thread, where everyone (or mostly everyone) agrees that either it should be closed or it should be opened. If this is reached (currently it doesn't look like it has been), usually a maintainer will close the PR. If concensus isn't reached, it will be escalated to tsc-agenda where the TSC makes the final decision.

Of course, the PR author can always close it at their own whim, if they CBA or have second thoughts.

@imyller

This comment has been minimized.

Copy link
Member Author

commented Jun 1, 2015

@brendanashworth Thank you for the explanation.

Console groups are only one part of the full console object standard proposal.

Main reason I chose to implement them is that they enable GUI debuggers (such as node-inspector) to offer similar features for Node and browser JS environments.

I think the discussion and consensus seeking should be done on whether io.js eventually wants to offer standard console or will it stay with the current similarly named console originating mostly from Node.js v0.8 - 0.10.

Having followed V8/Chrome, SpiderMonkey/FF and Chakra/Edge for a while it seems they are trying to stay mostly compatible with each others implementation of console. Maybe io.js should too?

@chrisdickinson

This comment has been minimized.

Copy link
Contributor

commented Jun 1, 2015

I'm +0 on this, since it brings us closer to other implementations of console, and having to memorize the differences between the environments can be a pain. @Fishrock123, could you expand on why you don't think this belongs in core?

@sam-github

This comment has been minimized.

Copy link
Member

commented Jun 1, 2015

I'm +0, too.

I don't see node being very similar to the browser here, things like https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#consoleerrorobject--object- are not going to work the same (their console.error is more like our console.trace). It seems like a lot of the console additions are built with a dev tools ui in mind, and you make the calls wanting some particular look in the UI.

But console output in node doesn't usually go to such a viewer, it goes to stdout, so node devs aren't going to use these new group methods, even if node-inspector learned how to patch them somehow, and treat them well. Mostly, they would help code written for the browser run on node. Is that common? Isn't the opposite direction more common?

Also, for pure node dev, having a bunch of API that doesn't do very useful things is kind of strange.

Maybe this would be best in a userland polyfill, maybe called "browser-support", it would patch into node things that aren't useful for server-side js, but that makes it easier to pretend to be enough like a browser that browser code can run on node.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Jun 2, 2015

@Fishrock123, could you expand on why you don't think this belongs in core?

Simply on the basis that we should decide beforehand formally where we want to follow browsers in core, and what we want to leave to userland.

We've argued "why not in userland" against other things; why not here also? I mean, Browsers don't have that same sort of easy access to a good package manager as we do, so it makes more sense for them to bundle more. But they are becoming more and more like a massive stdlib nightmare, and I'd like to stay far away from that.

That was a primary goal for us, right? -- to avoid implementing what can be done in userland? I don't really see the difference between something like #1177, and this, other than this is "in browsers". To me, "in browsers" is a weak argument on it's own. I really don't see how "isomorphism" for us is beneficial in that sense.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Jun 2, 2015

Console groups are only one part of the full console object standard proposal.

If the goal is to have it everywhere, it's their own fault for not pulling anyone from node into the discussions. :/

and having to memorize the differences between the environments can be a pain.

I am sympathetic to this point, but I would like to see everything discussed first, probably in a tc-meeting.

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2015

+1 from me, I'm a big fan of modules that work both in browsers and node, and ES6 modules will bring us closer to that. For that to work, we need to provide the same API where we can.

@imyller

This comment has been minimized.

Copy link
Member Author

commented Jun 2, 2015

@Fishrock123

To me, "in browsers" is a weak argument on it's own. I really don't see how "isomorphism" for us is beneficial in that sense.

The fact is that Node.js has already opted to include something that originates from browsers; the console object. So at this point I don't think this is about including or not; this is more about keeping up-to-date something that was decided in the past.

I would like to see everything discussed first, probably in a tc-meeting.

Sounds good. The future of console.

Keep in mind that pull request is never an attempt to force something on someone, its merely just a helpful suggestion and a place for discussion.

@chrisdickinson

This comment has been minimized.

Copy link
Contributor

commented Jun 2, 2015

I'm not sure we should bring this up in a TC meeting yet. We've brought issues similar in flavor to the TC before, only to have it punted back to the tracker; let's try to come to consensus in this issue.

That was a primary goal for us, right? -- to avoid implementing what can be done in userland? I don't really see the difference between something like #1177, and this, other than this is "in browsers".

There exists a set of objects and properties that make up the object graph of an expected JS runtime. Node has said that it won't support all of those objects and properties – some don't make sense for the environment we're trying to provide. However, for the global properties we've said we support – like console – we attempt to support the entire object subgraph. We did the same for TypedArray in v0.10, and the Intl object in v0.12.

With the assert case, userland has the option of using a different module to provide that functionality. In the console.group case, the feature being proposed isn't the grouping output itself, it's the ability for user code to rely on console.group without having to polyfill or otherwise check for availability. This is not a thing that userland can do reliably.

If the goal is to have it everywhere, it's their own fault for not pulling anyone from node into the discussions. :/

Perhaps it would be more constructive to seek out the people making those decisions and engage with them? We should keep in mind that our platform represents a far smaller install base than the major browsers. We can't expect the folks doing this work to mind our platform as well as their own without meeting them halfway.

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Jun 2, 2015

However, for the global properties we've said we support – like console – we attempt to support the entire object subgraph. We did the same for TypedArray in v0.10, and the Intl object in v0.12.

Fair enough. +0 then.

@bnoordhuis bnoordhuis force-pushed the nodejs:master branch to b926718 Jun 2, 2015

@yosuke-furukawa

This comment has been minimized.

Copy link
Member

commented Jun 3, 2015

+1 for support same API.
-0.5 for implement console.group firstly.

I think console.group priority is not so high.

If our console API follows the spec, We should follow the MUST and SHOULD level as a high priority.

And I have a concern about the compatibility. We may break the compatibility about the format-specifiers. (that is the SHOULD level).

https://github.com/DeveloperToolsWG/console-object/blob/master/api.md#format-specifiers

Specifier Description node/io.js implement status
%s Formats the value as a string (cooercing via toString() if necessary) implemented properly
%d, %i Formats the value as an integer Formats the value as a number (not integer and %i is not implemented)
%f Formats the value as a floating point value not implemented but %d is instead
%o Formats the value as an expandable DOM Element (or JavaScript Object if it is not) not implemented
%O Formats the value as an expandable JavaScript Object not implemented but %j is implemented
%c Formats the output string according to CSS styles you provide not implemented

And DeveloperToolWG codify de-facto standard API for console. But they do not consider node/io.js console API. We should open the issue to the repo and write our specification on console API.

@imyller

This comment has been minimized.

Copy link
Member Author

commented Jun 3, 2015

I agree with the points @yosuke-furukawa made.

However, as much as we all would like to create a single PR completely redefining the console, I dont believe that would (or should) ever pass the collaborator review. Small steps. Small steps.

@rvagg rvagg force-pushed the nodejs:master branch to 628a3ab Jun 25, 2015

@indutny indutny force-pushed the nodejs:master branch to eb35968 Jul 22, 2015

@jasnell

This comment has been minimized.

Copy link
Member

commented Aug 27, 2015

Finally getting around to this conversation... while I can understand the reluctance to introduce additional APIs that are questionably useful in general to node.js developers, I think aligning with the browser API here is important and trumps those concerns. So I'd be +1 on getting this, or some variation, landed.

@rvagg rvagg force-pushed the nodejs:master branch from 11c25c2 to ba02bd0 Sep 6, 2015

@silverwind

This comment has been minimized.

Copy link
Contributor

commented Oct 22, 2015

@imyller could you rebase and squash into a single commit? I'd like to revisit this.

@jasnell

This comment has been minimized.

Copy link
Member

commented Nov 16, 2015

@silverwind @Fishrock123 @imyller ... what do we want to do with this?

@Trott Trott force-pushed the nodejs:master branch to 082cc8d Dec 27, 2015

lib: add console.{group,groupCollapsed,groupEnd}()
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 imyller force-pushed the imyller:console-group branch 2 times, most recently to fcac8a0 Feb 2, 2016

@imyller

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2016

@silverwind @Fishrock123
Now squashed into a single commit

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

@nodejs/ctc ... ping? Any thoughts?

@Fishrock123

This comment has been minimized.

Copy link
Member

commented Feb 2, 2016

We basically already decided not to follow the "spec" (not really an actual spec) elsewhere, so that isn't really a good argument for it.

@imyller

This comment has been minimized.

Copy link
Member Author

commented Feb 2, 2016

Thank you for taking this feature suggestion into consideration.

My personal opinion is that the core libraries having their origins in browser JS (like console) should be kept in sync with their browser counterparts; but to a reasonable degree and where it does not conflict with Node.js specific needs.

Even with the lack of standard, browsers seem to loosely follow each others console API changes and I don't see why Node.js couldn't do the same.

@rvagg

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

some of us already commented in #1727, I'm somewhere between -0 and -1 on this

@jasnell

This comment has been minimized.

Copy link
Member

commented Feb 3, 2016

Given that, I don't think we have the necessary consensus to get this landed. I know it's come up several times before and the overall feeling was -1

@benjamingr

This comment has been minimized.

Copy link
Member

commented Mar 20, 2016

Anyone objects to this being closed?

@jasnell

This comment has been minimized.

Copy link
Member

commented Mar 21, 2016

Nope. Closing.

@jasnell jasnell closed this Mar 21, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.