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

Support @types, preview types, and stable types for Ember.js #1389

Merged
merged 36 commits into from
Jun 30, 2023

Conversation

gitKrystan
Copy link
Collaborator

@gitKrystan gitKrystan commented May 12, 2023

Requires #1405

I was getting all sorts of tsc issues on canary builds for an addon so took a stab at resolving them. This is a bit hack-y just to identify the issues.

Currently working to reconcile these types w/ existing preview types, as they are incompatible.

@gitKrystan gitKrystan changed the title WIP: Fix types for ember-source canary WIP: Fix types for ember-source 5.1 Jun 6, 2023
@gitKrystan gitKrystan mentioned this pull request Jun 6, 2023
gitKrystan added a commit to gitKrystan/ember-test-helpers that referenced this pull request Jun 6, 2023
Note that there are still issues with types support for Ember 5.1, which will emberjs#1389 will address.
ef4 added a commit to embroider-build/embroider that referenced this pull request Jun 27, 2023
ember-source 5.1 is out but it cannot typecheck correctly in an apps yet due to:

 - emberjs/ember.js#20479
 - emberjs/ember-test-helpers#1389
This introduces an intermediate module for the two `Owner`-related
proxy mixins, allowing us to resolve the types for them from the
various locations where they have lived on DT and in Ember. This is not
great, but isolates the complexity there in a single place where we can
later remove it, and makes it so consuming code can be unaware of it.
@ef4
Copy link
Contributor

ef4 commented Jun 30, 2023

Thanks for working on this!

These are unfortunately difficult/impossible to fully backport into 4.8
so just ts-ignore them for now. This is, to say the least, annoying, but
it will get the job done.
@chriskrycho chriskrycho changed the title WIP: Fix types for ember-source 5.1 Support @types, preview types, and stable types for Ember.js Jun 30, 2023
Comment on lines +74 to +75
// SAFETY: this is always present, but only the stable type definitions from
// Ember actually preserve it, since it is private API.
Copy link
Contributor

Choose a reason for hiding this comment

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

For reference: we cannot ts-expect-error this because it actually works on the stable types. 😂

},
export default function buildRegistry(resolver: Resolver) {
let namespace = new Application();
// @ts-ignore: this is actually the correcct type, but there was a typo in
Copy link
Contributor

Choose a reason for hiding this comment

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

Similarly, many of these work correctly… on some versions of Ember. I used ts-ignore here and below to make it so that if there is an error, it is suppressed :sigh: but if not, it still has type safety.

@chriskrycho chriskrycho merged commit 00db5bd into emberjs:master Jun 30, 2023

// Imports from `@types`
// @ts-ignore
import type CPM_DTS from '@ember/engine/-private/container-proxy-mixin.d.ts';
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Can you ELI5 this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Roughly, it imports from all the different paths where this type can exist, using ts-ignore on all of them to make the ones which are not present on any given Ember types version still pass the type checking, and then go through these type shenanigans so that it only resolves the one that actually does exist on the current version. The net result is that whether you are using the Definitely Typed types, the 4:8 preview types, the 4.12 preview types, or the stable types, one of these will be available and the others will not, and these exports will correctly resolve only that one. If you don’t do the type level shenanigans, you end up mixing any in (since that is the type of a ts-ignored import which has no type definition at that module path), which is definitely not what we want.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants