Symbols must be stringified explicitly

[jasonkarns]

Apparently (nodejs/node#927 (comment)),
Symbols cannot be implicitly coerced to string.

Template stringification leverages implicit coercion, so we must
stringify propKey before sending it to the template.

Options for explicitly stringifying:

• String(propKey)
• String.prototype.toString(propKey)

Differences:

> String(undefined)
'undefined'
> String.prototype.toString(undefined)
''

In this case, propKey represents the key for the stubbed function. If
the key is undefined, it would be implicitly coerced to "undefined"
as the property key. So String() is the explicit converstion we want.

fixes #300 and perhaps maybe fixes #124 ?

[searls]

Is there a test you could add for this?

[jasonkarns]

tests in progress. Also found 3 other places where this bug can manifest:

• https://github.com/testdouble/testdouble.js/blob/fix-symbol-to-string/src/imitate/create-imitation.js#L14
• https://github.com/testdouble/testdouble.js/blob/fix-symbol-to-string/src/object.js#L29
• https://github.com/testdouble/testdouble.js/blob/fix-symbol-to-string/src/constructor.js#L16

So there are 4 total instances in all. Two of which use . as a dot-notation prefix for the method, and one uses # ala ruby's "method" prefix. Do we care to switch to [Symbol(x)] style function name, as that's the common idiom when using or describing Symbols-as-method-names? Without special handling, a symbol name could end up with a tdFunction name as:

foo.Symbol(bar), or #Symbol(bar), or just .Symbol(bar)

[jasonkarns]

First commit is minimal change to fix the issue (just explicitly stringify the keynames)...

Second commit is an "improvement" to make the key names more idiomatic when stringified. (Symbol keys are wrapped in brackets). Not sure it's worth the extra code, though. Maybe something to revisit after the rewrite? I can nuke the second commit if we'd like to just do the minimal fix for now.

[searls]

Is this done?

[jasonkarns]

Yeah, it's ready if you're fine with the slight change to the stringified output of the function names (second commit)

[searls]

Hey @jasonkarns, apologies that I just got around to reading this (mea culpa). My feeling is that since I'm still mid-rewrite (😭), I don't want to add any superfluous branching anywhere I can avoid it, so I'd really rather merge this fix in without any changes to the stringy output.

Could you update with changes that just make the fix without adding the […] wrapper to symbol func names?

[jasonkarns]

@searls done. there were already separate commits, so i just removed the latter one from the PR

[searls]

Can you please use _.map for this? We're using lodash for all of our enumerable methods everywhere else, and there's a tacit (but not explicit) support for ancient browsers / IE etc that won't have Array.prototype.map defined.

[jasonkarns]

@searls _.map 👍

[searls]

Travis is having some downtime so I'm not going to watch this all day. How about you merge & release yourself (once Travis finishes), using this new guide I just wrote? https://github.com/testdouble/testdouble.js/blob/master/RELEASE.md

[jasonkarns]

Ugh. test:example tests are failing, but they also fail at the merge-base with master :(

Probably gonna rebase to latest master, if that's green...

[searls]

Definitely rebase to latest

…

On Jan 17, 2018, at 1:19 PM, Jason Karns ***@***.***> wrote: Ugh. test:example tests are failing, but they also fail at the merge-base with master :( Probably gonna rebase to latest master, if that's green... — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.