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

Transpiled template literals behave incorrectly #39744

Closed
lowr opened this issue Jul 25, 2020 · 10 comments · Fixed by #45304
Closed

Transpiled template literals behave incorrectly #39744

lowr opened this issue Jul 25, 2020 · 10 comments · Fixed by #45304
Assignees
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone

Comments

@lowr
Copy link
Contributor

lowr commented Jul 25, 2020

TypeScript Version: 3.9.7, 4.0.0-dev.20200725

Search Terms:

template literals, template strings, ES5, ES3

Code

class C {
    toString() {
        return "toString()";
    }
    valueOf() {
        return "valueOf()";
    }
}

const c = new C;
const s = `${c}`;
console.log(s);

Expected behavior:

According to ES2015 (and later) spec, the result of expressions in template literals are converted to primitives first by calling toString() and then by valueOf() (unless there is [Symbol.toPrimitive](), but I'll just ignore it here as Symbols wouldn't get transpiled). s therefore evaluates to "toString()".

Actual behavior:

s evaluates to "valueOf()".

The above code is compiled to the following when the target is ES5 or ES3. + operator attempts to convert its operands to primitives first by calling valueOf() and then by toString(), which is the opposite of the expected behavior.

"use strict";
var C = /** @class */ (function () {
    function C() {
    }
    C.prototype.toString = function () {
        return "toString()";
    };
    C.prototype.valueOf = function () {
        return "valueOf()";
    };
    return C;
}());
var c = new C;
var s = "" + c;
console.log(s);

FWIW, Babel compiles the above code to this unless loose transformation is enabled. It makes use of String.prototype.concat() and behaves as expected.

Playground Link: Link

Related Issues:

#30239: Although OP's suggestion is not related, some people commenting on the issue seem to confront this issue (like this comment)

@RyanCavanaugh RyanCavanaugh added the Needs Investigation This issue needs a team member to investigate its status. label Jul 29, 2020
@RyanCavanaugh RyanCavanaugh added this to the TypeScript 4.1.0 milestone Jul 29, 2020
@DanielRosenwasser
Copy link
Member

Is this something you ran into in a real-world app?

@lowr
Copy link
Contributor Author

lowr commented Jul 30, 2020

Yes, I had some trouble when I was trying to interact with Discord API using discord.js.

The library defines a class User, which extends another class Base. User overrides toString() so its instances can be serialized into a format Discord API specifies using template literals (see example on the library's documentation) while Base overrides valueOf() presumably for its internal uses. I tried to serialize a User instance with template literals as the example shows, which didn't work as expected since the transpiled code would call valueOf() from the Base class instead of toString() from the User class.

@gilmoreorless
Copy link

As a heads-up, this behaviour will cause problems with the upcoming Temporal proposal. Currently some of the Temporal objects are specified to throw an error in their valueOf() methods.

There's a discussion about TypeScript's transpilation behaviour for template literals at tc39/proposal-temporal#1681

@ljharb
Copy link
Contributor

ljharb commented Aug 2, 2021

This is a spec compliance issue that babel also had to deal with; it’s worth fixing.

It also comes up when interpolating Symbols.

@justingrant
Copy link
Contributor

As a heads-up, this behaviour will cause problems with the upcoming Temporal proposal. Currently some of the Temporal objects are specified to throw an error in their valueOf() methods.

There's a discussion about TypeScript's transpilation behaviour for template literals at tc39/proposal-temporal#1681

Yep. As a champion of the (currently Stage 3) Temporal proposal, this issue is concerning because it will cause divergence in behavior between spec-compliant interpolation of Temporal instances (which will succeed) and TSC-polyfilled interpolation (which will throw).

Given that TS transpilation is sometimes done via babel and sometimes done via TSC, I can see this creating a lot of confusion among developers, esp. those who are less familiar with the bundler/transpilation plumbing that underlies most modern apps.

Also, a common use case for interpolation of Temporal instances will be console.log statements, because logs often have timestamps or durations displayed. It'd be a bummer to have logging code that works great in JS suddenly start crashing when porting that code to TypeScript.

@ljharb
Copy link
Contributor

ljharb commented Aug 3, 2021

Totally agree. Hopefully typescript, which has a tc39 presence, can match the spec before it becomes a problem.

@justingrant
Copy link
Contributor

@andrewbranch would be great to see this in 4.5 final release if possible. The first (flagged) implementations of Temporal are starting to be released in JS engines, e.g. Safari just shipped the first tech preview with a partial Temporal implementation. And production polyfills are maturing. Would be great to have this fixed in TS before there's too much in-the-wild usage. Thanks!

@DanielRosenwasser
Copy link
Member

We generally avoid breaks after beta periods, so we'd have to investigate this for 4.6.

@rbuckton
Copy link
Member

Is this a breaking change though? It seems that the current implementation is a bug, and anyone relying on the non-spec-compliant behavior would have an issue if they switched to --target es2015. We wouldn't be changing documented behavior in any way, so it might be worth taking in the beta timeframe.

@andrewbranch
Copy link
Member

Changing emit is always a risk, but I tend to agree with this being a bug fix, and doesn’t seem like it should be very breaky in practice—it seems like it would be difficult to accidentally take a dependency on the wrong behavior here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Fix Available A PR has been opened for this issue Needs Investigation This issue needs a team member to investigate its status. Rescheduled This issue was previously scheduled to an earlier milestone
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants