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

Node.js doesn't properly implement %d %i %s console.log() format specifiers #51006

Open
jcbhmr opened this issue Dec 1, 2023 · 2 comments
Open

Comments

@jcbhmr
Copy link
Contributor

jcbhmr commented Dec 1, 2023

according to the console spec: https://console.spec.whatwg.org/#formatter

2.2.1. Summary of formatting specifiers

The following is an informative summary of the format specifiers processed by the above algorithm.

Specifier Purpose Type Conversion
%s Element which substitutes is converted to a string %String%(element)
%d or %i Element which substitutes is converted to an integer %parseInt%(element, 10)
%f Element which substitutes is converted to a float %parseFloat%(element, 10)
%o Element is displayed with optimally useful formatting n/a
%O Element is displayed with generic JavaScript object formatting n/a
%c Applies provided CSS n/a

but node.js doesn't follow that exactly. here's the current console.log() backing inspect() code: https://github.com/chenyuyang2022/node/blob/fix/console-log/lib/internal/util/inspect.js#L2196 that's used when running console.log().

specifically:
%s specifier doesn't do String(tempArg) #50909
%d, %i doesn't do parseInt() #48246 #23321

Why is this not good? causes interop issues with other js runtimes. Conforming to the standard is good! Here's a specific example that demonstrates how a problem may arise:

jcbhmr@PIG-2016:~/Documents$ node -e 'console.log("%d", process.hrtime.bigint())' | node -e 'console.log(Number(fs.readF
ileSync(0, "utf8")))'
NaN
jcbhmr@PIG-2016:~/Documents$ bun -e 'console.log("%d", process.hrtime.bigint())' | bun -e 'console.log(Number(fs.readFil
eSync(0, "utf8")))'
16716400

notice how Bun works! normally parseInt() and by extension %d returns a number and that number can be rehydrated via Number(). this doesn't work with the n suffix from bigints. (yes i know; bigint => number is lossy.)

Here's another example:

jcbhmr@PIG-2016:~/Documents$ cat testconsole.js
class Frac {
  constructor(num, den) {
    this.num = num
    this.den = den
  }
  [Symbol.toPrimitive]() {
    return this.num / this.den
  }
}
console.log("%f", new Frac(1, 2))
console.log("%f", 0.5)
jcbhmr@PIG-2016:~/Documents$ node testconsole.js
0.5
0.5
jcbhmr@PIG-2016:~/Documents$ bun testconsole.js
[Number (Frac): 0.5]
0.5
jcbhmr@PIG-2016:~/Documents$ deno run testconsole.js
NaN
0.5

This WORKS PROPERLY right now in Node.js, Chrome, Firefox... but fails in Bun and Deno. That's a divergence! Not good for isomorphic interoperable code.

Here's another example of Node.js erroneous handling: from #50909

jcbhmr@PIG-2016:~/Documents$ node -e 'console.log("%s", { [Symbol.toPrimitive]: () => "hello" })'
{ [Symbol(Symbol.toPrimitive)]: [Function: [Symbol.toPrimitive]] }
jcbhmr@PIG-2016:~/Documents$ bun -e 'console.log("%s", { [Symbol.toPrimitive]: () => "hello" })'
hello
jcbhmr@PIG-2016:~/Documents$ deno eval 'console.log("%s", { [Symbol.toPrimitive]: () => "hello" })'
hello

in another case, if i were using %s to try and be "no i actually don't want inspect()-style formatting, just coerce things to a string" then this would diverge across implementations.

jcbhmr@PIG-2016:~/Documents$ node -e 'console.log("%s", {})'
{}
jcbhmr@PIG-2016:~/Documents$ bun -e 'console.log("%s", {})'
[object Object]
jcbhmr@PIG-2016:~/Documents$ deno eval 'console.log("%s", {})'
[object Object]

why would i want to coerce everything to a string when logging? maybe i replaced Object.prototype.toString or some other weird thing. who knows. again, it's just Node.js that does something different which creates edge cases like if (isNode) { } else { }. not ideal.

related #10292

related deno issues
denoland/deno#21427
denoland/deno#21428

related bun issues:
oven-sh/bun#7400
oven-sh/bun#7401

related firefox issue:
https://bugzilla.mozilla.org/show_bug.cgi?id=1846606

i am of the opinion that achieving better interoperability on format specifiers is a good idea. to that end, i suggest trying to conform to the console spec. https://console.spec.whatwg.org/#formatter

sorry if this has been discussed in individual issues. this is intended to be a push not for fixing a specific case like the above examples, but instead to push to fix all of them by conforming to the spec. i understand that this is a breaking change and that there is some pushback.

@juanarbol
Copy link
Member

Not sure if this is directly implemented by V8 instead. Can anyone from @nodejs/v8 confirm?

@jcbhmr
Copy link
Contributor Author

jcbhmr commented Dec 6, 2023

I don't think console nor the formatting logic is native to V8. I think it's here:
https://github.com/nodejs/node/blob/main/lib/internal/console/constructor.js for the console.log() function impls which then calls...
https://github.com/nodejs/node/blob/main/lib/internal/util/inspect.js#L2179 the format() function from the node:util which actually does the %s %i %d %f stuff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants