[ENHANCEMENT] Add support for --typescript flag to app and addon blueprints#9972
[ENHANCEMENT] Add support for --typescript flag to app and addon blueprints#9972ef4 merged 16 commits intoember-cli:masterfrom
--typescript flag to app and addon blueprints#9972Conversation
eb1f2d3 to
e8ea613
Compare
| }, | ||
| rules: {}, | ||
| overrides: [ | ||
| // ts files |
There was a problem hiding this comment.
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!
| 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'] }; |
There was a problem hiding this comment.
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! 🤔
| // node files | ||
| <% if (typescript) { %> // ts files | ||
| { | ||
| files: ['**/*.ts'], |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I have this config in a few addons/projects without any problems! 🤷♂️
| } | ||
|
|
||
| Router.map(function () {}); | ||
| Router.map(function () {});<% if (typescript) { %> // eslint-disable-line @typescript-eslint/no-empty-function<% } %> |
There was a problem hiding this comment.
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
});There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Now using the suggested comment in the (otherwise still empty) function body!
|
For the record: it seems @ef4 has fixed the main For my own record: I recently used this branch to create a real new project, and realized that we need to also...
|
|
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 |
|
|
||
| function setupApplicationTest( | ||
| hooks: Parameters<typeof upstreamSetupApplicationTest>[0], | ||
| options?: Parameters<typeof upstreamSetupApplicationTest>[1] |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
Using the new export now!
f967b7d to
25aa489
Compare
|
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)! |
--typescript in app/addon blueprint--typescript in app/addon blueprint
|
(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.) |
|
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! |
chriskrycho
left a comment
There was a problem hiding this comment.
All right, one suggested tweak here, but looks good otherwise!
* 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
|
Status: Will wait for #10014 to get merged and rebase, after that this PR should hopefully be good then! |
bertdeblock
left a comment
There was a problem hiding this comment.
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 sin 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?
30b725e to
005db39
Compare
The logic around this warning message would emit the warning whenever the
I realized that some types packages like In my tests, a generated TS app lints, tests and runs fine now!
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 <[email protected]>
e5f0fc0 to
490e73d
Compare
|
I rebased this, so I think this is good to go now! |
--typescript in app/addon blueprint--typescript flag to app and addon blueprints
|
Thanks so much, great to have this landing! |
|
Thanks @simonihmig! |
This adds the
--typescriptflag to theember newandember addoncommands, 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-typescriptand 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 callingaddAddonToProject(), it forcefully installs all the dependencies, even in tests, regardless of the--skip-npmflag. This is inevitable I think, as otherwiseember-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 theprojectis shared between tests (see below). WheninitializeAddons()is called, it is supposed to also calldiscoverAddons()here. But it happens that_didDiscoverAddonsis true here, sodiscoverAddons()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 ofthis.addonsdepending on the test. In the typescript test,this.addonsstill would only contain the internal addons (due to the early return), which when trying to runember-cli-typescript's default blueprint makes this fail, saying thatember-cli-typescriptis an unknown addon (asthis.addonshasn'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#10014When 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 callawait ember(['new', 'foo', '--skip-npm', '--skip-bower', '--skip-git']);or similar. This eventually calls intoProject#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 tothis.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(...), withCannot find module './commands'thrown when@ember/optional-featurestries to (lazily!) require it here. See this CI run. This happens because in the (leaked) project instancethis.addonsstill 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'safterEach. So thatrequirenaturally 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 aJavaScript heap out of memoryerror. 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!