nodejs / node

Node.js JavaScript runtime ✨🐢🚀✨
https://nodejs.org
Other
106.67k stars 29.1k forks source link

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

Open jcbhmr opened 9 months ago

jcbhmr commented 9 months ago

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() https://github.com/nodejs/node/pull/48246 https://github.com/nodejs/node/pull/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 https://github.com/nodejs/node/issues/10292

related deno issues https://github.com/denoland/deno/issues/21427 https://github.com/denoland/deno/issues/21428

related bun issues: https://github.com/oven-sh/bun/issues/7400 https://github.com/oven-sh/bun/issues/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 commented 9 months ago

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

jcbhmr commented 9 months ago

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