Call ToString for template literal placeholders #1838
Conversation
We were not calling ToString correctly for template literal placeholder expressions. This lead to cases where we ended up with calling valueOf instead as required by a + b. Fixes google#1836
|
@caitp I got lazy and reused your tests. |
|
@johnjbarton PTAL |
| @@ -237,19 +233,7 @@ export class TemplateLiteralTransformer extends TempVarTransformer { | |||
| transformTemplateSubstitution(tree) { | |||
| let transformedTree = this.transformAny(tree.expression); | |||
| // Wrap in a paren expression if needed. | |||
| switch (transformedTree.type) { | |||
| case BINARY_EXPRESSION: | |||
There was a problem hiding this comment.
It's not obvious from the description that this change is related.
|
LGTM |
| @@ -283,6 +267,9 @@ export class TemplateLiteralTransformer extends TempVarTransformer { | |||
| binaryExpression = binaryExpression.right; | |||
| } | |||
| let transformedTree = this.transformAny(tree.elements[i]); | |||
| if (element.type === TEMPLATE_SUBSTITUTION) { | |||
| transformedTree = parseExpression `String(${transformedTree})`; | |||
There was a problem hiding this comment.
String() is not really correct (won't throw on symbols) but I dunno if it matters much
There was a problem hiding this comment.
Good point.
Not sure what to do here. Doing a real ToString requires a typeof test. At this point I'm tempted to just not care about this and close this PR.
|
According to 5.1 spec regarding Addition Operator (+)
So thoughts? |
Try |
|
@pflannery |
|
Thanks. It looks like I got my Strings in a twist ;). I see now I misread the |
|
Not going to do this one at the moment. |
We were not calling ToString correctly for template literal
placeholder expressions. This lead to cases where we ended up with
calling valueOf instead as required by a + b.
Fixes #1836