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

Template Literals Incorrect #1065

Closed
jacobp100 opened this issue Mar 23, 2015 · 15 comments
Closed

Template Literals Incorrect #1065

jacobp100 opened this issue Mar 23, 2015 · 15 comments

Comments

@jacobp100
Copy link

@jacobp100 jacobp100 commented Mar 23, 2015

Say I have the object,

let x = {
    toString() { return 'String' },
    valueOf() { return 4; }
};
`${x}`;

Expected result:

'String'

Actual result:

'4'

We should be calling String(x) for all the variables.

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

Do you have a spec citation?

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

Also your example has a ton of syntax errors.

@jacobp100

This comment has been minimized.

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

GetTemplateObject isn't the relevant section, ToString is just being called on index.

12.2.8.5 3 appears to be where the expression is turned into a string. Looks like the current (apparently incorrect) behaviour is the same as the current V8 and Traceur so bugs should potentially be reported on both of those.

Closing this as a wontfix for the time being until I collect more information to evaluate the practicality and perf impact of wrapping all expressions in a String.

@sebmck sebmck closed this Mar 23, 2015
@sebmck sebmck added the wontfix label Mar 23, 2015
@jacobp100

This comment has been minimized.

Copy link
Author

@jacobp100 jacobp100 commented Mar 23, 2015

Why is there a perf impact? x: ${x} should map to 'x: ' + String(x). In terms of perf, what you're doing currently is the equivalent of String(x.valueOf()).

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

Yep, and that's the performance impact I alluded to.

screen shot 2015-03-23 at 9 31 59 pm

@jacobp100

This comment has been minimized.

Copy link
Author

@jacobp100 jacobp100 commented Mar 23, 2015

http://jsperf.com/tostring-vs-appending-empty-string

Alright. There is a perf impact. However, this is clearly not spec compliant. To what level should optimisations allow spec deviations?

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

To what level should optimisations allow spec deviations?

Almost never but seeing as all other implementations follow this behaviour I think it's ok for the timebeing.

@jacobp100

This comment has been minimized.

Copy link
Author

@jacobp100 jacobp100 commented Mar 23, 2015

Firefox is correct.

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

Reopening, need to evaluate and investigate this more.

@sebmck sebmck removed revisit labels Mar 23, 2015
@AluisioASG

This comment has been minimized.

Copy link
Contributor

@AluisioASG AluisioASG commented Mar 23, 2015

Wouldn't this be the case for a spec transformer?

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

@AluisioASG Nope. The spec transformer category is very loosley defined and is just a catch-all for things that don't really belong in another category but are related to spec compliancy.

@sebmck sebmck reopened this Mar 23, 2015
@jacobp100

This comment has been minimized.

Copy link
Author

@jacobp100 jacobp100 commented Mar 23, 2015

https://code.google.com/p/v8/issues/detail?id=3980 for the V8 issue, although V8 is probably not a good metric to use for JS correctness.

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

@jacobp100 Didn't say it was. Just wanted to verify if it was present in the latest and not in the version of iojs I'm using.

@sebmck

This comment has been minimized.

Copy link
Contributor

@sebmck sebmck commented Mar 23, 2015

Relevant Traceur issue: google/traceur-compiler#1836

@sebmck sebmck closed this Mar 23, 2015
@lock lock bot added the outdated label Jul 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 24, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
3 participants
You can’t perform that action at this time.