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

doc: document console changes as breaking #51503

Closed

Conversation

marco-ippolito
Copy link
Member

@marco-ippolito marco-ippolito commented Jan 17, 2024

I think changing the output of console methods such as console.log or console.table should be marked as breaking change because it breaks snapshots tests.
While we dont consider modifying error messages breaking, console output is different because it will break ALL snapshot tests using that function.
Refs: #50135

I include a commit that duplicates inspect module in the internal/console, so we can keep the two implementations separated.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/tsc

@nodejs-github-bot nodejs-github-bot added the doc Issues and PRs related to the documentations. label Jan 17, 2024
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@mhdawson
Copy link
Member

@marco-ippolito can you clarify what would be included, do you mean the general format applied to all console output messages or the addition/removal of anthing written to the console?

@marco-ippolito
Copy link
Member Author

@marco-ippolito can you clarify what would be included, do you mean the general format applied to all console output messages or the addition/removal of anthing written to the console?

I mean the general format applied to all console output messages, for example the padding of console.table

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going too fast to change what I think is a fundamental policy around console behavior.
/cc @BridgeAR

@targos
Copy link
Member

targos commented Jan 18, 2024

Note that console methods delegate to util.format/util.inspect and https://nodejs.org/dist/latest-v21.x/docs/api/util.html#utilinspectobject-options

The util.inspect() method returns a string representation of object that is intended for debugging. The output of util.inspect may change at any time and should not be depended upon programmatically.

@marco-ippolito
Copy link
Member Author

@targos I see your point
I think snapshot testing is something widely used (and I immagine something we want to support in our assertion library) and we definitely can't break those test with a patch like happened with #50135.

Could we change documentation stating that also the changes in util.format/util.inspect should be marked as breaking?

Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As @targos outlined, changes to the output format of console are intentionally defined as not breaking.
We should not rely upon that output in case we need a stable output. It should as such also not be used to serialize input for snapshots. We have to define a stable output for things like that.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Jan 18, 2024

As @targos outlined, changes to the output format of console are intentionally defined as not breaking. We should not rely upon that output in case we need a stable output. It should as such also not be used to serialize input for snapshots. We have to define a stable output for things like that.

when you say a stable output you mean a new API that does something quite similar to util.format/util.inspect but its stable?

@BridgeAR
Copy link
Member

when you say a stable output you mean a new API that does something quite similar to util.format/util.inspect but its stable?

That would be a possibility, yes.

@BridgeAR
Copy link
Member

We could for example have an option as well that would output json or something else that would be stable (even though I believe having a separate API that is meant to be stable is better).

@marco-ippolito
Copy link
Member Author

We could for example have an option as well that would output json or something else that would be stable (even though I believe having a separate API that is meant to be stable is better).

I'm having trouble to immagine how that would work for console.methods like table. Maybe we can decouple console output from util.inspect and format? But then we have to maintain 2 apis that do the same things

@marco-ippolito marco-ippolito force-pushed the doc/document-console-output branch 2 times, most recently from 11190d5 to 3ff1a94 Compare March 15, 2024 15:39
@marco-ippolito marco-ippolito added the console Issues and PRs related to the console subsystem. label Mar 15, 2024
@marco-ippolito
Copy link
Member Author

marco-ippolito commented Mar 15, 2024

@BridgeAR @targos I copied the inspect functions to be an internal/console, we have to maintain now 2 separate implementation, but if it affects console we can now consider it a breaking change.

@marco-ippolito marco-ippolito changed the title doc: document console changes as breaking console, doc: document console changes as breaking Mar 15, 2024
@targos
Copy link
Member

targos commented Mar 15, 2024

It doesn't change my position. I disagree that console.log output should be generally stable between versions.

@marco-ippolito
Copy link
Member Author

marco-ippolito commented Mar 15, 2024

It doesn't change my position. I disagree that console.log output should be generally stable between versions.

can you please expand on why the console output should not be considered stable?
I think many of our users rely on it for snapshots, benchmark, immagine the impact of a PR like this #51629 being shipped in a patch

@aduh95
Copy link
Contributor

aduh95 commented Mar 15, 2024

@marco-ippolito you can add the dont-land labels to the other PR regardless of the status of this PR.

@mcollina
Copy link
Member

If we don't provide semver stability of console.table, then we should warn the user to not use it, because it would be impossible to have tests for it. A couple of my modules broke because of an output change that was backported (alignment).

@targos
Copy link
Member

targos commented Mar 18, 2024

I'm fine with that. console is a debugging tool, and is documented as such.

ruyadorno
ruyadorno previously approved these changes Apr 4, 2024
@ruyadorno

This comment was marked as outdated.

@targos
Copy link
Member

targos commented Apr 5, 2024

Here's an example of why I think it isn't realistic or desirable to consider all changes to console output to be breaking: #52283.
Note that what that PR does is not mitigated by having two separate implementations of the util.inspect function.

We might find consensus on a less drastic version of the change:

  • Remove the duplication of util.inspect code. I am honestly surprised that people find okay to copy-paste and start maintaining separately more than 2000 lines of code.
  • Document more specific behaviors as stable. For example:
    • What happens when specific types of arguments (like string) are passed
    • Color handling
    • Table layout

What I'm mostly against here, is the idea of freezing the entire console behavior and considering small improvements in the debugging experience of complex objects as breaking.

@marco-ippolito
Copy link
Member Author

Here's an example of why I think it isn't realistic or desirable to consider all changes to console output to be breaking: #52283.
Note that what that PR does is not mitigated by having two separate implementations of the util.inspect function.

We might find consensus on a less drastic version of the change:

  • Remove the duplication of util.inspect code. I am honestly surprised that people find okay to copy-paste and start maintaining separately more than 2000 lines of code.
  • Document more specific behaviors as stable. For example:
    • What happens when specific types of arguments (like string) are passed
    • Color handling
    • Table layout

What I'm mostly against here, is the idea of freezing the entire console behavior and considering small improvements in the debugging experience of complex objects as breaking.

I understand your concerns, I agree we should document changes that we consider breaking so we dont have to keep 2 implementations.
I also would make CITGM required for console changes.
What I consider breaking:

  • Changes in the table layout, like spacing and alignment
  • Change in the color output
  • Changing function signature.

What I would not consider breaking:

  • Nested object structure

I'll take some time to think more on how we can handle this

@benjamingr
Copy link
Member

What I consider breaking:

I'm not sure I agree with all of these.

Changes in the table layout, like spacing and alignment

We should explicitly document these as a debugging tool and mention the format is not stable across Node.js versions.

I am OK with conceding on the output of some of the methods (like console.log) under some cases (like only one parameter that is a string so formatting happens elsewhere).

Change in the color output

Ditto, that should not be breaking (and I'm not sure why it would break end users anyway)

Changing function signature.

This is just like any other API in Node.js (if we remove/break a function it absolutely should be semver-major)

@unphased
Copy link

I've been reading up in github here to learn about what happened with node.js to make it so that ever since version 20.15, it seems, console.error comes out red and console.warn comes out yellow. I also found even more interesting behaviors today:

  • If you give more than one arg, the colorization doesn't take place. E.g. node -e 'console.error("abc", 1)' won't make anything red.

I appreciate that the behavior is useful in typical situations, but I would like to share how it has a negative impact on any software that already tries to make good use of colors. If we emit strings that have any text colors (such as util.inspect output!) then I would have to do a good deal of bending over backwards to make sure to reach in there to change text color resets to the appropriate value. In this case if I know I am writing some colorized output of util.inspect into console.warn, I would need to do a string replacement of \e[39m with \e[33m to change text color reset escape codes into yellow text color escape codes. This would need to change to become \e31m in the case of console.error.

Since that's pretty awkward, the overall effect for me and possibly a lot of the dev community will likely be to avoid using the console APIs due to the added complexity.

@marco-ippolito
Copy link
Member Author

Updated with specific behaviors that should be considered stable, rewording, bikeshedding welcome

@marco-ippolito marco-ippolito changed the title console, doc: document console changes as breaking doc: document console changes as breaking Jul 1, 2024

* `console.table` (padding, alignment, etc...).
* changing the color of all console methods.
* changing the signature of all console methods.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The signature is as-stable as every other API call and the rest aren't stable (are they?)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I dont know if the signature is considered stable but not the output, if its obvious Im gonna remove it

@ruyadorno ruyadorno dismissed their stale review July 1, 2024 17:36

changed my opinion on this one after chatting about it with @BridgeAR at the last collab summit

The following changes in the output are to be considered breaking changes:

* `console.table` (padding, alignment, etc...).
* changing the color of all console methods.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this is less about the color (visual appearance when connected to a TTY) and more about the contract of outputting raw strings unmodified to the output stream when piped (without adding additional control sequences).

The visual color of the output can change easily as util.inspect() is evolved and doesn't matter for interactive use, but a lot of users rely on console.error(str) or console.log(str) to output str untouched to process.stderr/process.stdout, e.g. for logging JSON in production.

Copy link

@unphased unphased Oct 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to be fair process.stdout/stderr .write() has always been available to use for these needs and additionally gives control over whether a newline is inserted. The pain is around correcting all the code that needs this changed. One might argue it to be superior to make a NEW set of console or other APIs that could gain features like this and maybe steadily enhance those features over time.

I have to assume that this was the whole point of the console interface from the beginning... After all, it has slick stuff like console.table() that everyone knows not to rely on its implementation specific properties.

I am just as guilty myself for opting to use console.error instead of process.stderr.write. This situation seems unavoidable

Co-authored-by: Felix Becker <felix.b@outlook.com>
@jakub-g
Copy link
Contributor

jakub-g commented Sep 13, 2024

I just noticed this change as we upgraded nodejs recently, and I wanted to provide some extra context as someone who works at a monitoring company. (I don't have a strong opinion, just sharing my observations).

Let's say I have a CI job which does console.error('FOO BAR') - either my own code, or third-party.

As the output is now colorized by nodejs, this will show up in raw logs as [31m�[1mFOO BAR�[22m�[39m.

Now, suppose I want to grep the raw logs of the CI jobs which were ingested raw without color stripping.

  • Before node 20.15.0 / 22.2.0 I was able to search for FOO in my query. I could have created a log monitor to alert me on any FOO encountered in logs.
  • Post node 20.15.0 / 22.2.0 I need to refine my query as [1mFOO. If I had monitor on FOO, it would stop working.

(It's an actual known issue with my current CI / monitoring setup. To be honest though, the log ingestion pipeline should be stripping the colors IMO, or tokenizing better).

@marco-ippolito
Copy link
Member Author

changed my mind

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. doc Issues and PRs related to the documentations.
Projects
None yet
Development

Successfully merging this pull request may close these issues.