Repository navigation
util: change in console.log() output #29539
Description
Activity
- changed the title
[-]util: regression in console.log() output [/-][+]util: change in console.log() output [/+]on Sep 12, 2019 i don't know if this change is a bug or not, but people really shouldn't ever depend on the output of util.format. its only designed for humans.
Reacted by Christopher Allen, Lam Wei Li and Chris@bnb #21624 is independent from this. It changed in #23162. I am not totally happy with this change as I outlined in the PR. I plan on improving things at some point but it's nothing urgent for me.
I have to agree with @devsnek though that it's not ideal to depend on the output of
util.inspectorutil.format(which is used inconsole).If the console.log output changed, I would agree its semver-major, I think its reasonable to depend on the output of
console.log(). But, I don't see that happening across the reportedv10.8.0+:Now using node v10.7.0 (npm v6.1.0) core/node (master $% u=) % node -e 'console.log(1, "hi")' 1 'hi' core/node (master $% u=) % node -e 'console.log(1, "hi there")' 1 'hi there' core/node (master $% u=) % nvm i 10.8 v10.8.0 is already installed. Now using node v10.8.0 (npm v6.2.0) core/node (master $% u=) % node -e 'console.log(1, "hi")' 1 'hi' core/node (master $% u=) % node -e 'console.log(1, "hi there")' 1 'hi there'Am I missing something, @bnb? Did you mistype the version?
Reacted by Anna Henningsen and Christopher AllenI would only call it semver-major if
console.log(string)changed (which hopefully will never change).consoleis a debug tool, not a serialization tool.console is a debug tool,
In the browser, that's true, and some use it like that in node.js, which is very reasonable, but others use it as "node's printf", it even support prinf-like formats.
If I write a reporting tool that does
console.log(n, s)in a loop to write some results to stdout, and another tool parses my stdout, that's not crazy.But like it or not that people should depend on it, the definition of a breaking change is a change that breaks previously running code.There are things that are much more subtle changes in the API, like event ordering, or whether internal, undocumented properties as exposed or not, that we have treated as semver-major, I'd say
console.log()output is compatively much more "public" and deserves as much care towards change.Reacted by Anna Henningsen and Christopher Allenperhaps we need better documentation, but you should be using stuff like
process.stdout.writefor dependable output. afaik the only format we keep stable is single argument as a string (and the printf formatting that comes out of that first string argument, as required by the console spec)Well, that surprises me, and I'm not sure I agree with it. An API with stable output only for certain arg combinations seems a bit cranky, and I'm not seeing any docs distinguishing when a user can and can't depend on the output. All the call patterns seem equally allowed, that some are not equally stable seems worrisome.
Anyhow, at this point, I can't reproduce the bug, and I haven't heard lots of complaints about unstable output, so maybe there is no problem here. However, the "some console.log arg combinations are more equal than others in terms of stability" argument makes me more concerned than comforted.
Reacted by Anna Henningsen and Evan LucasI'm able to reproduce locally on when comparing Node.js v8.x with Node.js v12.x, @devsnek.
Spent some time trying to track down the origin of the actual change since it was obscured by a bunch of layers of indirection. Apologies for the incorrect reference @BridgeAR – did the best I could 😅
On relying on
util.formatandutil.inspect: people are going to useconsole.log()and they're fundamentally going to expect that the API is one that Just Works™. Convincing people otherwise is a losing battle – ask the folks who build IDEs and DevTools, who've been attempting this for years and never finding purchase. Tossing in aconsole.log()is – at this point in JavaScript – the go-to debugger for JavaScript developers. We probably shouldn't be breaking such an API.Reacted by Anna HenningsenFwiw, I’m surprised by this change myself – I always thought that when the first argument was not a string, every argument would be
util.inspect()’ed by default?I also don’t see any disadvantage in marking changes like this as semver-major, if we are convinced that they should happen. It might be a bit annoying for backporting, but the ability of our users to rely on stable results should come first, imo.
I always thought that when the first argument was not a string, every argument would be util.inspect()’ed by default?
That was before my change yes. The string-first case actually ended up in the code othat was doing printf token replacements so that's why it's formatting differed from the others. I don't think that behaviour was ever intended to be that way.
The change appeared in v12.0.0 and is documented on
util.format.v12.0.0 | If the format argument is not a format string, the output string's formatting is no longer dependent on the type of the first argument. This change removes previously present quotes from strings that were being output when the first argument was not a string.
Voting to close this.
I personally think the old behavior is better than the current one and I would rather revert to that.
IMO it would be ideal to inspect all arguments withutil.inspectin case there's more than a single argument (after replacing formatters) instead of checking for the first arguments type. But that would be another breaking change and we would have to check how much impact it would have on the ecosystem.Reacted by Anna HenningsenIf this didn't show up in an 8.x minor, but in 12.0.0 as @silverwind says, then its an allowed breaking change. If we don't like it, then it can be reverted in 13.x if someone is fast. 12.x is stuck with the behaviour, though, for stability.
I like the old behaviour better, I think, but I haven't seen the conversation that led to the change, maybe there was good reason. All of which is tangential to this particular bug report, though.
It does indeed seem like it was a breaking change in v12.0.0. Digging through the git logs it seemed like it'd shipped earlier. If it didn't, my bad – this was a major that shipped in a major.
On the topic of reverting, here's one reason why you'd want to revert:
If
undefinedornullare printed without ANSI colors, it's impossible to tell which is a string and which is a real error.
Reacted by Shobhit Chittora and Lam Wei Liit would be ideal to inspect all arguments with util.inspect in case there's more than a single argument (after replacing formatters) instead of checking for the first arguments type
I prefer consistency. Either format all or no arguments, regardless of argument count or type. Format all is probably too much of a breaking change if
consolestarts to log colors everywhere, thought, so I guess it would be a opt-in. Maybeutil.format.defaultOptions.formatAllor similar.+1 to reverting to the original behavior, regardless of the merits of the change, it's a semver-major difference and shouldn't have gone into 12.x
Reacted by Christopher Allen and Lam Wei Liit's a semver-major difference and shouldn't have gone into 12.x
It was done in 12.0.0, that by definition is a
semver-majorrelease. What's the issue?The original issue says:
This is seemingly a semver major change, despite having shipped in a semver minor release.
But that is not accurate, the change did not ship, at all, in any 10.x release: I tested 10.7, 10.8 -- the one claimed -- and 10.16.3, none have this change.
@bnb wrote
It does indeed seem like it was a breaking change in v12.0.0.
That comment could be misread as a statement that a major landed inappropriately breaking a release line (though I don't think that was the intention). As @silverwind says, we can land
semver-majorin the first release on a new release line, but not later, so the behaviour in 12.x isn't a bug.I can't reproduce anything that shows a break of nodejs' stability commitments.
However, many comments above indicate the change is not desireable.
Should we edit the description so it is accurate?
Or close this and open a new issue requesting the change be reverted in the next semver-major, 13.x?
but you should be using stuff like
process.stdout.writefor dependable outputThis is actually correct, if a bit annoying.
- added a commit that references this issue
on Oct 14, 2019 I don't think there's anything actionable at this point in time. Closing. Can reopen if necessary
In v10.8.0, #21624 shipped.
This is seemingly a semver major change, despite having shipped in a semver minor release.What's the change?
The result of:
changed from:
to:
Here's an example in the repl:
This was surfaced to me by @euantorano from their issue in Nim, in which their tests failed because they were built to expect the old behavior: nim-lang/Nim#12182 (comment)