Add Typescript support #42

simonihmig · 2022-07-06T15:26:03Z

Introduces the --typescript flag, closing #10.

Some notes:

the rollup and babel config is for now what I used so far. Will update to agreed upon best practices as soon as Add typecsript sample rollup config + exploration of different plugins' side-effects embroider#1099 is approved and merged! Therefore still a draft...
ESLint is configured for TS. Again what I have been using commonly.
tsconfig.json is using the setup that ember-cli-typescript is also generating recently, delegating to @tsconfig/ember.
this also sets up the infrastructure for the addon to use and expose Glint types. Which isn't official, and hasn't been RFCed. But I figured people using the latest stuff here w/ TS will probably want it? 🤔 Can remove, if that seems too controversial. But I love it 😍. Or add an additional --glint flag? Related: https://typed-ember.gitbook.io/glint/using-glint/ember/authoring-addons and Support shipping Glint types with v2 addons typed-ember/glint#338
the test-app generation delegates the --typescript flag to the official app blueprint (which in the latest canary version does support that as well). If the used Ember CLI version is not one that support it, then a warning message is shown, and the user has to manually setup TS in the test app.

To Do

files/__addonLocation__/rollup.config.__rollupExt__

NullVoxPopuli · 2022-07-06T15:50:42Z

files/__addonLocation__/rollup.config.__rollupExt__

+<% if (typescript) { %>    // compile TypeScript to latest JavaScript, including Babel transpilation
+    typescript({
+      transpiler: 'babel',
+      browserslist: false,


browserslist false adds extra transpilation -- I was unable to determine why, but "last 1 firefox versions, last 2 chrome versions" has babel doing the least work

Yeah, I remember this discussion in your other PR. As I said in the PR description, I just added what worked for me, for now. When your PR is merged, I will update all of this! So let's not duplicate the same discussion here! 😉

For the record: as I already stated in the related embroider-build/embroider#1099, I think the config here is right (browserslist: false) as documented by rollup-plugin-ts, it's just that the latter seems to not behave correctly here. See the issue I filed here: wessberg/rollup-plugin-ts#189

Given that this does not break anything, it just produces code that is not as optimized as it could be, I would think to keep this as is now, and not expose workarounds to created addons that later need to get rolled back causing churn for maintainers, and rather rely on getting this fixed upstream eventually!

NullVoxPopuli · 2022-07-06T15:51:33Z

files/__addonLocation__/rollup.config.__rollupExt__

+    typescript({
+      transpiler: 'babel',
+      browserslist: false,
+      transpileOnly: false,


I think we should add declarationMap here as well -- I use this config: https://github.com/NullVoxPopuli/ember-resources/blob/main/ember-resources/config/rollup.config.js#L52

NullVoxPopuli · 2022-07-06T15:52:35Z

files/__addonLocation__/types/glint.d.ts

@@ -0,0 +1,10 @@
+import '@glint/environment-ember-loose';


should this have been committed?

looks similar to https://github.com/embroider-build/addon-blueprint/pull/42/files#diff-09f964d563c8ddb4545dacec9afac59cb9370f1c25ea10a1da7bc5b957884aea

Yes, this is intentional. At least that's what worked well for me.

So this file is for making Glint aware of what your own addon consumes. Say this addon depends on your ember-popperjs. With classic templates, invoking <PopperJS> would make Glint fail, as it doesn't know what PopperJS is. So you would import the component here, and add its type to the registry.

The other file src/glint.ts is about making Glint aware of what your addon exposes. So the app would import this file (which is published on npm!), and have all the components etc. added to the app's registry.

This is what we came up with here: typed-ember/glint#303. And which is documented here: https://typed-ember.gitbook.io/glint/using-glint/ember/authoring-addons#publishing-addons-with-glint-types

should we link to that discussion in this glint.d.ts file?
it feels "non-standard" to me, but idk if that's because Glint is new.
even so, it's a hidden convention (no tooling/linting, to enforce, etc) 🤔

I think this relates to the question above about how we want to handle addon-only unpublished type declarations and making it as clear as possible to authors/readers the role they play.

I'm now 100% in camp "TS addons should have a types directory for local unpublished types"

index.js

files/__addonLocation__/tsconfig.json

NullVoxPopuli · 2022-07-06T15:55:11Z

files/__addonLocation__/tsconfig.json

+    "paths": {
+      // No paths, no absolute imports, only node_modules imports allowed
+      // But fallback for type-overrides and such
+      "*": ["types/*"]


do we want to allow this for the addon?

consuming projects wouldn't know about these types and could error as a result

Not sure what you mean here?

I took it from your comment here actually 😏

hmm I'll investigate

I think v2 addons should aim to avoid needing paths config at all. There's always declare module if absolutely necessary, but in general I don't think we want to make it easier for addon authors to write local type declarations that will break things when their consumers don't have them.

it turns out this is needed if a v2 addon wants the cached-decorator types.
the cached-decorator types go in the types directory, but we don''t want to provide those types, because they could conflict with the consumer's types for @cached

What's special about @cached that makes it require a paths entry?

Isn't *: [types/*] how you tell ts to check every file in that folder for type defs?

'*': ['types/*'] says "when any import fails, see if the mapping here produces the path to a module you can load instead, and if so, treat that as implicitly being the contents of the thing you were trying to look up"

include: ['types'] says "consider everything under types/ implicitly in scope as part of this project, whether it's imported or not" (roughly equivalent to specifying entrypoints for a bundler)

A declare module block anywhere covered by a project's include will be taken into account, even without path mapping shenanigans.

Given (2) and (3), anything you're implicitly relying on paths for can be wrapped in declare module instead. You can still align your declarations on disk 1:1 with the module layout if you want, but declare module also gives you the flexibility to e.g. group together declarations for a package in a single .d.ts file if you want.

excellent! thanks for explaining those things!

NullVoxPopuli · 2022-07-06T15:56:00Z

files/__addonLocation__/tsconfig.json

+    "types/**/*"
+  ],
+  "glint": {
+    "environment": "ember-loose",


why loose?
porque no template-imports?

tho 🤔 maybe that should be done as a part of #41

Yeah, I thought to go with the more "classic" setup first.
Otherwise we would have to add additional dependencies etc., so wasn't sure if that's not too experimental yet as a default? 🤷‍♂️

As far as I know it isn't even possible to use FCCTs in v2 addons yet, so including the template-imports environment here would be a pretty mean trick to play on authors 😜

NullVoxPopuli · 2022-08-09T03:07:32Z

files/__addonLocation__/babel.config.json

@@ -1,5 +1,6 @@
 {
-  "plugins": [
+<% if (typescript) { %>  "presets": [["@babel/preset-typescript"]],
+<% } %>  "plugins": [
    "@embroider/addon-dev/template-colocation-plugin",


before I forget, I think we'll want to change the default settings:

[ resolve('@babel/plugin-transform-typescript'), { allowDeclareFields: true, onlyRemoveTypeImports: true, // Default enums are IIFEs optimizeConstEnums: true, }, ],

This is what I use over here: https://github.com/NullVoxPopuli/ember-popperjs/blob/main/ember-popperjs/babel.config.cjs#L8

and cc @chriskrycho

@NullVoxPopuli A few questions:

Why plugin-transform-typescript instead of preset-typescript as Simon currently has?

Why resolve()?

Why default users to onlyRemoveTypeImports and optimizeConstEnums? I think both of those choices can be reasonable ones for an author to intentionally make, but I'm wary of diverging from default behavior in the blueprint unless there's a super compelling reason to do so.

Re: the third bullet, allowDeclareFields is a perfect example of something I do think is worth overriding for users by default in the blueprint: idiomatic Ember-in-TypeScript uses declare for decorated framework-managed fields, so folks can run into trouble pretty quickly if it's not set. (Also worth noting that Babel only defaults that one to false for backcompat reasons and is planning to default it to true in v8, if/when that day comes)

Why plugin-transform-typescript instead of preset-typescript as Simon currently has?

I think we need both -- preset-typescript for all TS things (and a general indicator to users, that we're using "TS stuff" for compilation -- and then the transform plugin to override declare fields.

Why resolve()?

catches typos with better errors at build time -- this is a personal preference, and it's a non issue if you just don't misspell things -- though maybe the error reporting has gotten better.

Why default users to onlyRemoveTypeImports and optimizeConstEnums?

for optimizeConstEnums, I don't understand why anyone would want the previous behavior of an IIFE per-enum.

For onlyRemoveTypeImports, yeah, I think this is an artifact of a darker age in my babel config's history -- we don't need this one (and it implies confusing behavior, considering what the rollup stuff is doing)

I think we need both

The preset sets up the transform for you, and accepts the same configuration

catches typos with better errors at build time

We aren't using resolve anywhere else in that file (and in fact we can't—it's .json). That feels like an unrelated change to this PR if you want to propose it.

for optimizeConstEnums, I don't understand why anyone would want the previous behavior of an IIFE per-enum

Sure, but const enum is somewhat of a power user feature with a lot of documented caveats and downsides (not to mention the "just use unions" crowd), so my argument here is that diverging from default behavior and introducing extra boilerplate config for maintainers isn't worth the tradeoff.

Someone who knows they have a highly performance-sensitive situation and that it means they want to use const enum is also likely to be able to determine that they want to change Babel's transpilation behavior as well. (Also, if you're in such a situation, actual consts + a union type will be more performant than optimizeConstEnums's output, since it skips an object field traversal)

The preset sets up the transform for you, and accepts the same configuration

Oh, nice! that would make the config even less verbose!

That feels like an unrelated change to this PR if you want to propose it.

yeah, I was copy pasting from one of my own projects -- I had hoped that we could implicitly convert between all the babel config formats in the split of the comment, rather than the details of the comment 😅

so my argument here is that diverging from default behavior and introducing extra boilerplate config for maintainers isn't worth the tradeoff.

fair enough 💯

Windvis · 2022-10-14T17:50:05Z

files/__addonLocation__/.eslintrc.js

@@ -2,7 +2,7 @@

 module.exports = {
  root: true,
-  parser: '@babel/eslint-parser',
+  parser: '<%= typescript ? '@typescript-eslint/parser' : '@babel/eslint-parser' %>',
  parserOptions: {


I think this ember-cli issue is also relevant here: ember-cli/ember-cli#10046

Since both parsers have different options I think the parserOptions should also be applied conditionally?

I was looking into fixing that in ember-cli but I'm not sure which options are needed exactly. I found this: https://typescript-eslint.io/docs/linting/typed-linting which sounds nice?

Agreed here. Eslint configs need to make heavy use of overrides, because we have a mix of node (cjs), node (esm), and browser (esm, js, ts).

Overrides are way easier to extend, change, target specific files, folders, etc.

It's also how my eslint-configs package can simultaneously support app, addon, v2 addon, js, ts, all from the same config

Will wait for ember-cli/ember-cli#10054 to get merged before applying the same fix here.

I just applied the changes as in ember-cli/ember-cli#10054. Think that should be good.

dfreeman

Thank you for all your hard work to drive this to completion @simonihmig!

I've left a few comments inline—happy to discuss out-of-band as well on Discord or a call at some point if sync communication is easier for any of it 🙂

dfreeman · 2022-10-17T11:28:38Z

files/__addonLocation__/babel.config.json

@@ -1,5 +1,6 @@
 {
-  "plugins": [
+<% if (typescript) { %>  "presets": [["@babel/preset-typescript"]],
+<% } %>  "plugins": [
    "@embroider/addon-dev/template-colocation-plugin",


@NullVoxPopuli A few questions:

Why plugin-transform-typescript instead of preset-typescript as Simon currently has?

Why resolve()?

Why default users to onlyRemoveTypeImports and optimizeConstEnums? I think both of those choices can be reasonable ones for an author to intentionally make, but I'm wary of diverging from default behavior in the blueprint unless there's a super compelling reason to do so.

Re: the third bullet, allowDeclareFields is a perfect example of something I do think is worth overriding for users by default in the blueprint: idiomatic Ember-in-TypeScript uses declare for decorated framework-managed fields, so folks can run into trouble pretty quickly if it's not set. (Also worth noting that Babel only defaults that one to false for backcompat reasons and is planning to default it to true in v8, if/when that day comes)

dfreeman · 2022-10-17T11:36:59Z

files/__addonLocation__/package.json

+    "@glint/core": "^0.9.1",
+    "@glint/environment-ember-loose": "^0.9.1",


@chriskrycho @jamescdavis What do you think; are we ready for primetime?

I could see an argument for waiting for Glint 1.0 (which we're getting close to, but is still blocked on one particularly large internal refactor that I'll need to be able to set aside a nontrivial block of time for).
On the other hand, the reality is that folks are already releasing Glint-enabled addons today, so you could argue we might as well make that as easy as possible.

Well, I don't really think this makes it into "prime time". 😅 At least until this blueprint replaces the official v1 addon blueprint eventually. So as this blueprint is still more on the experimental side of things, I wouldn't mind this to use other more experimental things (pre-stable Glint). 🤷

Haha that's fair—it's certainly true that anyone using the v2 blueprint today knows they're opting into something that isn't guaranteed to be 100% baked yet.

This will just be the first time (that I know of!) that anyone will find themselves set up with Glint without having gone out of their way to read the docs and decide it's something they want. This blueprint feels to me like a reasonable place to take that step, but I did want to call it out and make sure Chris and James are on the same page

dfreeman · 2022-10-17T12:34:16Z

files/__addonLocation__/tsconfig.json

+  },
+  "include": [
+    "src/**/*",
+    "types/**/*"


In a v1 addon, the root types directory is a place we can put non-published type declarations because it actually belongs to the dummy app—it's the exact same types directory that we generate for a normal app, except that we messed up and it still ended up in the project root for addons instead of under tests/dummy. That oversight is one of the sins I regret most from our early work to support addons in ember-cli-typescript, because it creates a blurry picture of the role the types directory plays in an addon repository.

As it stands, this change is (re)introducing types/ as a first-class unpublished directory belonging only to the addon. For the near term, I think we do need some kind of "internal-only type declarations" location for addons—the local Glint registry is a good example of that, though it eventually goes away with strict mode—but my hope would be that the need for such declarations otherwise would be pretty minimal. The more of them you're relying on them, the easier it is to accidentally end up publishing code that won't typecheck for your consumers because they don't have those declarations themselves.

What do you think of something like local-types/ or internal-types/, or even just internal-types.d.ts by itself, to help communicate clearly that these types are not part of the addon itself that you're publishing?

Another alternative might be to home types/ in the test app rather than the addon, but still include it under includes so that it's accounted for when typechecking the addon.

/cc @chriskrycho @jamescdavis

I share the regret. Deeply. 😩

I think we actually have a continuing problem here in that it's actually just really hard to clarify that these types only belong to the test infrastructure for the addon and will not and should not be published, so I think putting them in the test app is the right move—even something like local-types or internal-types risks some confusion: "local" or "internal" might still imply to someone that they're not public but should be published (if that makes sense).

I also wonder (not a blocker for this PR, as we can definitely iterate) whether we might explicitly have something like:

/ app/ ... addon/ ... tsconfig/ compiler-settings.json test.json publish.json tsconfig.json

where tsconfig.json has "extends": "./tsconfig/compiler-settings.json" and the "includes" you mention, and then tsconfig/publish.json is configured to do the right thing along similar lines. With smart use of comments (thanks, jsonc!) in the config files, that combination of naming could help clarify the relationships involved?

It's unfortunate that having those be separate is, if not quite needful, then at least potentially helpful, but it's the pattern I've increasingly moved to for handling things like this in my own libraries and I suspect it might be a helpful secondary move here.

I see some caveats with regards to moving the internal types to the test app:

the blueprint creates a single test-app, but in some cases people might actually want to have more than one test app, when they need to be different for covering different test cases, e.g. apps with different dependencies (e.g. with and without fastboot). In that case it might become ambiguous and thus confusing where to put these types? (both test-apps might need them)

things could also go into the other direction, that you don't need a test-app at all. Like when you are able to write plain unit tests for your addon that don't depend on an actual runnable Ember app, you could put those (jest, vitest, whatever) tests right into the addon package, but still need to add some ambient types

Does it maybe make sense to put these type declarations into neither the addon nor the test-app, but basically at /types at the root level of the monorepo, outside of any actual package? The downside is that when you need to import some types within your declarations, you would then need to add those to the monorepo's root devDependencies 😕. Or we make that folder a private types-only package that we import from? Just thinking aloud here...

I spun up a new addon blueprinted from this branch and played around a bit with possible setups.

After playing with it, I think moving the types directory outside of the addon is a tall order, because for anything registry-based (namely loose-mode Glint, but this would also apply to services or Data), you can wind up running into the same issues that prompted typed-ember/glint#439

The registry pattern almost requires that entries be added somewhere within the package/project that consumes them. Someone who's deeply familiar with both their package manager and TS's resolution scheme may be able to massage their config to get it to work (or if they're on Yarn 1, things may be incidentally hoisted in their favor), but I'd be hesitant to count on that.

Given that, I'm now leaning toward keeping local types within the addon directory for the time being. Maybe we'll figure out an alternative strategy (or maybe one of y'all can come up with one now!), but I wouldn't want to block this indefinitely on that. As far as naming confusion, we could try unpublished-development-types or something equally explicit (à la React.__SECRET_DOM_DO_NOT_USE_OR_YOU_WILL_BE_FIRED).

I also still kind of like the idea of a single .d.ts file rather than a directory, both to discourage too much growth and to give an easy place to leave a // These types are not part of your published addon, be very careful, blah blah etc explainer comment. I'm not attached enough to that to fight for it, though 🙂

That makes sense to me.

For the record, this is now implemented by having a unpublished-development-types/index.d.ts, that has a comment like suggested and the boilerplate for the glint registry (for the addon's own classic templates)

dfreeman · 2022-10-17T12:37:25Z

files/__addonLocation__/tsconfig.json

+    "types/**/*"
+  ],
+  "glint": {
+    "environment": "ember-loose",


As far as I know it isn't even possible to use FCCTs in v2 addons yet, so including the template-imports environment here would be a pretty mean trick to play on authors 😜

files/__addonLocation__/tsconfig.json

dfreeman · 2022-10-17T12:49:48Z

files/__addonLocation__/types/glint.d.ts

@@ -0,0 +1,10 @@
+import '@glint/environment-ember-loose';


I think this relates to the question above about how we want to handle addon-only unpublished type declarations and making it as clear as possible to authors/readers the role they play.

tests/cli.test.ts

files/__addonLocation__/package.json

simonihmig · 2022-10-18T08:13:36Z

@dfreeman thanks for your review! I addressed the directly actionable points (marked the discussion threads as resolved). I think at this point only the role of types/ is up for debate. I think I would prefer internal-types/ myself...

files/__addonLocation__/package.json

simonihmig · 2022-11-02T19:47:36Z

I have now incorporated the latest outcome of typed-ember/glint#439 here!

ef4 · 2022-11-08T15:40:44Z

Thanks so much @simonihmig, this is great to have.

simonihmig added the enhancement New feature or request label Jul 6, 2022

simonihmig requested a review from NullVoxPopuli July 6, 2022 15:26

NullVoxPopuli reviewed Jul 6, 2022

View reviewed changes

files/__addonLocation__/rollup.config.__rollupExt__ Show resolved Hide resolved

NullVoxPopuli reviewed Jul 6, 2022

View reviewed changes

index.js Outdated Show resolved Hide resolved

NullVoxPopuli reviewed Jul 6, 2022

View reviewed changes

files/__addonLocation__/tsconfig.json Show resolved Hide resolved

NullVoxPopuli reviewed Jul 6, 2022

View reviewed changes

simonihmig force-pushed the typescript branch 2 times, most recently from 46a3379 to 0b7650e Compare July 21, 2022 10:47

NullVoxPopuli reviewed Aug 9, 2022

View reviewed changes

simonihmig force-pushed the typescript branch from 0b7650e to 0765810 Compare October 14, 2022 14:09

Windvis reviewed Oct 14, 2022

View reviewed changes

dfreeman reviewed Oct 17, 2022

View reviewed changes

simonihmig force-pushed the typescript branch from 325f894 to f7b3c9c Compare October 18, 2022 08:24

dfreeman reviewed Oct 20, 2022

View reviewed changes

files/__addonLocation__/package.json Show resolved Hide resolved

simonihmig marked this pull request as ready for review October 21, 2022 15:54

simonihmig added 10 commits November 2, 2022 20:43

Add --typescript support

8b4162a

Add message for setting up TS in test-app

8e0dbd2

Add lint:ts script for addon, add Glint config

5c2e8e3

Add --typescript to ecu config

0c46c22

Fix after rebase, check ember CLI version for --typescript support

90b0672

Use .js for all entrypoints

598e2f3

Update addon-dev

7454c09

Rename type checking script to lint:types

9afbea4

Fix tests

756eb0d

Fix package.json update

93f7be4

simonihmig added 4 commits November 2, 2022 20:46

Add --typescript tests

f2f6d1c

Address PR feedback

bfd5cd9

Fix parserOptions for TS

980ebad

Use new Glint addon registry best practices

eb2861b

simonihmig force-pushed the typescript branch from d7ea1b6 to eb2861b Compare November 2, 2022 19:46

NullVoxPopuli mentioned this pull request Nov 4, 2022

Better monorepo support. Use @embroider/addon-blueprint NullVoxPopuli/ember-addon-migrator#37

Merged

5 tasks

simonihmig added 2 commits November 8, 2022 16:10

Rename types to unpublished-development-types

583a2c5

Fix TS tests, add CI job

c36da5e

simonihmig merged commit 69d6b35 into main Nov 8, 2022

simonihmig deleted the typescript branch November 8, 2022 15:27

		"@glint/core": "^0.9.1",
		"@glint/environment-ember-loose": "^0.9.1",

Add Typescript support #42

Add Typescript support #42

Conversation

simonihmig commented Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli Jul 6, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreeman Oct 17, 2022 • edited

Choose a reason for hiding this comment

NullVoxPopuli Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dfreeman left a comment

Choose a reason for hiding this comment

dfreeman Oct 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig commented Oct 18, 2022

simonihmig commented Nov 2, 2022

ef4 commented Nov 8, 2022

simonihmig commented Jul 6, 2022 •

edited

NullVoxPopuli Jul 6, 2022 •

edited

dfreeman Oct 17, 2022 •

edited

NullVoxPopuli Oct 17, 2022 •

edited

dfreeman Oct 17, 2022 •

edited