-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor --typescript
support in blueprints
#10283
Conversation
64f712f
to
e61dfec
Compare
e61dfec
to
80dcb54
Compare
blueprints/addon/index.js
Outdated
|
||
contents.typesVersions = { | ||
'*': { | ||
'test-support/*': ['declarations/addon-test-support/*'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here has a suggestion how to get around this?
"*": {
"test-support": ["declarations/addon-test-support/index.d.ts"]
}
should cover that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
Outdated
|
||
contents.typesVersions = { | ||
'*': { | ||
'test-support/*': ['declarations/addon-test-support/*'], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here has a suggestion how to get around this?
"*": {
"test-support": ["declarations/addon-test-support/index.d.ts"]
}
should cover that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work! so happy to see this landing 🎉
contents.scripts.prepack = 'tsc --project tsconfig.declarations.json'; | ||
contents.scripts.postpack = 'rimraf declarations'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add declarations/
to .gitignore
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
172d50c
to
a0bcb8e
Compare
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... |
a0bcb8e
to
c0ee61c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
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! |
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.
7710aa0
to
9cc0cf8
Compare
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!? 🤞 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😭 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
9cc0cf8
to
05797ad
Compare
@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.
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.
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.
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.
@@ -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'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How are we going to keep this up to date automatically? The dev/update-blueprint-dependencies.js doesn't play well with this currently.
This is refactoring the
--typescript
support in app and addon blueprints according to the RFC update of emberjs/rfcs#902. More specifically it willember-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.ember-cli-typescript
in the actual app/addonintroduce Glintactually 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.
prepack
/postpack
scripts for addon publishing from the commands provided byember-cli-typescript
to simple commands referring totsc
(rimraf
) directly. For this I had to introduce a separatetsconfig.declarations.json
(that replicates the previoustsc
options), as we need to changeincludes
to not produce type declarations for tests, and there is no--includes
we could pass to thetsc
CLI. Also the declarations are now written to a separatedeclarations
folder (dist
is already taken), and mapped to bytypesVersions
. This was needed so we can easily clean up those files as part ofpostpack
(actually, we could also just not clean up, and just gitignore the folder!? 🤔 ). Previously the.d.ts
were written into the root (sotypesVersions
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:
prepack
/postpack
scripts for addons without e-c-tsI 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