-
Notifications
You must be signed in to change notification settings - Fork 30k
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
doc: document console changes as breaking #51503
Conversation
Review requested:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
@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 |
There was a problem hiding this 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
Note that console methods delegate to
|
@targos I see your point Could we change documentation stating that also the changes in |
There was a problem hiding this 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.
when you say a stable output you mean a new API that does something quite similar to |
That would be a possibility, yes. |
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 |
11190d5
to
3ff1a94
Compare
It doesn't change my position. I disagree that |
can you please expand on why the console output should not be considered stable? |
@marco-ippolito you can add the |
If we don't provide semver stability of |
I'm fine with that. |
This comment was marked as outdated.
This comment was marked as outdated.
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. We might find consensus on a less drastic version of the change:
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.
What I would not consider breaking:
I'll take some time to think more on how we can handle this |
I'm not sure I agree with all of these.
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).
Ditto, that should not be breaking (and I'm not sure why it would break end users anyway)
This is just like any other API in Node.js (if we remove/break a function it absolutely should be semver-major) |
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,
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 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. |
3ff1a94
to
65567ac
Compare
65567ac
to
b339570
Compare
Updated with specific behaviors that should be considered stable, rewording, bikeshedding welcome |
b339570
to
3363f18
Compare
|
||
* `console.table` (padding, alignment, etc...). | ||
* changing the color of all console methods. | ||
* changing the signature of all console methods. |
There was a problem hiding this comment.
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?)
There was a problem hiding this comment.
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
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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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>
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 As the output is now colorized by nodejs, this will show up in raw logs as Now, suppose I want to grep the raw logs of the CI jobs which were ingested raw without color stripping.
(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). |
changed my mind |
I think changing the output of console methods such as
console.log
orconsole.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.