feat: NO_COLOR env var removes color in dev format

[jonchurch]

My approach to solve #302

Taking a dep on the env for the behavior of dev format is the whole feature, which required a little hardening of the test setup to ensure that people for whom this feature is intended, users of NO_COLOR can still run the normal test suite locally without it failing due to NO_COLOR failing the main suite's dev tests.

Because mocha doesnt support env or process isolation well (a second file in the same mocha invocation shares process.env across all files) and my approach favored checking NO_COLOR once at module require, I added a second mocha invocation to the main test npm script.

This shouldnt affect nyc coverage, it should still merge it all ¯\_(ツ)_/¯

[jonchurch]

I went way down a rabbit hole on this one tbh.

The original PR was mergeable, but still I didnt like the read of process.env on every log, it almost triples the perf cost (we're talking like ~200 nanoseconds to ~600 nanoseconds on an already only for dev format, so not really relevant, but still, and the cost was worse for users w/ it unset bc thats how process.env reads work). And the fact that the PR would fail tests for anyone who presumably uses NO_COLOR themeslves just seemed a little tragic.

We got like 10 autoamted PRs for this one issue or something, and I dont know that any even addressed these small issues, but I feel good about the time spent and taking this one over.

[krzysdz]

If we're going to check NO_COLOR on module import, maybe we could just "overwrite" the dev format instead of adding a condition that is executed every time (something that I suggested in #303 (comment)).

[jonchurch]

That's a good approach @krzysdz, I got too distracted by meeting what was already in there, ill push up a change

[krzysdz]

If morgan didn't have to support old Node.js versions we could use some functions from node:util/node:tty:

• util.styleText(): detects NO_COLOR, FORCE_COLOR, NODE_DISABLE_COLORS and support for displaying colours. Unfortunately, it would probably have worse performance as it checks those on every function call and requires Node.js v20.18.0/v22.8.0 (or v26.1.0 for #RRGGBB)
• util.stripVTControlCharacters(): just strips all control characters (not really necessary, especially since the user input is mostly sanitised - b3f5d9b), requires Node.js v16.11.0
• writeStream.hasColors() (as process.stdout.hasColors()): respects the env variables and checks if the terminal supports the requested number of colours (like util.styleText()). Probably the most useful, but requires Node.js v10.16.0

[jonchurch]

weirdly the ecosystem is not even very consistent w/ NO_COLORS, Node's builtins dont implement the NO_COLORS='' empty string case where it's supposed to only turn off colors "when present and not an empty string". That part was maybe added to the informal "spec" later after some implemented their behavior? (yeah, 2022 jcs/no_color@99f90e2)

If we land it like this, compliant with the text, then we're actually noncompliant with node's built in tools. Im down to merge this and not think about it again though.

[krzysdz]

weirdly the ecosystem is not even very consistent w/ NO_COLORS, Node's builtins dont implement the NO_COLORS='' empty string case where it's supposed to only turn off colors "when present and not an empty string". That part was maybe added to the informal "spec" later after some implemented their behavior?

If we land it like this, compliant with the text, then we're actually noncompliant with node's built in tools. Im down to merge this and not think about it again though.

writeStream.hasColors() on v26.8.1 checks if the NO_COLOR string is non-empty

It would be really funny if Node.js used different (and not compatible) ways of checking it in multiple places.