Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

console.error unexpectedly turns output red #53661

Closed
Jason3S opened this issue Jul 1, 2024 · 22 comments · Fixed by #54677
Closed

console.error unexpectedly turns output red #53661

Jason3S opened this issue Jul 1, 2024 · 22 comments · Fixed by #54677
Labels
console Issues and PRs related to the console subsystem.

Comments

@Jason3S
Copy link

Jason3S commented Jul 1, 2024

Related to PR #51629

I'm a bit surprised that this landed without a link to the discussion. #40361 is a feature request, but doesn't have a real discussion around the pros/cons and how it will impact the nodejs ecosystem.

I would have rather seen color added to the formatting of Errors instead of blanket red/yellow for console.error/console.warn.

I realize that in some way, this PR makes the behavior closer to that in the browser, but browsers do not have CLI applications.

I treat console.log and console.error as separate channels. Traditionally stdout is used for output of the program. It contains the result to be used, think grep. stderr is used for diagnostics, status, and messages to the user while the application is running. Color is not implied.

I would rather that this PR implemented color as a opt in vs trying to figure out how to opt out.

Originally posted by @Jason3S in #51629 (comment)

@RedYetiDev RedYetiDev added the console Issues and PRs related to the console subsystem. label Jul 1, 2024
@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 1, 2024

FWIW I'm +1 on the idea of making the coloring of console opt-in, but that's not my say. (Or just removing it entirely, and having the users handle it themselves)


CC @nodejs/console

@RedYetiDev
Copy link
Member

Related to #51503
Related to #51629
Related to #40361
Related to #53665

@jasnell
Copy link
Member

jasnell commented Jul 1, 2024

I'm +1 on making the colorization opt-in

@ruyadorno
Copy link
Member

I'm -1 on making colorization opt-in.

Color has been opt-out in the runtime for as long as I can remember and I would need a much stronger argument to be convinced otherwise.

Although I don't necessarily agree with the current state of things, the console APIs are properly documented as debugging only: https://nodejs.org/docs/latest/api/console.html#console - and as such their output is not meant to be stable. With that in mind, I would advise cli apps that need stable formatting for their output to write to process.stdout and process.stderr instead of relying on console.log and console.error.

@MoLow
Copy link
Member

MoLow commented Jul 1, 2024

-1 as well. maybe we can find another mitigation.
also, coloring is not new for console log since logged items are passed to util.inspect wich adds colors in various cases

@joyeecheung
Copy link
Member

Shouldn't the color only be default when the stdout/stderr are TTY? I wonder #53665 is more of a bug of the TTY detection not smart enough

@marco-ippolito
Copy link
Member

+1 on making the color opt-in, expect tty maybe

@ruyadorno
Copy link
Member

Shouldn't the color only be default when the stdout/stderr are TTY? I wonder #53665 is more of a bug of the TTY detection not smart enough

I remember testing TTY detection and it was working as expected.

@cjihrig
Copy link
Contributor

cjihrig commented Jul 1, 2024

From what I can tell, the TTY detection is working as expected.

@jakebailey
Copy link

Docs or not, I do think console.error's preexisting behavior was expected by most people and is widely used as a canonical way to "print to stderr". I've seen quite a few projects mentioning breakage due to this change who are hoping for it to be reverted. I have personally written code which assumed that console.log would go to stdout such that that info can be piped, while console.error did not and was treated as plain logging.

@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 1, 2024

FWIW The way I see it, there are few different choices here, such as:

  1. Do nothing
  2. Revert console: colorize console error and warn #51629
  3. Make the special colors opt-in
  4. Make the special colors opt-out

@jasnell
Copy link
Member

jasnell commented Jul 2, 2024

What major versions implement the updated colorization? As a compromise here, I'd suggest that we make sure the change is not backported to anything before 22 if it has been already and we retroactively mark it a semver-major change. Keep the new behavior moving forward with the additional documentation,

@RedYetiDev
Copy link
Member

RedYetiDev commented Jul 2, 2024

What major versions implement the updated colorization?

v20.15.0 (794e450) and v22.2.0 (db76c58)

@Jason3S
Copy link
Author

Jason3S commented Jul 2, 2024

FWIW, I think two different things are getting mixed up.

  1. This issue is about arbitrarily adding red/yellow ANSI color codes to all console.error/.warn strings before sending them to stderr.
  2. Colorization -- I like colorization and would like to keep it.

#40361 seems to have been the motivating factor for the change. If you ignore the title and read the body of the request, it was asking to colorize the Error title red. That could have been accomplished with through updating util.inspect colors to support customization of Errors.

console.log, .warn, and .error represent different logging levels. They do not imply color. Hence the issue #53665.

My vote:

  1. roll back the change.
  2. Fix Colorize console.error output to make it distinguishable #40361 using inspect.
  3. Add the ability to customize the default color for console.log, .warn, and .error.

#51629 was a great POC, it has established that some people want color added and others do not. Even so, I do not believe it is the right long term approach on how to add color to console.error/.warn.

@MoLow
Copy link
Member

MoLow commented Jul 2, 2024

FWIW there is already an option to opt-out from coloring when stderr is tty: FORCE_COLOR=0 node and opt-in when not in tty: FORCE_COLOR=1 node

@imranbarbhuiya
Copy link

imranbarbhuiya commented Jul 2, 2024

FWIW there is already an option to opt-out from coloring when stderr is tty: FORCE_COLOR=0 node and opt-in when not in tty: FORCE_COLOR=1 node

It'll opt-out all types of coloring which might have added intentionally by the user and not just console.warn/error right? So people who only want to opt-out from default coloring of warn/error and keep other coloring can't use it

@MoLow
Copy link
Member

MoLow commented Jul 2, 2024

It'll opt-out all types of coloring which might have added intentionally by the user and not just console.warn/error right? So people who only want to opt-out from default coloring of warn/error and keep other coloring can't use it

that is correct, but why is there an essential difference?

@Jason3S
Copy link
Author

Jason3S commented Jul 2, 2024

It'll opt-out all types of coloring which might have added intentionally by the user and not just console.warn/error right? So people who only want to opt-out from default coloring of warn/error and keep other coloring can't use it

that is correct, but why is there an essential difference?

It is a huge difference.

When I use console.error("Dump: %o", myObject), I'm expecting that in color mode, myObject might be colored. I even have control over its representation: Custom inspection functions on objects.

But, in the current implementation of console.error, I don't have any control over the ANSI Red that is getting injected, it is not possible to remove it from within my own code.

From my perspective, #51629 landed without any formal review of the impact or a design discussion on how to give the best experience. What problem was #51629 solving? It didn't solve #40361, that user was asking for more control over the output of errors. It does not work for that:

Example code:

const error = new Error('This is an error message');
console.error(error);
console.error('This is an error: ', error);
console.error('Just some text.');

Output: Node 20.15.0
image

Here is what #40361 was talking about:

code:

import util from 'node:util';
const error = new Error('This is an error message');
console.error(error);
console.log('We want the Error Title to be Red:');
console.error(util.styleText('red', util.format(error)));

output:
image

I can empathize with #40361, it is impossible to nicely change the format of the Error message.

Why should we keep #51629 when what it does could be easily implemented in user land:

console.error(util.styleText('red', 'My message.'));

@joyeecheung
Copy link
Member

I wonder if packages that get broken are intercepting stderr output by monkey patching process.stderr, which is still expected to work in TTY; in that case it seems the colorization should also check whether the process.stderr has been monkey patched.

@Jason3S
Copy link
Author

Jason3S commented Jul 10, 2024

@RedYetiDev,

What are the next steps to take? Should I make a PR that rolls back that change? So far, I see no benefit in keeping #51629.

@RedYetiDev
Copy link
Member

I'm not sure, some of the collaborators in this issue may know

p-mcgowan pushed a commit to acrontum/oas-codegen that referenced this issue Aug 9, 2024
- added import sorting, proper namespace import resolution
- added formatting options for imports and op id decorator
- on dry run, create files in virtual project and support outputting the
  changes to the terminal
- added support for configuring default service content
- patched console coloured output nodejs/node#53661
- added dts and source map to dev build (stripped in dist files)
- bump codegen parser dep
@RedYetiDev
Copy link
Member

RedYetiDev commented Aug 31, 2024

I've opened a revert #54677. WDYT?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
console Issues and PRs related to the console subsystem.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

10 participants