Support passing Symbol parameters

[Zirak]

Symbols throw a TypeError when being converted to a string by way of concatenation. So the following call:

format('hello', [Symbol('a')])

Would have thrown a TypeError. This commit explicitly passes parameters through the String function, which is the exact behaviour of the '%s formatter.

[davidmarkclements]

thanks for this!

ss is the serializer function, defaulting to JSON.stringify (wrapped in try/catch)

I think we need to check specifically for typeof symbol because:

String(undefined) // => 'undefined'
JSON.stringify(undefined) // => undefined

String(undefined) creates a string containing the word "undefined" whereas JSON.stringify returns the primitive: undefined.

Similarly:

String(Infinity) // => 'Infinity'
JSON.stringify(Infinity) // => 'null'
String(NaN) // => 'NaN'
JSON.stringify(NaN) // => 'null'

[Zirak]

Good catch! It's indeed a tricky thing I haven't noticed, I wonder though if it has an impact since the relevant lines:

    if (x === null || (typeof x !== 'object')) {
      str += ' ' + x
    } else {
      str += ' ' + ss(x)

End up calling ToString anyway. In other words, effectively:

[undefined, null, Infinity, NaN].forEach(x => {
    console.group(x);
    console.log('concat', '' + x);
    console.log('string', String(x));
    console.log('JSON', JSON.stringify(x));
    console.log('JSON concat', '' + JSON.stringify(x));
    console.groupEnd(x);
});

So while concatting and stringifying are not compatible with each other under these circumstances, concating with an empty string and calling String seem to be compatible.

[Zirak]

Having written all that I achieved post-"hit comment" clarity: We already sort of have a "check specifically for typeof symbol" in this case, don't we? The if for hitting the concat case is whether it's not an object, which a symbol isn't.

[davidmarkclements]

oh this is my fault - sorry I misread - I thought String was replacing ss whereas it's an alt for concat. I just did some poorman benchmarks in the REPL on concat vs String vs checking for symbol and then doing concat/String depending on whether it's a Symbol. Roughly speaking, String is 2x slower, but the typeof check with String or concat is 4x slower.

I'm annoyed that Symbol wasn't specced with a toString, because it means that all formatting will now be 2x slower just to support Symbol. However I don't think there's another way around it

[davidmarkclements]

oh.. Symbol was specced with toString.. just refuses to concat 🙄

[davidmarkclements]

ok I can't find a faster way to do this so we'll have to take the hit - thank you! merging

[davidmarkclements]

published as 3.0.3

[Zirak]

Thanks! Mind sharing your benchmarks? It's interesting that calling String has such overhead.