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

fix(39744): make template literals more spec compliant #45304

Merged
merged 3 commits into from
Oct 13, 2021

Conversation

lowr
Copy link
Contributor

@lowr lowr commented Aug 3, 2021

Fixes #39744

This PR makes the (compiled) template literals more spec compliant. As pointed out in the issue, currently the runtime semantics of the template literals is changed when transpiled. It now makes use of String.prototype.concat() instead of + operator.

I added an evaluation test as I thought it was impossible to test the runtime semantics otherwise. Let me know if there's a more appropriate folder I should place this kind of test in, or even this isn't necessary.

@typescript-bot typescript-bot added the For Milestone Bug PRs that fix a bug with a specific milestone label Aug 3, 2021
@ghost
Copy link

ghost commented Aug 3, 2021

CLA assistant check
All CLA requirements met.

@sandersn sandersn requested a review from rbuckton August 14, 2021 00:16
@sandersn
Copy link
Member

@rbuckton I don't think this is a 4.4 regression, but it might be worth shipping in 4.5 nonetheless.

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Apparently we don't have any tests that use template literals that emit source maps, because I would have expected to see source map changes. Can you add a test that includes source map emit so that we can verify the source map output?

@lowr
Copy link
Contributor Author

lowr commented Oct 12, 2021

Rebased to resolve conflicts and added a test which emits source map. Is this test adequate?

While resolving conflicts, I removed the code added in #46287, but this shouldn't be a problem since tsc wouldn't omit empty "head" string literals with this PR.

Copy link
Member

@rbuckton rbuckton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The only downside is that this increases emit size by 6 characters for every interpolation, but there's really no other way to do this correctly without some increase in emit output for <=ES5.

@andrewbranch andrewbranch merged commit cd0434a into microsoft:main Oct 13, 2021
crisbeto added a commit to crisbeto/angular that referenced this pull request Nov 14, 2021
These changes add support for interpreting `String.prototype.concat` calls. We need to support it, because in TypeScript 4.5 string template expressions are transpiled to `concat` calls, rather than string concatenations. See microsoft/TypeScript#45304.
jessicajaniuk pushed a commit to angular/angular that referenced this pull request Nov 19, 2021
These changes add support for interpreting `String.prototype.concat` calls. We need to support it, because in TypeScript 4.5 string template expressions are transpiled to `concat` calls, rather than string concatenations. See microsoft/TypeScript#45304.

PR Close #44167
jessicajaniuk pushed a commit to angular/angular that referenced this pull request Nov 19, 2021
These changes add support for interpreting `String.prototype.concat` calls. We need to support it, because in TypeScript 4.5 string template expressions are transpiled to `concat` calls, rather than string concatenations. See microsoft/TypeScript#45304.

PR Close #44167
function addTemplateHead(expressions: Expression[], node: TemplateExpression): void {
if (!shouldAddTemplateHead(node)) {
return;
expression = factory.createCallExpression(
Copy link

@leebyron leebyron Dec 16, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious - why create the concat call inside the loop of template spans instead of outside the loop?

In other words, why does a${x}b${y}c emit "a".concat(x, "b").concat(y, "c") instead of emitting "a".concat(x, "b", y, "c")?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@leebyron i believe the observable order of exceptions from ToString calls may require the chained calls; babel had the same change (please double check me)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aha, yes thank you for this.

There's a test included in this change that would break based on that idea

class C {
    counter: number;
    constructor() {
        this.counter = 0;
    }
    get foo() {
        this.counter++;
        return {
            toString: () => this.counter++,
        };
    }
}
const c = new C;
export const output = \`\${c.foo} \${c.foo}\`;

"".concat(c.foo).concat(c.foo) would run: get foo(), toString(), get foo(), toString()

and

"".concat(c.foo, c.foo) would run: get foo(), get foo(), toString(), toString()

The first one is what you'd expect to see from a template interpolation

dimakuba pushed a commit to dimakuba/angular that referenced this pull request Dec 28, 2021
These changes add support for interpreting `String.prototype.concat` calls. We need to support it, because in TypeScript 4.5 string template expressions are transpiled to `concat` calls, rather than string concatenations. See microsoft/TypeScript#45304.

PR Close angular#44167
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
For Milestone Bug PRs that fix a bug with a specific milestone
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Transpiled template literals behave incorrectly
7 participants