[ENHANCEMENT] Add support for `--typescript` flag to `app` and `addon` blueprints #9972

simonihmig · 2022-07-22T22:52:03Z

This adds the --typescript flag to the ember new and ember addon commands, as per the CLI integration section of RFC800.

While the blueprint changes itself were not very difficult, what did cost a lot of time was debugging and identifying problems with regard to Ember CLI itself and the test coverage.

What is special here, and what uncovered (not caused) the mentioned problems, is that the blueprint delegates much of the work to ember-cli-typescript and its own default blueprint, when the flag is present. This is per se not bad, and it is what the RFC actually says. But by calling addAddonToProject(), it forcefully installs all the dependencies, even in tests, regardless of the --skip-npm flag. This is inevitable I think, as otherwise ember-cli-typescript's blueprint couldn't run in tests. But it makes the test naturally quite slow, and it uncovered some unexpected issues unrelated to the actual changes, which I will walk you through in the following...

It seems the project is shared between tests (see below). When initializeAddons() is called, it is supposed to also call discoverAddons() here. But it happens that _didDiscoverAddons is true here, so discoverAddons() return early. The thing is here, that usually in blueprint tests you only deal with the internal addons of ember-cli. But with the new typescript tests, and external addons are actually installed from npm, we now deal with different sets of this.addons depending on the test. In the typescript test, this.addons still would only contain the internal addons (due to the early return), which when trying to run ember-cli-typescript's default blueprint makes this fail, saying that ember-cli-typescript is an unknown addon (as this.addons hasn't been updated). So I think the fix I added here is legit!?
Own fix replaced by [BUGFIX release] Make sure newly installed addons are discovered when running ember install #10014
When running the typescript blueprint tests (which again install real external dependencies) with npm, an (unrelated) peer dependency issue arises, which is described here: peerDeps don't support Ember beta with npm emberjs/ember-test-helpers#1236. This makes the test fail, but the issue is not related to the test itself, it's real. Therefor for now, the added tests only work with yarn, which does not share this behavior around peer deps.
When running the blueprint acceptance tests, they call await ember(['new', 'foo', '--skip-npm', '--skip-bower', '--skip-git']); or similar. This eventually calls into Project#nullProject(), which return a singleton project instance. But this means this instance is shared (leaked!) between tests. This seems to not have caused issues before, as the instance was not fundamentally different for different tests. But it is now! If you look back at item 1., you see that for the typescript tests real external addons are added to this.addons, which is not the case for all the other tests. But now that this state is leaking between tests, the following happens:
Any tests would pass when running in isolation. But any of the tests that follows one of the new typescript tests would fail when calling await ember(...), with Cannot find module './commands' thrown when @ember/optional-features tries to (lazily!) require it here. See this CI run. This happens because in the (leaked) project instance this.addons still contains all the external addons in memory (including @ember/optional-features, which just happens to cause the error as it's the first one), while the actual modules on disk in the tmp folder have been deleted by the (previous) test's afterEach. So that require naturally fails. But the root issue here is that the project's state is leaking here between tests I think.
I was able to overcome this with a quick'n'dirty hack in the last commit, which basically removes the singleton behavior. In the latest test run you can see that all of the blueprint tests are passing now! But...

Fixed, see #9972 (comment).

CI is now failing with a JavaScript heap out of memory error. Probably this is due to this hack, which might be causing some memory leaks? However I didn't want to spend even more time on investigating this, before the real path to overcoming 3. is not determined.

So 3. is really the point where I would like some assistance from some CLI experts here on how to proceed with that, as I don't fully understand what the deal is with this "null project", and what implications it would have to not treat it as a singleton to prevent it from leaking between tests, or if some other approach is better suited. I'll keep this in a draft state for now, but please review nevertheless!

simonihmig · 2022-07-23T15:48:36Z

tests/fixtures/app/typescript/.eslintrc.js

+  },
+  rules: {},
+  overrides: [
+    // ts files


Putting the TS extends into an override for only .ts files is not something the RFC explicitly mandated, but it is what I have been doing everywhere. Otherwise, when putting this into the global extends, you would get false positives from TS rules when ESLint runs on plain .js files.

If you suggest a different solution, please tell me!

simonihmig · 2022-07-23T15:58:01Z

lib/models/project.js

@@ -311,7 +311,7 @@ class Project {
    if (fs.existsSync(`${targetsPath}.js`)) {
      this._targets = this.require(targetsPath);
    } else {
-      this._targets = require('../../blueprints/app/files/config/targets');
+      this._targets = { browsers: ['last 1 Chrome versions', 'last 1 Firefox versions', 'last 1 Safari versions'] };


When requiring ../../blueprints/app/files/config/targets, node would for some reason also try to read from ../../blueprints/app/files/package.json, which is a template file, not a real one.

Previously this caused no issues, as the only parts with "template tags" (or whatever you call these <% %>) was within .name here. But now we have more conditionals in place, which make this package.json template invalid JSON (in the raw format), so node would fail parsing it.

As targets.js is a JS file, I also couldn't just simply read it without making node interpret it (like with something like fs.readJSONSync()), so the only fix I could come up with is to unfortunately just duplicate the exported value here. There is a test covering the output of this, so hopefully the chance of this and the actual targets file diverging is very low! 🤔

lib/models/project.js

blueprints/app/files/package.json

dfreeman · 2022-07-25T07:54:45Z

blueprints/app/files/.eslintrc.js

-    // node files
+<% if (typescript) { %>    // ts files
+    {
+      files: ['**/*.ts'],


Maybe this has been changed/fixed in more recent versions of ESLint, but the last I checked, a glob like this would still require you to specify --ext .js,.ts when executing the linter. I've found just '*.ts' still enables linting for nested files while also being simple enough for ESLint to understand it should automatically lint .ts files as well.

I have this config in a few addons/projects without any problems! 🤷‍♂️

dfreeman · 2022-07-25T13:42:15Z

blueprints/app/files/app/router.__ext__

@@ -6,4 +6,4 @@ export default class Router extends EmberRouter {
  rootURL = config.rootURL;
 }

-Router.map(function () {});
+Router.map(function () {});<% if (typescript) { %> // eslint-disable-line @typescript-eslint/no-empty-function<% } %>


I'm surprised no-empty-function is part of the @typescript-eslint recommended ruleset but not the regular eslint:recommended one. (But that's neither here nor there 😄)

Would changing this to something like the snippet below appease the rule without us needing to explicitly disable it?

Router.map(function () { // Add route declarations here });

Would changing this to something like the snippet below appease the rule without us needing to explicitly disable it?

Yes, it would!

I just wanted to try out what happens with the comment when you add a route by ember g route, only to find out that it actually does not work at all with a router.ts file! Is this known?

Not sure if https://github.com/ember-cli/ember-router-generator would actually support parsing TS, but the error is thrown even before that lib comes into play, in the ember-source blueprint here, as it only checks for a router.js.

Yep, that's a known issue (Which we should definitely fix now that TS support is official! Though obviously not a hard blocker for this PR in particular)

Now using the suggested comment in the (otherwise still empty) function body!

blueprints/app/files/package.json

simonihmig · 2022-08-18T12:24:10Z

For the record: it seems @ef4 has fixed the main nullProject problem here, so thanks a bunch for that! 🎉

For my own record: I recently used this branch to create a real new project, and realized that we need to also...

convert tests/helpers/index.js to TS (you'll get a type error, as this is imported from TS).

ef4 · 2022-08-18T14:04:01Z

Yes, the NullProject question is resolved. The summary is that NullProject is supposed to be for the case where ember-cli is running outside of any project. That is the situation when you run ember new. But then to install addons into the newly generated project, we need to have a real Project and not NullProject.

simonihmig · 2022-08-18T16:23:42Z

blueprints/app/files/tests/helpers/index.ts

+
+function setupApplicationTest(
+  hooks: Parameters<typeof upstreamSetupApplicationTest>[0],
+  options?: Parameters<typeof upstreamSetupApplicationTest>[1]


TS team: please have a look here!

I could have typed the first arg as hooks: NestedHooks, as @types/ember-qunit declares this type globally. But not the SetupOptions interface, which is not even exported. So the only way I found to type this was like this. And for consistency I chose to do this for both args...

This could also be (...[hooks, options]: Parameters<typeof upstreamSetupApplicationTest>), not that that's much better. I think we should probably just export the SetupOptions type from @types/ember-qunit.

I am doing that now!

DefinitelyTyped/DefinitelyTyped#62073 – I exported it as @internal, since it is expressly not part of the public API, but is important for collaborators here. I also opened emberjs/ember-qunit#957 to track converting ember-qunit to TS!

Using the new export now!

simonihmig · 2022-08-19T07:07:07Z

I think this PR is good now, with only the somewhat philosophical question remaining of how to name and integrate the type checking script.

So please give this a/another review, Ember-CLI team & TS-Team (@dfreeman @chriskrycho @jamescdavis)!

ef4 · 2022-08-19T13:51:45Z

(Please don't take my discussion above over the definition of "lint" as a blocker, I've said my piece but I can live with whatever will get this moving.)

chriskrycho · 2022-08-19T21:10:30Z

Will review Monday or Tuesday. And for what it's worth the Typed Ember team generally shared the "not going to block on this" POV about naming. It's probably unsurprising that we do disagree about whether to think about type checking and linting as equivalent—but we would all rather ship this with a name we don't love than not ship it while arguing over that name!

This unblocks ember-cli/ember-cli#9972.

chriskrycho

All right, one suggested tweak here, but looks good otherwise!

blueprints/app/files/package.json

* ember-qunit: make SetupTestOptions public This unblocks ember-cli/ember-cli#9972. * ember-qunit: document that SetupTestOptions is internal * ember-qunit: SetupTestOptions is *not* internal! * ember-qunit: only change ember-qunit, not other Ember stuff

lib/models/project.js

simonihmig · 2022-09-18T13:10:46Z

Status: Will wait for #10014 to get merged and rebase, after that this PR should hopefully be good then!

bertdeblock

Noticed a few things:

When generating an app or addon, I'm seeing the following warning logged about 40 times:

You passed the '--typescript' flag but there is no TypeScript blueprint available. A JavaScript blueprint will be generated instead.

Running ember s in a new app or addon fails with the following error:

Error: Typechecking failed

Do we want/need to specify/control the version of ember-cli-typescript?

tests/acceptance/new-test.js

simonihmig · 2022-09-25T16:29:20Z

When generating an app or addon, I'm seeing the following warning logged about 40 times:
You passed the '--typescript' flag but there is no TypeScript blueprint available. A JavaScript blueprint will be generated instead.

The logic around this warning message would emit the warning whenever the --typescript flag is explicitly passed, and the blueprint files contain any non-ts files, like hbs. So this seems to be a general bug, that probably way only uncovered now with the blueprint shipping so many other non-ts files. Fixed that in 538acd0.

Running ember s in a new app or addon fails with the following error:
Error: Typechecking failed

I realized that some types packages like @types/ember-qunit were not installed by e-c-ts' own blueprint, as the fix by @ef4 would leave the stale version of project in blueprintOptions, so the e-c-ts blueprint would not see that ember-qunit is installed here and therefore not install its types. Fixed in 005db39.

In my tests, a generated TS app lints, tests and runs fine now!

Do we want/need to specify/control the version of ember-cli-typescript?

Idk, why would we not want to have the latest version?

I realize some people love highly-mocked unit tests like these, but IMO they are an extremely poor value for effort. This whole are can only really be profitably tested in a more integration test pattern.

`

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

simonihmig · 2022-10-04T21:39:00Z

I rebased this, so I think this is good to go now!

ef4 · 2022-10-05T17:39:06Z

Thanks so much, great to have this landing!

bertdeblock · 2022-10-05T17:55:03Z

Thanks @simonihmig!

simonihmig force-pushed the ts-blueprint branch 2 times, most recently from eb1f2d3 to e8ea613 Compare July 23, 2022 11:32

simonihmig mentioned this pull request Jul 23, 2022

Add Typescript support embroider-build/addon-blueprint#42

Merged

2 tasks

simonihmig commented Jul 23, 2022

View reviewed changes

lib/models/project.js Outdated Show resolved Hide resolved

simonihmig commented Jul 23, 2022

View reviewed changes

blueprints/app/files/package.json Outdated Show resolved Hide resolved

dfreeman reviewed Jul 25, 2022

View reviewed changes

ef4 force-pushed the ts-blueprint branch from 0ee00ba to f7a0ac3 Compare August 17, 2022 21:55

simonihmig commented Aug 18, 2022

View reviewed changes

simonihmig force-pushed the ts-blueprint branch 2 times, most recently from f967b7d to 25aa489 Compare August 18, 2022 18:58

simonihmig marked this pull request as ready for review August 19, 2022 07:07

simonihmig changed the title ~~Add support for --typescript in app/addon blueprint~~ [ENHANCEMENT] Add support for --typescript in app/addon blueprint Aug 19, 2022

chriskrycho added a commit to chriskrycho/DefinitelyTyped that referenced this pull request Sep 1, 2022

ember-qunit: make SetupTestOptions public

e3a2ebe

This unblocks ember-cli/ember-cli#9972.

chriskrycho added a commit to chriskrycho/DefinitelyTyped that referenced this pull request Sep 1, 2022

ember-qunit: make SetupTestOptions public

5a8a765

This unblocks ember-cli/ember-cli#9972.

chriskrycho mentioned this pull request Sep 1, 2022

ember-qunit: export SetupTestOptions DefinitelyTyped/DefinitelyTyped#62073

Merged

7 tasks

chriskrycho requested changes Sep 1, 2022

View reviewed changes

blueprints/app/files/package.json Outdated Show resolved Hide resolved

bertdeblock reviewed Sep 8, 2022

View reviewed changes

lib/models/project.js Show resolved Hide resolved

simonihmig mentioned this pull request Sep 18, 2022

[BUGFIX release] Make sure newly installed addons are discovered when running ember install #10014

Merged

bertdeblock reviewed Sep 18, 2022

View reviewed changes

tests/acceptance/new-test.js Outdated Show resolved Hide resolved

simonihmig force-pushed the ts-blueprint branch from 30b725e to 005db39 Compare September 25, 2022 16:12

ef4 and others added 14 commits October 4, 2022 22:26

possible solution for NullProject test issue

d83467f

update unit tests mocking

5b0032a

I realize some people love highly-mocked unit tests like these, but IMO they are an extremely poor value for effort. This whole are can only really be profitably tested in a more integration test pattern.

handle optional feature

b25bd03

Refactor to use existing shouldTransformTypeScript infrastructure

8f3f5ff

`

lint:types

7c452dc

Co-authored-by: Chris Krycho <hello@chriskrycho.com>

Add empty comment in router

e4e7029

Add e-c-ts as dependency for addons

d0cbbd0

Use new types from ember-qunit

874a920

Fix warning message

66f0e29

Fix tests

8a042cd

Fix stale project

a8683ad

Fix tests

09ada8f

Fix test for test helper

85f99a5

Fix embroider test

490e73d

simonihmig force-pushed the ts-blueprint branch from e5f0fc0 to 490e73d Compare October 4, 2022 20:26

bertdeblock added the enhancement label Oct 4, 2022

bertdeblock changed the title ~~[ENHANCEMENT] Add support for --typescript in app/addon blueprint~~ [ENHANCEMENT] Add support for --typescript flag to app and addon blueprints Oct 4, 2022

ef4 merged commit dea31e7 into ember-cli:master Oct 5, 2022

This was referenced Oct 13, 2022

Creating a new app or addon with the --typescript flag overrules the use of the --skip-npm flag #10045

Closed

Make addAddonsToProject support creating a new project with custom target directory #10048

Merged

simonihmig deleted the ts-blueprint branch October 14, 2022 19:07

jaredgalanis mentioned this pull request Dec 8, 2022

Ember 4.9 release post ember-learn/ember-blog#1240

Merged

chriskrycho mentioned this pull request Sep 28, 2023

Should router.ts also be supported? typed-ember/ember-cli-typescript#165

Closed

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

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

[ENHANCEMENT] Add support for `--typescript` flag to `app` and `addon` blueprints #9972

[ENHANCEMENT] Add support for `--typescript` flag to `app` and `addon` blueprints #9972

simonihmig commented Jul 22, 2022 •

edited

simonihmig Jul 23, 2022

simonihmig Jul 23, 2022 •

edited

dfreeman Jul 25, 2022

simonihmig Jul 25, 2022

dfreeman Jul 25, 2022

simonihmig Jul 25, 2022

dfreeman Jul 25, 2022

simonihmig Sep 25, 2022

simonihmig commented Aug 18, 2022 •

edited

ef4 commented Aug 18, 2022

simonihmig Aug 18, 2022

dfreeman Aug 23, 2022

chriskrycho Sep 1, 2022

chriskrycho Sep 1, 2022

simonihmig Sep 25, 2022

simonihmig commented Aug 19, 2022

ef4 commented Aug 19, 2022

chriskrycho commented Aug 19, 2022 •

edited

chriskrycho left a comment

simonihmig commented Sep 18, 2022

bertdeblock left a comment

simonihmig commented Sep 25, 2022

simonihmig commented Oct 4, 2022

ef4 commented Oct 5, 2022

bertdeblock commented Oct 5, 2022

[ENHANCEMENT] Add support for --typescript flag to app and addon blueprints #9972

[ENHANCEMENT] Add support for --typescript flag to app and addon blueprints #9972

Conversation

simonihmig commented Jul 22, 2022 • edited

Choose a reason for hiding this comment

simonihmig Jul 23, 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

simonihmig commented Aug 18, 2022 • edited

ef4 commented Aug 18, 2022

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 Aug 19, 2022

ef4 commented Aug 19, 2022

chriskrycho commented Aug 19, 2022 • edited

chriskrycho left a comment

Choose a reason for hiding this comment

simonihmig commented Sep 18, 2022

bertdeblock left a comment

Choose a reason for hiding this comment

simonihmig commented Sep 25, 2022

simonihmig commented Oct 4, 2022

ef4 commented Oct 5, 2022

bertdeblock commented Oct 5, 2022

[ENHANCEMENT] Add support for `--typescript` flag to `app` and `addon` blueprints #9972

[ENHANCEMENT] Add support for `--typescript` flag to `app` and `addon` blueprints #9972

simonihmig commented Jul 22, 2022 •

edited

simonihmig Jul 23, 2022 •

edited

simonihmig commented Aug 18, 2022 •

edited

chriskrycho commented Aug 19, 2022 •

edited