lib,test,doc: add option to highlight outputs of warn and error methods

[rayw000]

What changed

1. Add color style types for console.error and console.warn.
2. Colorize output from console.error and console.warn.
3. Update related tests.

Fix #40361

Screenshot:

[targos]

/cc @BridgeAR

[rayw000]

I'm trying to find out why test case test-tty-color-support gives different result on my laptop.

The CI Action outputs

expect=3
actual=2
patterns:
pattern = ^.*\[91m\(node:.*\)\ Warning:\ The\ 'NO_COLOR'\ env\ is\ ignored\ due\ to\ the\ 'FORCE_COLOR'\ env\ being\ set\.$
pattern = ^\(Use\ `.*\ \-\-trace\-warnings\ \.\.\.`\ to\ show\ where\ the\ warning\ was\ created\)$
pattern = ^.*\[39m$
outlines:
outline = (node:89630) Warning: The 'NO_COLOR' env is ignored due to the 'FORCE_COLOR' env being set.
outline = (Use `node --trace-warnings ...` to show where the warning was created)

while on my laptop, the outlines seem correct if I cancel my change to let this test fail and get its output.

expect=2
actual=3
patterns:
pattern = ^\(node:.*\)\ Warning:\ The\ 'NO_COLOR'\ env\ is\ ignored\ due\ to\ the\ 'FORCE_COLOR'\ env\ being\ set\.$
pattern = ^\(Use\ `.*\ \-\-trace\-warnings\ \.\.\.`\ to\ show\ where\ the\ warning\ was\ created\)$
outlines:
outline = [91m(node:22915) Warning: The 'NO_COLOR' env is ignored due to the 'FORCE_COLOR' env being set.
outline = (Use `node --trace-warnings ...` to show where the warning was created)
outline = [39m

[BridgeAR]

Thanks for working on the color support in Node.js!

The current approach would work but I would rather not replace the current coloring with just a single font color. Instead, we could change the background color and or add a sign to the beginning of the output to indicate that it's an error or warning. That way we'd keep the original coloring in place and profit from the stronger warn / error indicator.

As last note: colors are perceived differently around the globe. Red is a positive sign in many Asian countries. See e.g., https://www.shutterstock.com/blog/color-symbolism-and-meanings-around-the-world. So thanks for adding the new style to util.inspect.styles to allow the users to change that. It just does not feel like the right place to add such option (it won't be used by util.inspect() and it's an option directly related to it). Instead, please add an option to the console constructor to allow editing that color and document the new option.

Summary:

1. Add option to console constructor to allow editing the colors
2. Document the option
3. Remove util.inspect.styles entries
4. Use a light / dim background color instead of changing the foreground colors

[rayw000]

Really valuable suggestions. Thank you @BridgeAR !

I'm happy to take them and re-implement later.

[rayw000]

I just pushed my new implementation, along with necessary tests and documentation. Now we can create Console instance with option highlight to enable warn and error highlighting, such as:

const { Console } = require('console');
const logger = new Console({
  stdout: process.stdout,
  stderr: process.stderr,
  highlight: {
    warn: { bgColor: 'bgYellowBright' },
    error: { bgColor: 'bgRedBright', indicator: '\u274c' },
  }
});

logger.warn('Warning!');
// Will output message 'Warning!' along with bright yellow background.
logger.error(new Error('Ah oh!'));
// Will output message 'Ah oh!' along with bright red background,
// and a cross sign in front of the message indicating error.
// I turned down the color brightness of my terminal to make it more readable.

It's not enabled by default. Only explicitly pass option highlight to the constructor of Console can make it work. If this PR is acceptable, I'll introduce environment variables such as NODE_HIGHLIGHT_WARN and NODE_HIGHLIGHT_ERROR, or properties like console.highlight to enable highlighting for the global console instance.

Any suggestions are welcome. Thank you!

[jasnell]

I'm generally ok with making the changes discussed here. I just want to make sure that accessibility concerns are being looked at... specifically, will the color schemes chosen by default be problematic for colorblind users?

[rayw000]

Thank you @jasnell.

This feature doesn't provide a default color theme. It's not enabled until user sets its colors explicitly.

But to be honest, choosing a background color working with multiple foreground colors without losing accessibility is not that easy. It's not only a problem for colorblind users, but also for non-colorblind. As described in my last comment, I have to decrease the brightness of color bright red (it's now actually a dark red) in my terminal settings to make highlighted text not that unreadable. It may not be a problem currently, but it will be once we give the global console instance a default highlight color theme.

[Trott]

@rayw000 Is this still something that you would like to complete? I'd like to see this feature land, but I'm not sure what the next step would be. Is it ready for review from your perspective? Or is there still an issue with a test behaving differently on your laptop than in CI?

[rayw000]

Thank you @Trott .

In my perspective, this PR is ready for review. The differences between my laptop and CI have been solved.

Let me make a summary of the current state.

1. This PR introduces an option highlight to Console, by which we can colorize the output. This feature is not enabled by default. Users can create their own instance of Console to make things colorful, just like what I've mentioned here: lib,test,doc: add option to highlight outputs of warn and error methods #40424 (comment)
2. If this PR is acceptable, I'll expose environment variables like NODE_HIGHLIGHT_WARN and NODE_HIGHLIGHT_ERROR to users to customize the output color schemes in their terminal.

I've just solved the conflicts. We could send it to CI now, if you think this PR is OK.
And I'm happy to discuss if you have any suggestions. :)

[nodejs-github-bot]

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

[rayw000]

Rebase master and do force-with-lease pushing to retry flaky tests.

[nodejs-github-bot]

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

[aduh95]

Rebase master and do force-with-lease pushing to retry flaky tests.

FYI it's better not to force push when there are flaky CI results, it's easier to simply wait for a collaborator to re-spawn them (otherwise all the jobs have to re-run, even the ones that were already green).

[rayw000]

Rebase master and do force-with-lease pushing to retry flaky tests.

FYI it's better not to force push when there are flaky CI results, it's easier to simply wait for a collaborator to re-spawn them (otherwise all the jobs have to re-run, even the ones that were already green).

Glad to hear that!

[nodejs-github-bot]

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