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

console: add color support #19372

Closed
wants to merge 5 commits into
base: master
from

Conversation

@addaleax
Member

addaleax commented Mar 15, 2018

  • util: introduce formatWithOptions()

    Identical to format() except that it takes an options argument
    that is passed through to inspect().

  • doc: document Console(…, ignoreErrors) option

  • console: add color support

    Add a way to tell Console instances to either always use, never use
    or auto-detect color support and inspect objects accordingly.

  • console: auto-detect color support by default

    This makes Node pretty-print objects with color by default
    when console.log()-ing them.

The last commit could be split out into a separate PR if that’s preferred.

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
let MAX_STACK_MESSAGE;
function Console(stdout, stderr, ignoreErrors = true) {
function Console(stdout, stderr, ignoreErrors = true, colorMode = 'auto') {

This comment has been minimized.

@tniessen

tniessen Mar 15, 2018

Member

This is the fourth positional argument to the constructor, maybe we should consider an options object at some point (unless it is foreseeable that we won't add more options to the constructor).

This comment has been minimized.

@addaleax

addaleax Mar 15, 2018

Member

Yea, I’m happy to do that if you think it’s a good idea.

I don’t think an ergonomic API matters all that much here, because almost nobody is constructing their own console objects, but I could totally see the argument being made for it.

This comment has been minimized.

@tniessen

tniessen Mar 15, 2018

Member

I shouldn't argue for either side, I never use the constructor, it's just a general concern I'd have with any API. If you type-check colorMode strictly, we can still replace it with an options object later.

This comment has been minimized.

@addaleax

addaleax Mar 15, 2018

Member

I think if we do go for an options argument, it should probably encompass all arguments here – so, new Console({ stdout, stderr, ignoreErrors, colorMode, ... }).

I’m not sure how well that would work, though, because all of these properties could just be accidentally set on the stdout object itself…

This comment has been minimized.

@jasnell

jasnell Mar 16, 2018

Member

An options object would be best, I think.

This comment has been minimized.

@addaleax

addaleax Mar 17, 2018

Member

@jasnell Do you have thoughts on how we would detect an options object?

I guess we could try to see whether it’s a duck and has a write() property…

This comment has been minimized.

@tniessen

tniessen Mar 17, 2018

Member

Maybe Console(stdout, [stderr, [options]])? That kind of makes sense as ignoreErrors and colorMode have reasonable defaults and can be distinguished from an object.

This comment has been minimized.

@jasnell

jasnell Mar 17, 2018

Member

If going with the options property I would use the single argument strategy you suggest above and check if the first argument is an object with at least the stdin property set that does not have a write or maybe _write property whose value is a function.

-->
This function is identical to [`util.format()`][], except in that it takes
an `inspectOptions` argument which specifies default options for

This comment has been minimized.

@tniessen

tniessen Mar 15, 2018

Member

Question: "default" as in "unless overriden by custom inspect functions"?

This comment has been minimized.

@addaleax

addaleax Mar 15, 2018

Member

Yeah, I think default can be dropped here. :) It’s not really overridden unless it’s a format string with %o in it, but we already document that.

@@ -62,6 +67,7 @@ function Console(stdout, stderr, ignoreErrors = true) {
Object.defineProperty(this, '_stderrErrorHandler', prop);
this[kCounts] = new Map();
this[kColorMode] = colorMode;

This comment has been minimized.

@tniessen

tniessen Mar 15, 2018

Member

The constructor should check for invalid values before assigning.

This comment has been minimized.

@addaleax

addaleax Mar 15, 2018

Member

👍 done!

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Mar 15, 2018

@addaleax Do you think if we should support a subset of the %c feature?

E.g. console.log('%cgreen%c, normal', 'color:green', '').
In this example, first %c sets the green style, second %c resets by applying empty styles.

That is a part of console spec and works in major browsers, and I suppose it could be relatively easy implemented with the specific set of numbered colors that are supported by terminals.

@addaleax

This comment has been minimized.

Member

addaleax commented Mar 15, 2018

Do you think if we should support a subset of the %c feature?

I think that would be awesome. But it’s also more or less independent of this PR, right?

@ChALkeR

This comment has been minimized.

Member

ChALkeR commented Mar 15, 2018

@addaleax Yes, that could be discussed separately.

@addaleax

This comment has been minimized.

Member

addaleax commented Mar 17, 2018

@jasnell @tniessen Okay, updated this to use an options object. PTAL

@lpinca

lpinca approved these changes Mar 18, 2018

* `colorMode` {boolean|string} Set color support for this `Console` instance.
Setting to `true` enables coloring while inspecting values, setting to
`'auto'` will make color support depend on the value of the `isTTY` property
of the respective stream. Defaults to `auto`.

This comment has been minimized.

@lpinca

lpinca Mar 18, 2018

Member

Nit: 'auto' for consistency.

tempStr = inspect(arguments[a++],
{ showHidden: true, showProxy: true });
{
const opts = Object.assign({}, inspectOptions, {

This comment has been minimized.

@lpinca

lpinca Mar 18, 2018

Member

Nit: how about

const opts = Object.assign({
  showHidden: true,
  showProxy: true
}, inspectOptions);

Feel free to ignore.

This comment has been minimized.

@addaleax

addaleax Mar 18, 2018

Member

@lpinca This is kind of intentional … we document that the %o specifier works this way, so I wouldn’t expect per-call options to override it

This comment has been minimized.

@lpinca

lpinca Mar 18, 2018

Member

In what case would it be overridden? It's still a new copy per call no?

This comment has been minimized.

@addaleax

addaleax Mar 23, 2018

Member

@lpinca I meant, if inspectOptions happens to contains showProxy: false, then that would override the behaviour of %o with your suggestion, whereas right now the behaviour of %o is left untouched.

If you do think that that is the right thing to do, then I’m okay with that; I’d prefer to keep this as it is currently documented, though.

This comment has been minimized.

@lpinca

lpinca Mar 23, 2018

Member

You are right, ignore my comment.

@BridgeAR BridgeAR self-requested a review Mar 20, 2018

@addaleax

This comment has been minimized.

@benjamingr

Hey, just temporarily requesting changes so that you get a chance to see #19652 (comment) before anyone lands this.

Actual PR content looks good to me. Once you have had a chance to read it (and consider adding a flag maybe?) please feel free to dismiss this review :)

@addaleax

This comment has been minimized.

Member

addaleax commented Mar 28, 2018

@benjamingr I’m not sure I quite understand what you are pointing at … are you worried that libraries might have to clean up Node’s output after this patch? I don’t think we need to worry about that, since this feature is only enabled by default if output goes to a TTY that has some indication for color support.

In any case, thanks for the ping, I rebased this :)

Dismissing my review since @addaleax saw the ping.

@benjamingr

This comment has been minimized.

Member

benjamingr commented Mar 28, 2018

I'm not familiar with Jest's internals and I was worried that it might be problematic for tooling folk that uses it. If you're sure that's not an issue then I think this is a cool feature to have :D

@addaleax

This comment has been minimized.

Member

addaleax commented Mar 28, 2018

@benjamingr I guess it’s not a bad idea to run CITGM on this, since I’m not familiar with Jests internals either, but yes, it seems highly unlikely that this is going to be an issue for them… The coloring is not going to show up if Node’s output is captured from a child process or something like that.

@addaleax addaleax added the blocked label Mar 28, 2018

@addaleax

This comment has been minimized.

Member

addaleax commented Mar 28, 2018

I’ll label this as blocked until #17615 (comment) is resolved. If we do remove color depth support from TTY, I guess we can just ignore that check or turn it into something simpler.

@BridgeAR

This comment has been minimized.

Member

BridgeAR commented Apr 9, 2018

@addaleax I think the issue is resolved and this is not blocked anymore.

@BridgeAR BridgeAR removed the blocked label Apr 9, 2018

* `stdout` {stream.Writable}
* `stderr` {stream.Writable}
### new Console(options)
### new Console(stdout[, stderr][, ignoreErrors])

This comment has been minimized.

@addaleax

addaleax Apr 9, 2018

Member

@vsemozhetbyt The doctool doesn’t seem to be able to handle this anymore, since 9a6dd07?

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 9, 2018

Member

Do you mean two consecutive headings? I cannot say why it should not. We have many of them in the bufer.md and they seem to be handled properly. Do you encounter issues with tests or doc building?

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 9, 2018

Member

Oh, sorry, now I've tested this. It seems the consecutive headings we already have contain similar parameters and they do not differ from the list parameters below. Before 9a6dd07, these differences were checked, but they did not throw, just reported to stderr. It seems we should either split the section or change the parser strictness, which may not be good for detecting differences between signatures and parameter lists.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 9, 2018

Member

BTW, consecutive headings are parsed (and were parsed) as clones: the section bodies are copied and checked identically: see here and here. This is not changed in 9a6dd07, I've just make the differences throw.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 12, 2018

Member

So two headings with a similar name but different signatures do not confuse the JSON parser as far as the second signature is in sync with the parameter list below. However, it seems this is still the only case in our docs for now. And it can be a bit confusing: the first signature parameters are not openly listed (they are similar to object properties/options fields, but this is a bit non-formal way). Maybe we can split them, add an own minimal parameter list to the first signature and a link to the common description in the second section? It is a vague thing for me and it is not a strong opinion, so feel free to ignore) We still have a valid JSON (just a bit overflooded with parameter list clones) and the doc is still pretty readable in this way.

description: The `ignoreErrors` option was introduced.
- version: REPLACEME
pr-url: https://github.com/nodejs/node/pull/19372
description: The `Console` constructor now supports an options argument,

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 10, 2018

Member

options -> `options`?

and the `colorMode` option was introduced.
-->
* `options` {object}

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 10, 2018

Member

{object} -> {Object}

* `stdout` {stream.Writable}
* `stderr` {stream.Writable}
* `ignoreErrors` {boolean} Ignore errors when writing to the underlying
streams. Defaults to `true`.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 10, 2018

Member

Defaults to `true`. -> **Default:** `true`.

Setting to `true` enables coloring while inspecting values, setting to
`'auto'` will make color support depend on the value of the `isTTY` property
and the value returned by `getColorDepth()` on the respective stream.
Defaults to `'auto'`.

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 10, 2018

Member

Defaults to `'auto'`. -> **Default:** `'auto'`.

<!-- YAML
added: REPLACEME
-->

This comment has been minimized.

@vsemozhetbyt

vsemozhetbyt Apr 10, 2018

Member

Maybe it is worth to just mention the parameter types:

* `inspectOptions` {Object}
* `format` {string}

addaleax added some commits Mar 15, 2018

util: introduce `formatWithOptions()`
Identical to `format()` except that it takes an options argument
that is passed through to `inspect()`.
console: add color support
Add a way to tell `Console` instances to either always use, never use
or auto-detect color support and inspect objects accordingly.
console: auto-detect color support by default
This makes Node pretty-print objects with color by default
when `console.log()`-ing them.
@addaleax

This comment has been minimized.

Member

addaleax commented Apr 12, 2018

@addaleax

This comment has been minimized.

Member

addaleax commented Apr 12, 2018

Landed in 039cdeb...565fd50

@addaleax addaleax closed this Apr 12, 2018

@addaleax addaleax deleted the addaleax:console-tty-colors branch Apr 12, 2018

@addaleax addaleax removed the author ready label Apr 12, 2018

addaleax added a commit that referenced this pull request Apr 12, 2018

util: introduce `formatWithOptions()`
Identical to `format()` except that it takes an options argument
that is passed through to `inspect()`.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Apr 12, 2018

doc: document `Console(…, ignoreErrors)` option
PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Apr 12, 2018

console: allow `options` object as constructor arg
PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Apr 12, 2018

console: add color support
Add a way to tell `Console` instances to either always use, never use
or auto-detect color support and inspect objects accordingly.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

addaleax added a commit that referenced this pull request Apr 12, 2018

console: auto-detect color support by default
This makes Node pretty-print objects with color by default
when `console.log()`-ing them.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 16, 2018

util: introduce `formatWithOptions()`
Identical to `format()` except that it takes an options argument
that is passed through to `inspect()`.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 16, 2018

doc: document `Console(…, ignoreErrors)` option
PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 16, 2018

console: allow `options` object as constructor arg
PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 16, 2018

console: add color support
Add a way to tell `Console` instances to either always use, never use
or auto-detect color support and inspect objects accordingly.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

jasnell added a commit that referenced this pull request Apr 16, 2018

console: auto-detect color support by default
This makes Node pretty-print objects with color by default
when `console.log()`-ing them.

PR-URL: #19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

util: introduce `formatWithOptions()`
Identical to `format()` except that it takes an options argument
that is passed through to `inspect()`.

PR-URL: nodejs#19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

doc: document `Console(…, ignoreErrors)` option
PR-URL: nodejs#19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

console: allow `options` object as constructor arg
PR-URL: nodejs#19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

console: add color support
Add a way to tell `Console` instances to either always use, never use
or auto-detect color support and inspect objects accordingly.

PR-URL: nodejs#19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>

BridgeAR added a commit to BridgeAR/node that referenced this pull request May 1, 2018

console: auto-detect color support by default
This makes Node pretty-print objects with color by default
when `console.log()`-ing them.

PR-URL: nodejs#19372
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins

This comment has been minimized.

Member

MylesBorins commented Aug 7, 2018

is this something we should backport to v8.x?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment