Skip to content
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

Revise RFC 0800 with learnings from implementation #902

Merged
merged 5 commits into from
Sep 7, 2023
Merged

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Feb 7, 2023

This revises the original RFC #800 with several points learned during implementation:

  • Instead of implementing this RFC in terms of ember-cli-typescript, we plan to sunset it. We will write a corresponding blog post and coordinate with the CLI team about timing to finish up a transition that really began several years ago.

  • We learned that keeping around the service registry is useful for integration with Owner, and implemented that in terms that make it user extensible (this needs further documentation, but is well-defined).

  • We answered the unresolved questions: how to start projects, updates for blueprints, how we handle classic features, how we integrate “constructibility” as defined in RFC: Semantic Versioning for TypeScript Types #730, and how we plan (at a high level) to do side-by-side TS and JS docs.

Copy link
Member

@wagenet wagenet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems generally good to me.

Copy link
Contributor

@simonihmig simonihmig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have this draft PR lined up to implement the changes proposed here. While working on this and seeing the result in action two questions came up, which I am leaving here for further discussion...

- Include `@typescript-eslint` in the `plugins` key.
- Include `plugin:@typescript-eslint/recommended` in the `extends` key.
- Install the `@typescript-eslint` dependencies instead of, or in addition to (as appropriate), the Babel ESLint dependencies in `package.json`.
- Configure [Glint][glint] for type checking and, for addons, emitting type declarations during build.
Copy link
Contributor

@simonihmig simonihmig Jun 1, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for addons, emitting type declarations during build

Any idea or prescriptions how to do that exactly? With e-c-ts we have dedicated commands for this, which I believe put the .d.ts files at the root level and know how to properly clean this up again as part of postpack. Without it, will we just put a long tsc command into prepack, mostly replicating what the command does? But then how do we clean this up? Or should we put all declarations into a separate folder (we could use dist if it wasn't used already for the Ember CLI build, so maybe declarations?) and have exports and typesVersions configs in package.json point to it, similar to what v2 addons already do?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. I appreciate that you just solved this while I was buried in other things. 😁

  2. I think the approach you landed on over at Refactor --typescript support in blueprints ember-cli/ember-cli#10283 is a good one. For other folks who haven't read that (and for posterity): the blueprint emits types into declarations and uses typesVersions to make resolution work correctly for consumers. (It also removes that after publishing.) This is probably what we would have done in the first place with ember-cli-typescript if we had had typesVersions available all those years ago.

text/0800-ts-adoption-plan.md Show resolved Hide resolved
@gitKrystan
Copy link
Contributor

Per TS core meeting: @chriskrycho to check status

@chriskrycho
Copy link
Contributor Author

Thanks to folks for (a) feedback and (b) implementing this! I think it's ready to merge, pending a review from @wagenet and/or @gitKrystan.

@wagenet wagenet merged commit ead7970 into master Sep 7, 2023
8 checks passed
@delete-merged-branch delete-merged-branch bot deleted the revise-800 branch September 7, 2023 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-Polaris Work for the Polaris Edition T-ember-cli RFCs that impact the ember-cli library T-TypeScript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants