Do the entirety of the <template> transform in rollup.

[NullVoxPopuli]

Pending feedback, tho here are the tradeoffs to this approach:

Pros:

• minimize risk of people using the intermediate format (whether intentional or not) if they forget to configure babel
• the intermediate format can be entirely private / self-contained
• less for the user to setup
• 
  Less docs for us to maintain

Cons:

• running babel ourselves is overhead over including the babel plugin in the normal babel process

---

Tasks

• ensure the inline babel transform in the rollup plugin can handle typescript syntax (as well as other syntax) by deferring to the conusmer's babel config (but only transforming the <template> code
• update README, per: Do the entirety of the <template> transform in rollup. #6 (comment)
• figure out a way to test errors or at least document how type errors show up since code-with-<template>-compiled out is what is passed to rollup-plugin-ts

[changeset-bot]

🦋 Changeset detected

Latest commit: c5272ba

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

Name	Type
rollup-plugin-glimmer-template-tag	Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[dfreeman]

High level this seems reasonable to me. As you say, running through Babel's parse/serialize cycle an additional time will have a perf impact, but this definitely lowers the possibility of unexpected interactions with other Rollup plugins encountering the intermediate format.

A couple thoughts/questions:

• Does the advice in the README to set transpileOnly: true for rollup-plugin-ts still apply with this change? What does that mean for declaration generation?
• If we had the ability for Babel to directly parse .gjs/.gts files, how (if at all) would that change the calculus here?

(Side note, not specific to this change: given "tagged templates" already being a general ECMAScript feature, especially in combination with the existence of rendering tests and GlimmerX-style hbs-tagged strings, the name of this plugin really threw me off when I first saw it 😅)

[ynotdraw]

running babel ourselves is overhead over including the babel plugin in the normal babel process

This definitely feels like a win to me if this is the only con! It makes the configuration quite a bit easier to follow by now only having two steps. The tradeoff you're making here, although maybe adding an additional concern for the package, seems as though it'll make it easier on end users which is a huge benefit imo. This is super exciting! 🎉

[NullVoxPopuli]

Does the advice in the README to set transpileOnly: true for rollup-plugin-ts still apply with this change?

it would if the output from the babel plugin is untyped. but there is the tradeoff there with type checking output as opposed to input feels somewhat rude to consumers (esp if types are missing, or break due to any variety of reasons).

What does that mean for declaration generation?

unchanged, rollup-plugin-ts still outputs declarations

If we had the ability for Babel to directly parse .gjs/.gts files, how (if at all) would that change the calculus here?

@ef4 did some exploration here, and it required forking babel's parser due to partially implemented parser-plugin support within babel -- creating a babel-plugin-syntax-glimmer-template-tag would be most ideal, I think, but I don't know how much is involved with that yet.

This is super exciting!

Thanks! There is still more work todo, like I found that my TS tests were not sufficient, because I wasn't actually using TS syntax -- easily resolved, but proves this rollup plugin is def still pre 1.0.

given "tagged templates" already being a general ECMAScript feature, especially in combination with the existence of rendering tests and GlimmerX-style hbs-tagged strings, the name of this plugin really threw me off when I first saw it

yeah --- classic naming struggles 😅 -- I originally thought rollup-plugin-gjs, but also supporting gts would be a smdige misleading. and gjs / gts isn't what exactly is happening in this plugin, because it's just focused on <template>. And <things> are tags, and <templaet> is a template tag .. def some ambiguity with tagged-templates! lol
I was reading through: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/template for help, and nothing stood out -- here all references are "element", which isn't correctly meaningful either in our scenario.

[dfreeman]

Does the advice in the README to set transpileOnly: true for rollup-plugin-ts still apply with this change?

it would if the output from the babel plugin is untyped. but there is the tradeoff there with type checking output as opposed to input feels somewhat rude to consumers (esp if types are missing, or break due to any variety of reasons).

I'm not sure I understand this answer. Are you saying transpileOnly should or should not still be set?

What does that mean for declaration generation?

unchanged, rollup-plugin-ts still outputs declarations

There's a reason I paired this question with the previous one. If your assertion is that typechecking the output of this plugin will produce bad results, then doesn't that mean the generated declarations could have issues (e.g. dropping in any in places where there was a type error)?

[NullVoxPopuli]

Are you saying transpileOnly should or should not still be set?

should not, but this is a personal preference. The work in this PR ideally 🤞 fixes the issue called out in the README where transpileOnly is required and instead makes it optional. Since the output of the template-imports/babel-plugin generates correctly typed code (iirc), folks will hopefully be able to choose how they want their build to behave. The README in this (#6) PR should be updated to reflect this.
The downside that needs to be called out though is that this transpiled code is what is passed to rollup-plugin-ts, so I'll need to do some error tests and see what happens with line-numbers, etc.

There's a reason I paired this question with the previous one.

not everyone has that level of intentionality. 🙃

then doesn't that mean the generated declarations could have issues

It does. But those issues should be caught by lint:types / C.I. / etc.
This is something to call out in the README once I fix some things.

(e.g. dropping in any in places where there was a type error)?

unsure about specifics, I'll double check once some more progress is made.

[NullVoxPopuli]

Looks like I was wrong about:

Since the output of the template-imports/babel-plugin generates correctly typed code (iirc)

Because I got this error with transpileOnly: false (default)

[!] (plugin Typescript) RollupError: Cannot find module '@ember/template-compilation' or its corresponding type declarations.
src/components/ts-class-demo.ts (2:36)

2 import { precompileTemplate } from "@ember/template-compilation";
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

[NullVoxPopuli]

(e.g. dropping in any in places where there was a type error)?

unsure about specifics, I'll double check once some more progress is made.

So when there is a type error, any is not used, and the type error would be seen downstream (ideal!!)

For example, I commented the import of the RouterService in ts-class-demo.gts and this is the declaration output:

// dist/components/ts-class-demo.d.ts
import Component from '@glimmer/component';
declare class TsClassDemo extends Component {
    router: RouterService;
    greeting: string;
}
export { TsClassDemo as default };

So this is ideal, I believe as errors are not hidden.

For an added bonus, when there are no type errors, we get declaration maps!

import Component from '@glimmer/component';
import RouterService from '@ember/routing/router-service';
declare class TsClassDemo extends Component {
    router: RouterService;
    greeting: string;
}
export { TsClassDemo as default };
//# sourceMappingURL=components/ts-class-demo.d.ts.map

[NullVoxPopuli]

While there is a performance hit to doing more work, I think this is acceptable:

ts-demo.gts: 0.468ms
ts-class-demo.gts: 0.142ms

🎉

[dfreeman]

So when there is a type error, any is not used, and the type error would be seen downstream (ideal!!)

Is publishing declarations with a type error in them, instead of flagging the error during publish, ideal?

Edit: I'm realizing I may have misinterpreted what you meant by "downstream." Does that mean "downstream by rollup-plugin-ts when emitting declarations" or "downstream in a consuming application"?

Hopefully in the general case type errors are caught by other tooling, like tsc or glint in CI, but if what we're talking about here is specifically errors that are introduced by the transformation process, then those wouldn't be visible to such a check.

2 import { precompileTemplate } from "@ember/template-compilation";
                                     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

It seems like we should probably have declarations for @ember/template-compilation, since that's an RFCed API. But more to the point, this kind of thing is what I was getting at when I asked about any: in your example where you deleted an import, that produced an error but could still create the overall shape of the type in question.

If we had code that produced export const compiledTemplate = precompileTemplate(...), wouldn't the resulting .d.ts file have const compiledTemplate: any in it, since TS would have no idea what precompileTemplate returns?

[NullVoxPopuli]

Does that mean "downstream by rollup-plugin-ts when emitting declarations" or "downstream in a consuming application"?

gotchya! I consider downstream the consuming application (like in a monorepo is where they'd catch this pre-publish, yet during development) -- upstream (to me), would be rollup-plugin-ts.

Is publishing declarations with a type error in them, instead of flagging the error during publish, ideal?

This is still a good question though -- I did not intend to imply this. I do agree that having some publish protection would be good. Maybe prepack can include lint / lint:types?

It seems like we should probably have declarations for @ember/template-compilation, since that's an RFCed API.

yeah, I can PR that -- is it just a new folder + index.d.ts file at this location: https://github.com/emberjs/ember.js/tree/master/types/preview/%40ember (plus a type-test?)

wouldn't the resulting .d.ts file have const compiledTemplate: any in it, since TS would have no idea what precompileTemplate returns?

I suppose only if folks had noImplicityAny turned off? -- I haven't verified if noImplicitAny also applies to things in node_modules though (I assume it does, but assumptions are dangerous).

[dfreeman]

upstream (to me), would be rollup-plugin-ts.

rollup-plugin-ts is indeed upstream of the consuming app, but both are downstream of this plugin, which is where my confusion came from. Regardless, I think we're on the same page now 👍

I suppose only if folks had noImplicityAny turned off? -- I haven't verified if noImplicitAny also applies to things in node_modules though (I assume it does, but assumptions are dangerous).

noImplicitAny wouldn't come into play here — I'm saying the generated .d.ts file would have an explicit any in it. You can see what I mean if you look at the ".D.TS" tab on the right in this example:

https://www.typescriptlang.org/play?#code/JYWwDg9gTgLgBAbzmKBTAxhcwA2qAqq4OAhjKnAL5wBmUWcA5AAJEBGqUA9OcWagFpM2UjGAQAdowDcAKFmoAHpFhxMEgM7waECHAC8yNMLC4CRMKNQAKAJTSgA

The resulting .d.ts file doesn't have any type errors to be flagged by consumers, and with transpileOnly: true the user won't get an error about the failed import during declaration generation, so the whole thing could go unnoticed.

[NullVoxPopuli]

Thanks for explaining that! I'll get a PR up for the missing types so we can soon flip transpileOnly back to false / omit it.

[ef4]

I think this would be a good mode for some use cases, like v2 addon authors. But I don't think it should be the only mode because this will be unacceptably expensive in large application, for two reasons.

The first is the double babel parse which is discussed above. When we eliminated a second babel parse from ember-auto-import is saved tens of seconds from big apps. This would add that much cost back.

The second is the double HBS parse. The HBS parse itself is a relatively expensive part of every build, and this strategy would need to do it twice. Once to build up the scope bag, and once to actually convert the template to wire format.