Default logger middleware doesn't check that stdout is a TTY

[gpanders]

What version of Hono are you using?

4.6.16

What runtime/platform is your app running on? (with version if possible)

Cloudflare Workers

What steps can reproduce the bug?

Use the default logger middleware provided by Hono https://hono.dev/docs/middleware/builtin/logger

Observe that emitted logs contain ANSI escape sequences when the output is not a TTY:

This screenshot is taken from Cloudflare Workers logs, but the same would be true for any log output which is not a terminal.

What is the expected behavior?

Hono should check that stdout is a TTY when determining whether or not to emit ANSI escape sequences for colors: https://github.com/honojs/hono/blob/2ead4d8faa58d187bf7ec74bac2160bab882eab0/src/utils/color.ts#L13-L26

In Node this can be done with Boolean(process.stdout.isTTY) === true https://nodejs.org/api/tty.html#tty

Deno has Deno.stdout.isTerminal() https://docs.deno.com/api/deno/~/Deno.stdout.isTerminal

What do you see instead?

No response

Additional information

A workaround in the meantime is to set NO_COLOR manually if stdout is not a TTY:

if (Boolean(process.stdout.isTTY) === false) {
    process.env["NO_COLOR"] = "1"
}

~~Note however that even this will not work in Cloudflare Workers because of another issue with the process polyfill: https://github.com/unjs/unenv/issues/380~~

[yusukebe]

HI @gpanders

Thank you for raising the issue. There is a similar issue https://github.com/honojs/hono/issues/3751. We have the hacky way, but using checking TTY is a good idea.

Hey @ryuapp! What do you think of checking TTY for the logger?

[ryuapp]

Hi

Hey @ryuapp! What do you think of checking TTY for the logger?

It seems like one good idea. One concern is that some, such as GitHub Actions, don't have a TTY, which makes it difficult to control. Then someone might ask for FORCE_COLOR and make things complicate. Hono is generally not executed with GitHub Actions, so the benefits of checking TTY look great for me.

[yusukebe]

Since version 4.8.0 includes #4094 (thank you, @ryuapp ), the logger looks at NO_COLOR in Cloudflare Workers environment variables. So, developers who want to disable coloring can set the value to NO_COLOR in the production environment.

It's good, but ideally, we want the logger to disable coloring without setting a variable, by checking the TTY. Hey @ryuapp, should we try to implement that feature? What do you think of this?

[ryuapp]

Sorry for the late reply.

It's good, but ideally, we want the logger to disable coloring without setting a variable, by checking the TTY. Hey @ryuapp, should we try to implement that feature? What do you think of this?

This seemed good at the time, but then I realized that with Cloudflare Workers, TTYs don't exist. Therefore, I believe disabling color on Cloudflare Workers and supporting FORCE_COLOR instead will meet most needs for now. same comment: https://github.com/honojs/hono/issues/4301#issuecomment-3114092802

[yusukebe]

@ryuapp

Thank you for the comment!

Regarding FORCE_COLOR, did you mean we can get FORCE_COLOR in the app on Cloudflare Workers? I think it's impossible, but can we do that?

[NamesMT]

https://github.com/unjs/std-env have a isColorSupported() util that is pretty good including TTY check, I have created a PR to extract it in the past: https://github.com/honojs/hono/pull/3017, but I can't remember why I drafted it, kinda reach a burnt-out in that time.

[yusukebe]

but I can't remember why I drafted it, kinda reach a burnt-out in that time.

Haha. I'll take a look at it.

[ryuapp]

Regarding FORCE_COLOR, did you mean we can get FORCE_COLOR in the app on Cloudflare Workers? I think it's impossible, but can we do that?

As with NO_COLOR, this can be done by dynamically importing cloufdlare:workers to get env object. The approach in #3017 looks barely correct once you resolve the conflicts.

PS: The following was is a little misunderstanding on my part. Since there are no TTYs on workers, we can correctly create hasTTY.

This seemed good at the time, but then I realized that with Cloudflare Workers, TTYs don't exist.

[jdmarshall]

https://nodejs.org/api/util.html#utilstyletextformat-text-options Takes care of this automatically, but I’m not sure what bun and Deno have available