Respect NO_COLOR environment variable in the dev format - #372
Conversation
Fixes expressjs#302. morgan('dev') always emitted ANSI color codes, with no way to disable them, even though NO_COLOR (https://no-color.org/) is the widely adopted convention for this and colored escape codes in logs can be a real accessibility problem. When NO_COLOR is set to a non-empty value, compile and use a plain, escape-code-free variant of the dev format line instead of the colored one, caching it the same way the colored variants are already cached.
MaddyGuthridge
left a comment
MaddyGuthridge
left a comment
There was a problem hiding this comment.
Looks pretty good to me. The design issues I spot with it are just continuations of the existing (imo questionable) design that already exists in the package, and so it's probably better to fix those in a different PR if we can be bothered.
| var plainFn = developmentFormatLine.plain || (developmentFormatLine.plain = | ||
| compile(':method :url :status :response-time ms - :res[content-length]')) |
There was a problem hiding this comment.
Although it does match the behaviour elsewhere in this function, adding attributes to the function seems like an extremely strange design decision. It's not type-safe, and makes it quite unclear where the definitions actually come from.
There was a problem hiding this comment.
yeah it's house style and not worth a rewrite for this, but these are internal if that makes you feel any better about the type safety of it
| // NO_COLOR (https://no-color.org/): when present and not an empty | ||
| // string, regardless of its value, ANSI color codes must not be added | ||
| if (process.env.NO_COLOR) { | ||
| var plainFn = developmentFormatLine.plain || (developmentFormatLine.plain = |
There was a problem hiding this comment.
The nullish coalescing operator would be preferable for the expression too, although I don't think it is supported by versions of Node as old as what Morgan targets.
There was a problem hiding this comment.
correct, cannot use ?? in this release line
|
Im going to supersede this PR with my own in #377 |
4 participants
Summary
Fixes #302.
morgan('dev')always emits ANSI color escape codes with no way to turn them off, even thoughNO_COLORis the widely adopted convention for this, and colored escape codes mixed into logs can be a real accessibility problem for some terminal/screen-reader setups.Fix
When
NO_COLORis set to a non-empty value,devcompiles and uses a plain, escape-code-free variant of the format line instead of the colored one (:method :url :status :response-time ms - :res[content-length], i.e. the same fields the colored line already prints, just without the\x1b[...]codes). It's cached the same way the colored variants already are, so there's no extra compile cost per request.No other formats are affected —
devis the only built-in format that emits color.Test plan
devdescribe block: withNO_COLOR=1set, asserts the logged line contains no\x1bescape character and matches the expected plain fields.indexOf('\x1b')returns0instead of-1, i.e. an escape code is present at the start of the line).npx mocha --check-leaks --reporter spec— 89 passing.npm run lint(eslint) passes on the changed files.