Enabled colorization

[synapse]

• Turned on colorization by default
• Fixed tests to match colored outputs

fixes #35

Checklist

• run npm run test and npm run benchmark
• tests and/or benchmarks are included
• documentation is changed or added
• commit message and code follows the Developer's Certification of Origin
  and the Code of conduct

[mcollina]

This is partially correct. We should mimic the behavior of pino-pretty, where colorization is enable if there is a human being attached (a TTY), and disable it if the output goes to a file etc. Check out pino-pretty.

[synapse]

Hey @mcollina, sure.
I took a look at pino-pretty and it's using colorette to determine if colorization is supported const { isColorSupported } = require('colorette').
What would you recommend, install that dependency or to try grabbing the code from it?

[mcollina]

I think we could expose it in pino-pretty, and then use it here from there.

[synapse]

@mcollina could you give me a review on this one: pinojs/pino-pretty#513

[synapse]

Hi @mcollina, I wanted to let you know that I have made a new commit that includes a check for the presence of colors in TTY, which is set to true by default. However, updating pino-pretty is still necessary. Once the PR is merged and deployed, I will create another commit. Let me know if that works for you.

[mcollina]

pino-pretty v11.1.0 is out.

[gurgunday]

lgtm

[mcollina]

I've pulled this and it does not seem to work. If I redirect the output to a file, the colors are still there.

[synapse]

I've pulled this and it does not seem to work. If I redirect the output to a file, the colors are still there.

Hey @mcollina just a quick question, were you running this from your local bash? I'm guessing pino-pretty.isColorSupported is looking and setting the boolean value based on your TTY support for color and not the redirect output by looking at the colorette package.

const isDisabled = "NO_COLOR" in env || argv.includes("--no-color")
const isForced = "FORCE_COLOR" in env || argv.includes("--color")
const isWindows = platform === "win32"
const isDumbTerminal = env.TERM === "dumb"

const isCompatibleTerminal =
  tty && tty.isatty && tty.isatty(1) && env.TERM && !isDumbTerminal

const isCI =
  "CI" in env &&
  ("GITHUB_ACTIONS" in env || "GITLAB_CI" in env || "CIRCLECI" in env)

export const isColorSupported =
  !isDisabled &&
  (isForced || (isWindows && !isDumbTerminal) || isCompatibleTerminal || isCI)

Do you have any suggestions on how to know, from inside the logger formatter, what will be the outcome of the log (file, std or else)? I'm am guessing this should also overwrite the isColorSupported value, right?

[mcollina]

running the process with CI=true shows the colors

[synapse]

From the colorette's logic when you set CI env you should also need one of the other 3.
As for the other thing you've mention to prevent having colors when redirected to a file, we're still looking into it (will keep you in the loop). Would it make sense to have this directly in pino-pretty?

[mcollina]

Maybe. I'm pretty happy with how pino-pretty work, I need to do some checks first, as I might have been too quick on my last test.

#42 (comment)

[simoneb]

Hey folks, just a quick note here, after discussing offline with @synapse . In order to figure out how existing tools handle it, we checked out jest and here's what we found.

• jest's output goes primarily to stderr. some messages go to stdout but that's a minor detail, the bulk of it goes to stderr
• if you redirect stdout to a file, jest detects that and stops colorizing output, but because the output goes to stderr, you will still see it in the terminal
• if you redirect both stdout and stderr to a file, you get the behavior you want: 1) the output is written to the file and 2) the output is not colorized

So we started looking into how jest does it, and we discovered that:

1. jest uses chalk to colorize output
2. chalk has a fairly convoluted logic to detect this, which is not straightforward to implement, and is considerably more complex than colorette's. see chalk's compared to colorette's

[synapse]

Hey @mcollina any news on this?

[mcollina]

This is ok. There is still one tiny bug to fix.

Consider this:

'use strict'

const fastify = require('fastify')({
  logger: {
    transport: {
      target: './index.js',
      options: {
        colorize: true,
      }
    }
  }
})

fastify.get('/', (req, reply) => {
  req.log.fatal('some info')
  reply.send({ hello: 'world' })
})

fastify.listen({ port: 3042 })

Note that colorize: true is set.

If I do node example.js | less, I get:

However if run it in the normal terminal, I get:

[synapse]

Hey @mcollina,

The less command behaves differently across operating systems (in my case I get colored texts in utf-8 codes).

Here's a summary of our current status to help us decide the next steps:

• This PR was opened after you suggested that enabling colors by default would be best (see your comment).
• The PR removes the colorize option and relies on the isColorSupported function from pino-pretty, which is controlled through various options found in colorette to disable the colors (see here).

Unfortunately, color detection in colorette is very basic (compared to chalk - which is also used in libraries like jest and tap).

To automatically disable the output of UTF-8 color strings when not needed, we should address this issue in either the pino-pretty or colorette repositories.

What do you suggest as the best steps moving forward?

[mcollina]

It doesn't seem we are in the same page. I see that the colorize: true option generates two different outputs when the destination is a TTY or not.
However, the same does not happen with pino-pretty. As both use the same utility, you should be able to achieve the same behavior.

[synapse]

@mcollina I've updated the PR with the changes and also added it in the docs

[mcollina]

linting is failing

[mcollina]

lgtm