Refactor `--typescript` support in blueprints #10283

simonihmig · 2023-06-01T17:05:44Z

This is refactoring the --typescript support in app and addon blueprints according to the RFC update of emberjs/rfcs#902. More specifically it will

not use ember-cli-typescript for the scaffolding - which fixes Creating a new app or addon with the --typescript flag overrules the use of the --skip-npm flag #10045 and helps with a bunch of issues where the blueprint is used downstream: v2 addon blueprint, migration tools etc.
not use ember-cli-typescript in the actual app/addon
~~introduce Glint~~
actually I reverted my change to not do this (yet), because I had some concerns about it. We do add some Glint setup to make the (dummy) app what I was calling "Glint-ready" in the sense that it is able to consume Glint-enabled addons without running into type errors, as discussed here on Discord, but it is not enabling full Glint type checking for the app itself. Omitting this potentially controversial choice felt like increasing the chances of getting this merged faster. If we still think full Glint-enablement should be part of the revised RFC0800, then we can add that later as a separate PR easily.
replace the prepack/postpack scripts for addon publishing from the commands provided by ember-cli-typescript to simple commands referring to tsc (rimraf) directly. For this I had to introduce a separate tsconfig.declarations.json (that replicates the previous tsc options), as we need to change includes to not produce type declarations for tests, and there is no --includes we could pass to the tsc CLI. Also the declarations are now written to a separate declarations folder (dist is already taken), and mapped to by typesVersions. This was needed so we can easily clean up those files as part of postpack (actually, we could also just not clean up, and just gitignore the folder!? 🤔 ). Previously the .d.ts were written into the root (so typesVersions was not needed), but there was some special housekeeping in place to selectively remove those files again as part of the e-c-ts commands, which we cannot replicate easily without introducing custom commands again...

To Do:

figure out how to do the prepack/postpack scripts for addons without e-c-ts
finalize discussion and get Revise RFC 0800 with learnings from implementation emberjs/rfcs#902 merged? - are we blocked on this?
battle-test the produced output (especially TS addons consumed by TS apps)

I cannot request reviewers here, so pinging a few (active and former) CLI + TS folks for some of you to review please! 😀
@kategengler @kellyselden @bertdeblock @ef4 @NullVoxPopuli @mansona @chriskrycho @gitKrystan @jamescdavis @wagenet @dfreeman

simonihmig · 2023-06-08T07:54:44Z

blueprints/addon/index.js

+
+      contents.typesVersions = {
+        '*': {
+          'test-support/*': ['declarations/addon-test-support/*'],


This is doing Ember's conventional mapping of addon-test-support (author) → test-support (consumer).

I have seen some issues though importing things from index-modules using that typesVersions approach, like having a addon/foo/index.ts module and trying to import that as import foo from 'my-addon/foo', which did cause type errors, but import foo from 'my-addon/foo/index' did not. This seems to be some bug in TypeScript itself, but maybe someone here has a suggestion how to get around this?

That's where definitely some more testing is needed...

here has a suggestion how to get around this?

"*": { "test-support": ["declarations/addon-test-support/index.d.ts"] }

should cover that?

What about arbitrary modules you might want to export? Like import foo from 'my-addon/foo/bar/baz'

However coincidentally I was running into that also in another project, and we got it working I think with something like that:

{ '*': { 'test-support/*': ['declarations/addon-test-support/*', 'declarations/addon-test-support/*/index.d.ts'], '*': ['declarations/addon/*', 'declarations/addon/*/index.d.ts'], },

I need to double check that though...

I tested different variations of imports, and this is what seems to be working fine now:

"typesVersions": { "*": { "test-support": [ "declarations/addon-test-support/index.d.ts" ], "test-support/*": [ "declarations/addon-test-support/*", "declarations/addon-test-support/*/index.d.ts" ], "*": [ "declarations/addon/*", "declarations/addon/*/index.d.ts" ] } }

blueprints/addon/index.js

blueprints/addon/files/tsconfig.json

NullVoxPopuli · 2023-06-08T14:25:08Z

blueprints/addon/index.js

+
+      contents.typesVersions = {
+        '*': {
+          'test-support/*': ['declarations/addon-test-support/*'],


here has a suggestion how to get around this?

"*": { "test-support": ["declarations/addon-test-support/index.d.ts"] }

should cover that?

blueprints/app/files/package.json

tests/fixtures/addon/typescript/package.json

tests/fixtures/addon/typescript/tsconfig.json

NullVoxPopuli

nice work! so happy to see this landing 🎉

blueprints/addon/files/tsconfig.declarations.json

blueprints/addon/files/tsconfig.json

dfreeman · 2023-06-12T11:17:10Z

blueprints/addon/index.js

+      contents.scripts.prepack = 'tsc --project tsconfig.declarations.json';
+      contents.scripts.postpack = 'rimraf declarations';


Should we also add declarations/ to .gitignore?

Yeah, thought about this as well... but then we don't really need that rimraf script anymore, right? The previous behaviour of e-c-ts was to create the declarations only before publishing and immediately delete them afterwards, without them being git-ignored. And this is what is basically happening here as well.

So should we git-ignore declarations and remove rimraf, or have both?

Now that the declarations are in their own directory instead of scattered directly throughout the project, I think removing them isn't such a big deal, but I don't feel particularly strongly about it either way 🙂

Discussed with embroider core meeting and we recommend adding to gitignore and also calling rimraf either before a fresh build or after (so that extra files are not left around and picked up by typescript).

Should we also add declarations/ to .gitignore?

Just did that!

The .gitignore blueprint file is used for both apps and addons, so apps now also get this change. Which isn't really needed. But also that file is already a bit blurry (yarn vs. npm, ember-try stuff for apps), so I think it's ok?

blueprints/app/files/tsconfig.json

blueprints/app/files/package.json

blueprints/addon/files/ember-cli-build.js

simonihmig · 2023-06-12T16:53:44Z

Thanks @dfreeman for your review! I just pushed the changes addressing that. I resolved all the conversations here where I basically applied your suggestion.

Only mildly controversial thing left is that rimraf vs. gitignore thing, which I also don't have strong opinions on (also personally I'm not really interested in creating new v1 addons anyway 😅). I'll leave this here for others to chime in...

dfreeman

I'll let @chriskrycho weigh in w.r.t. how this relates to merging emberjs/rfcs#902, but everything I'd flagged looks good to me now. Thanks for your hard work here, @simonihmig!

chriskrycho · 2023-06-28T20:32:58Z

I think it's totally good and fine to merge this as it stands! The RFC states intent and we can keep iterating toward that. Thanks so much for driving this forward, @simonihmig!

NullVoxPopuli · 2023-06-28T23:15:29Z

doh, looks like a conflict is present!

This will allow them to consume addons shipping Glint types, but will not yet enable Glint for the app itself.

simonihmig · 2023-06-29T10:22:46Z

Rebased this, and synced some fixtures, all blueprint tests are passing now, including those for Embroider!

The failing scenarios for Embroider seem unrelated, as these are also failing on master! 🤔

So I think this is good to merge now!? 🤞

kategengler

😭 Unfortunately CI started failing last night. I will merge this as soon as we get it green, since we cannot release until then.

Fixes after review Fixes after Dan's review Fixes after rebase Add declarations to gitignore

simonihmig · 2023-07-01T18:36:36Z

@kategengler seems the Embroider issue was fixed. I forced pushed to kick off another CI run, and... ✅

Use canary version of ember-cli on master branch, until we ge get a stable/beta release on npm, which includes better support of `--typescript` ([PR](ember-cli/ember-cli#10283)). Cleans up previous weirdness.

Update our blueprint to take account of changes when generating the test-app with `--typescript` ([PR](ember-cli/ember-cli#10283)). The new blueprint now drops ember-cli-typescript and opts into the TS transform of ember-cli-babel, which we must not override.

Use canary version of ember-cli on master branch, until we ge get a stable/beta release on npm, which includes better support of `--typescript` ([PR](ember-cli/ember-cli#10283)). Cleans up previous weirdness.

kellyselden · 2023-10-30T21:07:27Z

blueprints/addon/index.js

@@ -77,6 +82,22 @@ module.exports = {
    // 100% of addons don't need ember-cli-app-version, make it opt-in instead
    delete contents.devDependencies['ember-cli-app-version'];

+    // add scripts to build type declarations for TypeScript addons
+    if (this.options.typescript) {
+      contents.devDependencies.rimraf = '^5.0.1';


How are we going to keep this up to date automatically? The dev/update-blueprint-dependencies.js doesn't play well with this currently.

simonihmig force-pushed the refactor-typescript branch from 64f712f to e61dfec Compare June 1, 2023 17:09

simonihmig mentioned this pull request Jun 1, 2023

Revise RFC 0800 with learnings from implementation emberjs/rfcs#902

Merged

simonihmig force-pushed the refactor-typescript branch from e61dfec to 80dcb54 Compare June 7, 2023 19:32

simonihmig marked this pull request as ready for review June 8, 2023 07:46

simonihmig commented Jun 8, 2023

View reviewed changes

IgnaceMaes reviewed Jun 8, 2023

View reviewed changes

blueprints/addon/index.js Show resolved Hide resolved

NullVoxPopuli requested changes Jun 8, 2023

View reviewed changes

NullVoxPopuli mentioned this pull request Jun 8, 2023

package + test-app, eslintrc NullVoxPopuli/ember-addon-migrator#68

Closed

NullVoxPopuli approved these changes Jun 10, 2023

View reviewed changes

dfreeman reviewed Jun 12, 2023

View reviewed changes

simonihmig force-pushed the refactor-typescript branch from 172d50c to a0bcb8e Compare June 12, 2023 16:47

simonihmig force-pushed the refactor-typescript branch from a0bcb8e to c0ee61c Compare June 13, 2023 11:05

dfreeman approved these changes Jun 13, 2023

View reviewed changes

simonihmig mentioned this pull request Jun 15, 2023

Fix cd <path> in CONTRIBUTING.md embroider-build/addon-blueprint#128

Merged

simonihmig added 4 commits June 29, 2023 09:51

Refactor TypeScript support for app blueprint

ff3e15e

Refactor TypeScript support for addon blueprint

c67d20f

Make apps "Glint-ready", but don't enable Glint by default yet

cc93e93

This will allow them to consume addons shipping Glint types, but will not yet enable Glint for the app itself.

Add compile scripts for TypeScript addons

d337b6f

simonihmig force-pushed the refactor-typescript branch 3 times, most recently from 7710aa0 to 9cc0cf8 Compare June 29, 2023 08:51

kategengler approved these changes Jun 29, 2023

View reviewed changes

Fix fixtures for Embroider + TypeScript

05797ad

Fixes after review Fixes after Dan's review Fixes after rebase Add declarations to gitignore

simonihmig force-pushed the refactor-typescript branch from 9cc0cf8 to 05797ad Compare July 1, 2023 16:38

kategengler merged commit 992126a into ember-cli:master Jul 1, 2023
12 checks passed

simonihmig deleted the refactor-typescript branch July 2, 2023 19:36

simonihmig mentioned this pull request Jul 2, 2023

Run tests with latest Ember CLI embroider-build/addon-blueprint#134

Merged

simonihmig mentioned this pull request Jul 3, 2023

Move off rollup-plugin-ts embroider-build/addon-blueprint#136

Merged

simonihmig mentioned this pull request Jul 3, 2023

Update for ember-cli --typescript changes embroider-build/addon-blueprint#137

Merged

ijlee2 mentioned this pull request Sep 14, 2023

Updated blueprints ijlee2/ember-codemod-v1-to-v2#66

Merged

This was referenced Oct 29, 2023

Upgrade add-on to Ember.js v5.4.0 DazzlingFugu/ember-cli-embedded#305

Merged

Upgrade add-on to Ember.js v5.4.0 DazzlingFugu/ember-feature-controls#255

Merged

kellyselden reviewed Oct 30, 2023

View reviewed changes

This was referenced Nov 12, 2023

Release v4.0.0 DazzlingFugu/ember-cli-embedded#311

Merged

Release v5.0.0 DazzlingFugu/ember-feature-controls#262

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor `--typescript` support in blueprints #10283

Refactor `--typescript` support in blueprints #10283

simonihmig commented Jun 1, 2023 •

edited

Loading

simonihmig Jun 8, 2023

NullVoxPopuli Jun 8, 2023

simonihmig Jun 8, 2023 •

edited

Loading

simonihmig Jun 12, 2023

NullVoxPopuli Jun 8, 2023

NullVoxPopuli left a comment

dfreeman Jun 12, 2023

simonihmig Jun 12, 2023

dfreeman Jun 12, 2023

ef4 Jun 13, 2023

simonihmig Jun 13, 2023

simonihmig commented Jun 12, 2023

dfreeman left a comment

chriskrycho commented Jun 28, 2023

NullVoxPopuli commented Jun 28, 2023

simonihmig commented Jun 29, 2023

kategengler left a comment

simonihmig commented Jul 1, 2023

kellyselden Oct 30, 2023 •

edited

Loading

		contents.scripts.prepack = 'tsc --project tsconfig.declarations.json';
		contents.scripts.postpack = 'rimraf declarations';

Refactor --typescript support in blueprints #10283

Refactor --typescript support in blueprints #10283

Conversation

simonihmig commented Jun 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonihmig Jun 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NullVoxPopuli left a 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 Jun 12, 2023

dfreeman left a comment

Choose a reason for hiding this comment

chriskrycho commented Jun 28, 2023

NullVoxPopuli commented Jun 28, 2023

simonihmig commented Jun 29, 2023

kategengler left a comment

Choose a reason for hiding this comment

simonihmig commented Jul 1, 2023

kellyselden Oct 30, 2023 • edited Loading

Choose a reason for hiding this comment

Refactor `--typescript` support in blueprints #10283

Refactor `--typescript` support in blueprints #10283

simonihmig commented Jun 1, 2023 •

edited

Loading

simonihmig Jun 8, 2023 •

edited

Loading

kellyselden Oct 30, 2023 •

edited

Loading