| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
Review requested:
|
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
|
@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? |
Sorry, something went wrong.
I mean the general format applied to all console output messages, for example the padding of console.table |
Sorry, something went wrong.
|
Note that console methods delegate to util.format/util.inspect and https://nodejs.org/dist/latest-v21.x/docs/api/util.html#utilinspectobject-options
|
Sorry, something went wrong.
|
@targos I see your point Could we change documentation stating that also the changes in util.format/util.inspect should be marked as breaking? |
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
when you say a stable output you mean a new API that does something quite similar to util.format/util.inspect but its stable? |
Sorry, something went wrong.
That would be a possibility, yes. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
|
@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. |
Sorry, something went wrong.
|
It doesn't change my position. I disagree that console.log output should be generally stable between versions. |
Sorry, something went wrong.
can you please expand on why the console output should not be considered stable? |
Sorry, something went wrong.
|
@marco-ippolito you can add the dont-land labels to the other PR regardless of the status of this PR. |
Sorry, something went wrong.
|
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). |
Sorry, something went wrong.
|
I'm fine with that. console is a debugging tool, and is documented as such. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
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 |
Sorry, something went wrong.
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) |
Sorry, something went wrong.
|
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:
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. |
Sorry, something went wrong.
|
Updated with specific behaviors that should be considered stable, rewording, bikeshedding welcome |
Sorry, something went wrong.
|
|
||
| * `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.
The signature is as-stable as every other API call and the rest aren't stable (are they?)
Sorry, something went wrong.
There was a problem hiding this comment.
I dont know if the signature is considered stable but not the output, if its obvious Im gonna remove it
Sorry, something went wrong.
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.
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.
Sorry, something went wrong.
There was a problem hiding this comment.
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
Sorry, something went wrong.
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 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.
(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). |
Sorry, something went wrong.
|
changed my mind |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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.