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

RFC: TypeScript Adoption Plan #800

Merged
merged 2 commits into from
Apr 15, 2022
Merged

RFC: TypeScript Adoption Plan #800

merged 2 commits into from
Apr 15, 2022

Conversation

chriskrycho
Copy link
Contributor

@chriskrycho chriskrycho commented Feb 24, 2022

Rendered RFC


RFC #0724 commits Ember to officially supporting TypeScript and articulates an overall philosophy for what official support means. This RFC defines a detailed implementation plan for officially supporting TypeScript in Ember, including:

  • the SemVer policies Ember packages should adopt, following RFC #0730:
    • supported TypeScript versions
    • the "breaking change" policy
  • an Edition support policy
  • how we will migrate users from depending on the @types definitions on DefinitelyTyped to Ember packages
  • test infrastructure to catch regressions early
  • updates to Ember CLI to support TypeScript
  • release "channel" testing analogous to Ember's existing feature flag system for runtime code
  • a basic communication plan for the rollout
  • updates to our guides, API docs, and even the version release blog post announcement

…and more!


Thanks to the folks on the TypeScript adoption strike team for discussion and feedback over the course of 2021, as well as to the present and past members of the Typed Ember team for many productive discussions and input over the years, which have brought us to today!

@chriskrycho chriskrycho self-assigned this Feb 24, 2022
@chriskrycho chriskrycho added T-ember-cli RFCs that impact the ember-cli library T-ember-data RFCs that impact the ember-data library T-framework RFCs that impact the ember.js library T-learning labels Feb 24, 2022
@chriskrycho chriskrycho force-pushed the ts-adoption-plan branch 2 times, most recently from 2e2af92 to 4aff3a4 Compare February 24, 2022 23:36
@dbendaou
Copy link

@wandroll

@joukevandermaas
Copy link

Forgive me if I missed it, but does this include typing hbs templates? Currently with the addon you can do whatever you want in the template and the compiler does not care.

@chriskrycho
Copy link
Contributor Author

@joukevandermaas We do have (currently experimental, but rapidly stabilizing) support for type checking templates with Glint, and we are already doing a bunch of work to make that a first-class part of the ecosystem, but I didn't say anything about it one way or the other in the RFC. Whether we should is a reasonable question, and I'll bring it up with folks on the Ember TypeScript and Ember Framework teams to think about!

@chriskrycho
Copy link
Contributor Author

chriskrycho commented Mar 23, 2022

@joukevandermaas thanks again for that question – we talked about it at last week's Framework Core team meeting, and decided to intentionally decouple it from this RFC, but with two important qualifications:

  1. I'm going to add have added a note to that effect explicitly in this RFC, because it's super important: a7ed514

  2. I'm going to open have opened an "amendment" RFC to RFC: Official TypeScript support #724 stating our intent to support template type checking as part of the overall effort: Amend RFC 0724 to include template type checking #808

You should see both of those by the end of tomorrow (America time). Edit: or how about: now? 🎉


#### Primary packages

The primary packages in the ecosystem, `ember-source`, `ember-data`, and `ember-cli`, will adopt the [rolling window](https://github.com/chriskrycho/ember-rfcs/blob/semver-for-ts/text/0730-semver-for-ts.md#rolling-support-windows) policy:
Copy link
Member

@rwjblue rwjblue Mar 25, 2022

Choose a reason for hiding this comment

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

Need at least a few more packages here I think? @ember/test-helpers, @glimmer/component, @glimmer/tracking, @ember/test-waiters, ....etc?

Basically, packages that are very tied to ember-source should also follow the rolling policy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will update to that effect. The one thing I would say (that we should decide on together) is: some of those actually currently support more than Ember itself does, and for that reason I think it would be fine for them to adopt a “simple majors” policy with a caveat that they will never support less than Ember itself does.


<details><summary>On including <code>noUncheckedIndexedAccess</code></summary>

We include the `noUncheckedIndexedAccess` setting in our recommended settings because it catches one of the most significant holes in the TypeScript type system prior to its introduction: not accounting for the fact that "index"-style access to objects and arrays *always* allows arbitrary access to any index (numeric or string), whether or not it is defined. For example, consider this type (not recommended for *other* reasons, but legal):
Copy link
Member

Choose a reason for hiding this comment

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

Any specific reason this flag isn't part of strict: true? Seems like it absolutely "fits the bill" of maximal strictness...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You and I strongly agree. The short version is: the TS team makes judgment calls about when a given flag will break “too many” existing use cases, and chooses not to incorporate it into strict: true when they judge it will be too much of a problem, and that was the case here. It’s also possible that they might incorporate it into strict: true later, e.g. when they cut TS 5.0: they have started making bigger breaking changes at Actual Majors™ even though their explicit policy is that they can do breaking changes at minor versions.

Comment on lines +102 to +104
### Semantic Versioning

Ember packages which publish types will adhere to the [Semantic Versioning for TypeScript Types][rfc-0730] proposal. See that RFC for details on strategies for mitigating breaking changes from TypeScript, and see [**How We Teach This: Documenting SemVer**](#documenting-semver) below for discussion of how we will document this at the framework level.
Copy link
Member

Choose a reason for hiding this comment

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

What does it mean to have this RFC move forward before the underlying RFC's that are referenced? Should we add some prose somewhere here, to mention that the specifics might be modified by those RFCS, and if this RFC lands first then that those RFC's would update the prose accodingly...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good question. For this reason we should also move the SemVer RFC forward—I just forgot to bring it up the last few times we looked at advancing RFCs to FCP!

@rwjblue
Copy link
Member

rwjblue commented Mar 25, 2022

We discussed this in the core team meeting today, and are in favor of moving it into final comment period.

- In the`.eslintrc.js` blueprint:
- use `@typescript-eslint/parser` instead of the Babel ESLint parser
- include `@typescript-eslint` in the `plugins` key
- include `plugin:@typescript-eslint/recommended` in the `extends` key
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered also including @typescript-eslint/recommended-requiring-type-checking?

AFAIK the only drawback is performance. But given that ESLint runs as an external command, mostly in CI, and not anymore integrated into Ember's test infrastructure (using ember-cli-eslint back then), I would tend to prefer strictness over performance.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@simonihmig the Ember TypeScript team has not generally recommended using those. While it’s nice that they exist, they are (as you say) dramatically slower than regular ESLint rules, because they have to invoke the compiler. That’s a pretty big experiential difference for people, and it hasn’t (to date) been clear that it’s worth doing that out of the box, so we have avoided recommending them by default. (Also because it's quite easy to add them later if you want them!)

Copy link
Contributor

Choose a reason for hiding this comment

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

We strongly recommend you do use type-aware linting

That's a quote from the TS-ESLint docs above. So if you have good reasons that you wouldn't want to recommend them contrary to that statement, then I am fine with that. I would still want to use them whenever possible, as again I wouldn't mind the performance loss compared to the more robust code...

Also because it's quite easy to add them later if you want them!

Hm, I would disagree here. Yes, it's easy to add the rules later. But it can become quite hard to make your code comply with them, when you have already grown it over time not taking those rules into account.

When I look back at when we started to use TS, one of the painful mistakes was to start with a too loose setup (I believe it was the default config from e-c-ts back at the time, but no TS-ESLint), that fooled ourself into thinking the code was solid, while it was just not checked strict enough (e.g. implicit any's). Took us quite some effort to refactor it later. So I'd rather start with a too strict setup and then if really necessary loosen it where appropriate, than the other way around.

Choose a reason for hiding this comment

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

As a counter argument, I'd immediately turn this off (no matter how nice the errors are) because I don't think the perf impact is acceptable ¯\_(ツ)_/¯

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To clarify, I distinguish strongly between type checking, which should be as strict as possible, and linting, which has a lot more flexibility in terms of what makes sense as a baseline. Different teams and projects need different things there. I’ll also clarify: I really mean “it’s easy to add them at setup time yourself if you want them”—certainly adding lint rules later can be non-trivial amounts of work!

One last note: the default config from ember-cli-typescript has, as far as I remember, always included noImplicitAny: we’ve always tried to enable every possible strict flag by default, the only historical exception being strictFunctionTypes, which our attempts to work with mixin types and get/set could not handle. It’s quite possible you’re thinking of the ESLint no-explicit-any rule, though. I strongly recommend that one! Also it’s in the baseline and doesn’t require type checking. 😉

@chriskrycho
Copy link
Contributor Author

We talked about this again at today's Framework Team meeting and are merging it! 🎉

wagenet added a commit that referenced this pull request Aug 24, 2023
Advance RFC #800 "Typescript Adoption Plan" to Stage Ready for Release
wagenet added a commit that referenced this pull request Aug 24, 2023
Advance RFC #800 `"TypeScript Adoption Plan"` to Stage Released
ef4 added a commit that referenced this pull request Sep 15, 2023
Advance RFC #800 `"TypeScript Adoption Plan"` to Stage Recommended
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Final Comment Period T-ember-cli RFCs that impact the ember-cli library T-ember-data RFCs that impact the ember-data library T-framework RFCs that impact the ember.js library T-learning
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants