nodejs
console: colorize console error and warn
Related to : 40361
Colorise string type args of console.error and console.warn. We are skipping the non-string types.
Tasks:
- [ ] Run CITGM
@MoLow I have changed the value of clear from '\u001bc' to '\u001b[0m'. Please check.
I believe it's important to run through citgm to gauge the impact of this change.
I once again believe this should be marked as semver major change: https://github.com/nodejs/node/pull/51503
There are some values which look quite broken with this change:
./node -e 'console.warn(new Error);console.error(new Error)'shows that that color stays until the first greyed line in the stack../node -e 'console.error(Promise.resolve(2))'./node -e 'console.error(new ArrayBuffer)'
Also worth noting other primitive values (number, BigInt, functions, symbols):
./node -e 'console.error(0)'is still yellow../node -e 'console.error(Symbol("test"))'is still green.
I wonder if it would make more sense to apply the style only to strings, and leave the other types of value untouched.
There are some values which look quite broken with this change:
./node -e 'console.warn(new Error);console.error(new Error)'shows that that color stays until the first greyed line in the stack../node -e 'console.error(Promise.resolve(2))'./node -e 'console.error(new ArrayBuffer)'Also worth noting other primitive values (number, BigInt, functions, symbols):
./node -e 'console.error(0)'is still yellow../node -e 'console.error(Symbol("test"))'is still green.I wonder if it would make more sense to apply the style only to strings, and leave the other types of value untouched.
Thanks for the suggestions. Now, we will colorise only for string types. The other improvements mentioned here for existing cases, will handle in a different issue or PR.
I think something like special printing Error in console.error|console.warn could be a potential evolution of this implementation, e.g:
$ node -e 'console.error(new Error("ERR"))'
- Error: ERR
at [eval]:1:15
at runScriptInThisContext (node:internal/vm:209:10)
at node:internal/process/execution:118:14
at [eval]-wrapper:6:24
at runScript (node:internal/process/execution:101:62)
at evalScript (node:internal/process/execution:133:3)
at node:internal/main/eval_string:51:3
But as you can see in my little illustration above, I would prefer it to be just a highlight of that header rather than the entire error stack - in order for that bit to be a proper highlight - instead of an entire coloured blob of text that ultimately diminishes its meaning.
would prefer it to be just a highlight of that header rather than the entire error stack - in order for that bit to be a proper highlight - instead of an
Currently we are skipping the non-string types. So, node -e 'console.error(new Error("ERR"))'. will be skipped colouring.
I'm sorry @MrJithil but while this is a good work in progress and I initially had no comments, I took some time thinking a bit more about this change and I currently have 2 objections to landing this PR:
- Colouring the entire output might ultimately diminish the meaning of that colouring and only makes the entire output hard to read (see Use color with intention).
- We recently landed util.styleText which might be more suitable for this use case since users can now chose what part to color from their output using a native helper from the runtime. If this PR is to land as-is it should be using that API or its internals.
One other thought I currently have in my mind is that if there is even any prior art from other runtimes on colouring error / warn output by default similar to how it's implemented here. If there are examples of that it could then be used as supporting argument for this change.
There is no specific request which demands the urgency of this feature. I thought it would be great if have something like this in REPL. Like browsers, it would be great to differentiate an error, warn and log from its colour.
As you suggested styleText can be used to colour the warn and error.
But, one question here, in REPL don't we need to check whether the tty supports colouring before adding colour codes?
Currently we are skipping the non-string types. So,
node -e 'console.error(new Error("ERR"))'. will be skipped colouring.
Then I don't think this PR fixes #40361, might be better to update its description.
There is no specific request which demands the urgency of this feature. I thought it would be great if have something like this in REPL. Like browsers, it would be great to differentiate an error, warn and log from its colour.
I'm happy to approve this PR if it applies to REPL-only but I don't believe this to be the case at the moment.
Currently we are skipping the non-string types. So,
node -e 'console.error(new Error("ERR"))'. will be skipped colouring.Then I don't think this PR fixes #40361, might be better to update its description.
There is no specific request which demands the urgency of this feature. I thought it would be great if have something like this in REPL. Like browsers, it would be great to differentiate an error, warn and log from its colour.
I'm happy to approve this PR if it applies to REPL-only but I don't believe this to be the case at the moment.
Its not only for REPL. It will be applicable to all node console output.
I thought a bit more about this after talking with a few other folks and changed my mind on the remaining objection. LGTM 👍
thank you. proceeding further.