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:

hono/src/utils/color.ts

Lines 13 to 26 in 2ead4d8

	export function getColorEnabled(): boolean {
	// eslint-disable-next-line @typescript-eslint/no-explicit-any
	const { process, Deno } = globalThis as any
	const isNoColor =
	typeof Deno?.noColor === 'boolean'
	? (Deno.noColor as boolean)
	: process !== undefined
	? // eslint-disable-next-line no-unsafe-optional-chaining
	'NO_COLOR' in process?.env
	: false
	return !isNoColor
	}

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: unjs/unenv#380

[yusukebe]

HI @gpanders

Thank you for raising the issue. There is a similar issue #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: #4301 (comment)

[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: #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