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

Tagged template literals compiled incorrectly, TemplateStringsArray not cached #17956

Closed
justinfagnani opened this issue Aug 21, 2017 · 27 comments
Labels
Bug A bug in TypeScript ES6 Relates to the ES6 Spec Fixed A PR has been merged for this issue

Comments

@justinfagnani
Copy link

TypeScript Version: 2.4.1

Code

let previousStrings;

function test(strings, ...value) {
  if (previousStrings !== undefined && previousStrings !== strings) {
    window.alert('failure');
  }
  previousStrings = strings;
}

function go(n) {
  return test`literal ${n}`;
}

Expected behavior:

According to the ECMAScript spec, every time a template tag is evaluated, the same strings object should be passed as the first argument:

Each TemplateLiteral in the program code of a realm is associated with a unique template object that is used in the evaluation of tagged Templates (12.2.9.6). The template objects are frozen and the same template object is used each time a specific tagged Template is evaluated.

https://tc39.github.io/ecma262/#sec-gettemplateobject

Actual behavior:

The above snippet compiles to:

var previousStrings;
function test(strings) {
    var value = [];
    for (var _i = 1; _i < arguments.length; _i++) {
        value[_i - 1] = arguments[_i];
    }
    if (previousStrings !== undefined && previousStrings !== strings) {
        window.alert('failure');
    }
    previousStrings = strings;
}
function go(n) {
    return (_a = ["literal ", ""], _a.raw = ["literal ", ""], test(_a, n));
    var _a;
}

You can see that _a is going to have a new identity for each call of go().

FWIW, Babel compiles this correctly:

'use strict';

var _templateObject = _taggedTemplateLiteral(['literal ', ''], ['literal ', '']);

function _taggedTemplateLiteral(strings, raw) { return Object.freeze(Object.defineProperties(strings, { raw: { value: Object.freeze(raw) } })); }

var previousStrings = void 0;

function test(strings) {
  if (previousStrings !== undefined && previousStrings !== strings) {
    window.alert('failure');
  }
  previousStrings = strings;
}

function go(n) {
  return test(_templateObject, n);
}

This is breaking lit-html when used in TypeScript: lit/lit#58

@mhegazy mhegazy added Bug A bug in TypeScript ES6 Relates to the ES6 Spec labels Aug 22, 2017
@PatrickJS
Copy link

PatrickJS commented Aug 26, 2017

Is there a workaround for typescript to do this correctly or do we have to wait for a patch? For future facing JavaScript frameworks moving away from virtual-dom (hyperHTML, lit-html) this is a bit of a problem (trying to avoid memory leaks)

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 26, 2017

For now I'd rather wait on the most recent discussions (https://github.com/rwaldron/tc39-notes/blob/master/es8/2017-07/jul-27.md and tc39/ecma262#840) to get settled out first before we revisit the implementation.

Specifically, it's not clear whether the following behavior will hold:

function id(x) { return x }

id `` === id `` // true

@justinfagnani
Copy link
Author

@DanielRosenwasser whether or not equal literals from separate source locations remain identical might be up in the air, but this should be true:

let t = () => id``;

t() === t(); // true

That it isn't is a pretty critical bug that prevents using TemplateStringsArray as map key. Until this is fixed I need to tell my TypeScript users to target esnext, then compile with Babel.

The incremental fix is to hoist the TemplateStringsArray declaration to the top-level:

var _a = ["literal ", ""];
_a.raw = ["literal ", ""];

function go(n) {
    return test(_a, n);
}

@PatrickJS
Copy link

PatrickJS commented Aug 27, 2017

for anyone using template literals for example hyperHTML, vyperHTML, or lit-html you can use this feature detection

const shouldCacheTemplates = ((_t) => _t() === _t())(() => ((s: any) => s)``);
const cacheTemplates = new Map<string, TemplateStringsArray>();


function tagged(strings: TemplateStringsArray, ...values: any[]) {
  if (shouldCacheTemplates) {
    let key = strings.join('{{typescriptProblems}}');
    let _strings = cacheTemplates.get(key)
    if (_strings === undefined) {
      cacheTemplates.set(key, strings)
    } else {
      strings = _strings
    }
  }

}

@PatrickJS
Copy link

do you think a typescript plugin could be used as a workaround?
https://github.com/Microsoft/TypeScript/wiki/Writing-a-Language-Service-Plugin

@justinfagnani
Copy link
Author

@gdi2290 A plugin would need to change its behavior depending on the target. When the target is es2015+, there's no bug. Only when the target is <= ES5 is there a problem. That may be possible though, I haven't written a plugin yet.

@WebReflection
Copy link

@DanielRosenwasser is there an ETA for this bug to be fixed?

If not, is there a way from JS side to feature-detect TypeScript env is running?

I already fallback for old versions of FireFox that were incompatible with ES2015 standard, but being template literals feature-undetectable I need to use some ugly hack to know it's TypeScript and fallback with the solution I know it works already (per realm).

Thanks in advance for any advice.

@DanielRosenwasser DanielRosenwasser added this to the TypeScript 2.6 milestone Sep 6, 2017
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 6, 2017

@WebReflection Technically you can check whether the raw property is enumerable. TypeScript's is enumerable, whereas the one in ES2015 runtimes should be non-enumerable. But I hesitate from recommending you take a dependency on that since we still intend on supporting the feature in ES3 runtimes.

I already have a prototype working. We're thinking about 2.6, but are concerned with trying to do the more standards-compliant thing but still coming short with respect to having globally cached template literal objects.

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

Thanks @DanielRosenwasser, but it's still undetectable to me, unless I put that check (even once) in the entry point of my logic.

It's a great hint, but it's too specific. What I really would like to know, is if there's any (hidden?) global/scoped TypeScript leak I can detect upfront, like a typeof _classCreator === 'function' within my code, in case it gets "compiled" with TypeScript projects.

@justinfagnani
Copy link
Author

In lit-html we check for this bug by comparing the results of two calls to a tag:

const envCachesTemplates = ((t) => t() === t())(() => ((s) => s)``);

The expanded version is a little easier to read:

const t = () => ((s) => s)``;
const envCachesTemplates = t() === t();

@DanielRosenwasser
Copy link
Member

@justinfagnani but that's more of a thing that client code needs to opt into, isn't it? In other words, wouldn't users of lit-html need to inform lit-html of the current behavior rather than lit-html discovering it on its own?

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

@justinfagnani you are using code that's going to be transpiled to address a bug caused by transpilers.
I cannot use a single backtick string on my code because it runs on every browser already.

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

@DanielRosenwasser lit-html uses TypeScript already so it's inevitably trusting the transpiling env it's running itself. My case is different. I don't need/want/use transpilers at all (you know, use the platform and target every browser and stuff), and even if the surrounding code uses transpilers, my library would be transpiler resistant and it will work there no matter what.

However, I need to figure out non-standard behaviors upfront, and using a transpiled feature to do that is not really an option because you can include my library as <script> resource right away, and it should be compatible with whatever env is there.

If I understand correctly, lit-html, will fallback with hacks for every browser once it's used transpiled because that check can be resolved only after TypeScript transpilation.

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

To add more content, hyperHTML can be embedded on any website now through unpkg.com/hyperhtml@latest. You cannot do the same with lit-html unless you are targeting already 100% compliant ES2015 browser (only WebKit these days?).

So lit-html trusts transpilation by default, hence it can do feature detections through the resulting code, but that's not possible for my library.

As summary, is there really nothing TypeScript leaks beside template literals raw enumerability ?

I might go and find out myself 😄

@WebReflection
Copy link

so, typeof __extends looks safe enough as assumption, isn't it?

@DanielRosenwasser
Copy link
Member

so, typeof __extends looks safe enough as assumption, isn't it?

If you believe all of your users will be creating subclasses, yes, but that's probably not always the case.

@WebReflection
Copy link

is there anything that always leak ?

@WebReflection
Copy link

Found it. Please let me know when this bug is solved. Until then, I'll slightly penalize TypeScript transpiled code, degrading them like Firefox 54 or lower.

function isTypeScript() {
  var k, v;
  for (k in window) {
    if (k.charAt(0) === '_') {
      v = window[k];
      if (Array.isArray(v) && 'raw' in v) {
        return true;
      }
    }
  }
  return false;
}

@WebReflection
Copy link

actually I have doubts that would happen on the global scope if there's something like

(function(){}``);

I think the pkayground is ling to me ... thoughts? Thanks

@PatrickJS
Copy link

PatrickJS commented Sep 6, 2017

@WebReflection
I think it's better to have a feature detection for your usecase rather than banning TypeScript and having to patch it in the future when it's fixed. This means detecting if the reference is the same.

const shouldCachesTemplates = ((t) => t() === t())(() => ((s) => s)``);

TypeScript might take 2+ years to fix this because their original template codegen wasn't up to spec (at the time). It's at a point where introducing a "fix" might be worse if the spec changed.
Here's an example of a long lived issue dependent on TC39 #16

The thing that's blocking the TC39 is something that hyperHTML and lit-html can benefit from. The edge case is when a WeakMap is used, rather than a Map, in order for the template string to be garbage collected. Right now if you use a WeakMap to store the template string it's kept in-memory and is never collected (kind of defeating the purpose of using WeakMap). For hyperHTML, this isn't an issue since the design is meant use the templates as a key. If hyperHTML were to allow certain templates to only wire once (they never rerender for example a footer render etc) then it would be better to release the template string from memory.
(I guess with that example you can just never store it given another codepath)

htmlOnce`yolo ${ content }`

hyperHTML can benefit by using the same codepath html rather than creating a new one via htmlOnce

@WebReflection
Copy link

@gdi2290 which part of I cannot use template literals in my code wasn't clear?

@WebReflection
Copy link

@gdi2290 just in case it's stil not clear: I cannot feature detect template literals. This breaks my browsers:

//                                                      see this ? ↓↓
const shouldCachesTemplates = ((t) => t() === t())(() => ((s) => s)``);

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

@gdi2290 moreover

The thing that's blocking the TC39 is something that hyperHTML and lit-html can benefit from.

I'm looking forward to that, meanwhile I have production code to offer so I need to make it work.

I'm implementing right now the one-off hack suggested by @DanielRosenwasser which seems to be reliable and Babel proof.

Once again, hyperHTML can be used as <script src="hyperHTML"> without needing tooling or anything else and it's going to work down to IE9. It's code cannot possibly feature detect backticks, but thanks for the rest of the thoughts.

@justinfagnani
Copy link
Author

@DanielRosenwasser since lit-html is distributed, for the moment, only as standard ES2015, the presumption with this part of the fix, is that if the app is running in ES5, the consumer code and lit-html were compiled with the same compiler. On the Polymer project we distribute all our code as ES2015 and let applications compile as needed. Internal to Google, all TypeScript code is compiled at app build time with the same version. This detection should also work for uncompiled code running on browsers with buggy template literal implementations.

It's true that this particular detection won't work if lit-html is either uncompiled or compiled with a spec-compliant compiler, and the client code is compiled with a buggy compiler. There are other checks we can do, like you suggested.

@WebReflection lit-html's use of TypeScript internally has absolutely nothing to with how most users will consume it. lit-html is distributed as standard ES2015, and we do not "trust" TypeScript's downlevel compilation because we don't use it.

@WebReflection
Copy link

WebReflection commented Sep 6, 2017

@justinfagnani my point is what you wrote already to Daniel. lit-html trusts inevitably transpilations at its core, because it's written in TypeScript, which is not a standard, and it's distributed as ES2015, which is 80% of the time transpiled these days.

hyperHTML is written in ES5, and that's about the end of the story. Any consumer, every browser on the planet.

This is a huge different in terms of runtime, transpiler potentially aware, features detection.

hyperHTML can be a script tag on top of any new and old project, it doesn't have tooling as core dependency for today web.

On top of that, I don't care much, really, I just want to make my library transpiler proof, hence I need to know what these "beasts" do with the JavaScript code they are targeting.

@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Sep 7, 2017

When either of you get the chance, let me know how the branch at #18300 works.

WebReflection added a commit to WebReflection/hyperHTML that referenced this issue Sep 7, 2017
As discussed in bug microsoft/TypeScript#17956
TypeScript does a bad job compared to Babel when it comes
to transform tagget template literals.

Apparently users are ignoring this issue
#99
so this version of hyperHTML should fix it.
@WebReflection
Copy link

Hi @DanielRosenwasser

FYI the detection hack you proposed has been implemented and published.

If TypeScript intend to set raw as not enumerable without fixing issues with template literals please do let me know. Thank You!

@DanielRosenwasser DanielRosenwasser added the Fixed A PR has been merged for this issue label Oct 3, 2017
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A bug in TypeScript ES6 Relates to the ES6 Spec Fixed A PR has been merged for this issue
Projects
None yet
Development

No branches or pull requests

5 participants