[ember] Make @ember/* modules the source of truth rather than the Ember namespace

[dfreeman]

Overview

This PR refactors @types/ember to use the RFC 176 modules as the source of truth for types, re-exporting them from the Ember namespace rather than the other way around. It's not going to be super fun to review.

This arrangement more closely reflects how Ember's own codebase is organized, and in the future as those packages are actually published independently, it will make it easier to either shift these into independent @types packages or to just remove them if the corresponding @ember package is published with types of its own.

It also has the nice side effect of no longer requiring us to create fake subclasses for better tooling support (e.g. export default class Controller extends Ember.Controller {}), which eliminates some weirdness we had with things like Ember.ComputedProperty<T> | ModuleComputed<T>.

Other Notable Changes

• run.next, computed.alias, etc are no longer accepted by the type system for the module-import forms of run and computed. These happen to work today because of the way the modules are shimmed at runtime, but strictly run and computed are only functions in the RFC 176 world, and using them as namespaces could break at any time, so the general consensus was to go ahead and update the types
• For reasons that aren't 100% clear to me, TS 2.4 and 2.5 have some trouble with this refactored version. 2.6-2.8, as well as rc and next, have no trouble, which is in line with our tentative "last two versions maybe?" support thinking (and ember-cli-typescript only works with 2.7+ anyway). This necessitated bumping the minimum on @types/ember and all other DT packages that depend on it to 2.6.
• The ember-data typings needed to be updated to add the store property to the default module exports rather than the Ember.* members.
• I bit the bullet and added myself as an author to @types/ember

Testing

I've checked a few small projects of my own, as well as ember-osf-web, with these definitions, and everything appears to work as expected. That said, I'd very much appreciate additional checks in other established private projects as folks have the time ❤️

With this module-sourced-types branch pulled locally, I ran this in the projects I was testing to link the updated definitions into place:

for project in ember ember-data; do
  cp $PATH_TO_DT/types/$project/index.d.ts node_modules/@types/$project
done

The PR Checklist

• Use a meaningful title for the pull request. Include the name of the package modified.
• Test the change in your own code. (Compile and run.)
• Add or edit tests to reflect the change. (Run with npm test.)
• Follow the advice from the readme.
• Avoid common mistakes.
• Run npm run lint package-name (or tsc if no tslint.json is present).

[typescript-bot]

@dfreeman Thank you for submitting this PR!

🔔 @jedmao @bttf @dwickern @chriskrycho @theroncross @mfeckie @alexlafroscia @mike-north @tansongyang - please review this PR in the next few days. Be sure to explicitly select Approve or Request Changes in the GitHub UI so I know what's going on.

If no reviewer appears after a week, a DefinitelyTyped maintainer will review the PR instead.

[typescript-bot]

After 5 days, no one has reviewed the PR 😞. A maintainer will be reviewing the PR in the next few days and will either merge it or request revisions. Thank you for your patience!

[dfreeman]

For whichever maintainer comes along to review this — I'm happy to hold off and let some of the tagged folks take this for a spin. It's a big refactor, so getting actual burn in in real-world code bases is going to be important.

[armanio123]

Sounds good @dfreeman. For any of the reviewers (@jedmao @bttf @dwickern @chriskrycho @theroncross @mfeckie @alexlafroscia @mike-north @tansongyang), could you please take a look and let me know if this is ready for merge?

[typescript-bot]

@dfreeman Unfortunately, this pull request currently has a merge conflict 😥. Please update your PR branch to be up-to-date with respect to master. Have a nice day!

[paulvanbrenk]

I clearly don't have enough context to give meaningful feedback about this change.

@jedmao @bttf @dwickern @chriskrycho @theroncross @mfeckie @alexlafroscia @mike-north @tansongyang, could you please take a look and let me know if this is ready for merge?

[dfreeman]

I know @chriskrycho has been on vacation recently but hopefully he'll have a chance to give this a look soon?

[chriskrycho]

First vacation and then two weeks (now into the third) of a really intense push at work. I'm planning to look at these not later than Thursday afternoon, when I have a work-approved, dedicated block of time to go after OSS stuff!

[paulvanbrenk]

No problem, just making sure this is still on somebody's radar.

[dfreeman]

😭 another merge conflict...

[mike-north]

I hate to ask these kinds of questions after someone has put in such a huge amount of work, but we should seriously consider:

• Do the tangible benefits of a refactor of this magnitude offset the risks?
  • If the answer is yes, can we enumerate specific tangible benefits consumers will experience? (i.e., XYZ type was broken in a particular scenario and is now fixed)
• Is it wise to do it all at once in a 6500 line code change?
• What effects will this have on modifications that consumers have made to ember type definitions? (i.e., if they extended a type defined in the 'ember' module and then consumed it via the '@ember' equivalent -- will their changes still apply and in the same way now that the source of truth has changed?)
• The current version of ember-cli-typescript requires TS 2.7, but this type information could still be used with TS 2.4 and an older version of the ember addon. This PR effectively ends support for those users. Is this something we want to do? In SemVer, this would constitute a major version bump. -- Addressed by @chriskrycho here

As teams start to adopt a TypeScript+Ember stack, we must place a high value on stability. It's almost impossible to effectively review a PR of this magnitude and nature (specific challenge here -- this type information is decoupled from the framework it aims to describe).

If this broad reorganization is something we feel it's important to take on (and if now is the time to do it), I'd like us to consider mirroring the "stability without stagnation" approach taken for the RFC 176 work in ember its self

• substantial bolstering of tests (including adequate coverage for use cases involving consumption via namespace AND @ember modules), followed by
• careful, incremental movement of the "source of truth" out of the namespace and into @ember modules

[chriskrycho]

Really important things you raise, @mike-north.

Stability/churn considerations

The first thing to note is our previous discussion of these questions. The core group working on the Ember TypeScript bits came to the broad conclusions there that (1) we will not treat updating our supported version of TypeScript as a breaking change; and (2) that we will actively support the latest two versions of TypeScript.

You can see the reasoning in more detail there, but the short version is that in general upgrading the TS compiler version is not a breaking change without also changing strictness settings; and tracking relatively closely lets us use new features that make the type definition experience substantially better for users – e.g. 2.8's conditional types and the upcoming unknown and variadic type argument changes in 3.0.

Thus, to these points:

The current version of ember-cli-typescript requires TS 2.7, but this type information could still be used with TS 2.4 and an older version of the ember addon. This PR effectively ends support for those users. Is this something we want to do? In SemVer, this would constitute a major version bump.

As teams start to adopt a TypeScript+Ember stack, we must place a high value on stability. It's almost impossible to effectively review a PR of this magnitude and nature (specific challenge here -- this type information is decoupled from the framework it aims to describe).

Stability is indeed very important! That said, the best bet for real stability-without-stagnation in Ember's type definitions is for the ongoing projects of converting Ember to TypeScript and truly modularizing the Ember source to land: we will get all of these things essentially for free.

In the meantime, we are of course not going to go out of our way to break backwards compatibility of TS versions on the type definitions, and we'll always support the minimum we reasonably can.

However, if there's a good reason to make a change, we will! For example: we will rewrite some of our types using conditional types in the relatively near future: it will make for much better type inference in important parts of the application, and will in fact be necessary to properly type some of the world in a post-Ember 3.1 world.

Specific thoughts on this PR

I think the biggest consideration I have here is coverage – we don't have the type coverage, and don't have an app we know exercises everything – so there is some risk of something having gotten missed along the way.

(I was finally just able to pull it down and test it in my app, and ran into a problem with cycles in the DS.Model registry which I haven't encountered in the existing version of the types. Switching to classes + decorators will solve that for me, but it does appear to be a new issue – though it may actually exist earlier than this change; I'm on @types/ember^2.8.22 and @types/ember-data^2.14.16; the latest is 2.8.29 and 2.14.17 respectively.)

The flip side is that this is a major boon to working on these types, not least insofar as we could actually start breaking it up into individual files with this in place.

I think my overall take is roughly: @dfreeman, can we close this PR, but keep the branch around so we can use it as the baseline for an incremental transition – adding tests and validating against real-world apps that @mike-north suggests? That seems like the best way to avoid having wasted this work, if it's a viable approach.

[typescript-bot]

@dfreeman I haven't seen anything from you in a while and this PR currently has problems that prevent it from being merged. The PR will be closed tomorrow if there aren't new commits to fix the issues.

[dfreeman]

Thanks for the feedback, @mike-north and @chriskrycho, and apologies for the slow response—I've been traveling (and will continue to be for the next couple weeks). I'm going to go ahead and close this out, but I'll address a couple of the points raised.

• Do the tangible benefits of a refactor of this magnitude offset the risks?
  • If the answer is yes, can we enumerate specific tangible benefits consumers will experience? (i.e., XYZ type was broken in a particular scenario and is now fixed)
• Is it wise to do it all at once in a 6500 line code change?
• What effects will this have on modifications that consumers have made to ember type definitions? (i.e., if they extended a type defined in the 'ember' module and then consumed it via the '@ember' equivalent -- will their changes still apply and in the same way now that the source of truth has changed?)

I think the answers to these three questions are bound up with one another. Part of the issue at hand is that the structure of the type definitions doesn't align with the way Ember today is taught or internally structured. This makes the already-complex typings more imposing to someone trying to understand how they work or why some particular expression isn't typechecking, and it's a layer of mental overhead for the maintainers of the types as well.

Currently in the module declarations, we're exporting empty subclasses (e.g. export default class Controller extends Ember.Controller {}) in order to get better tooling support for things like auto-imports. This works well for the most part, since in principle those types should be identical to the source ones, but it's nevertheless introduced some peculiarities elsewhere in the definitions, with workarounds where we need to reference both the namespaced and the module-sourced versions, like:

    type ComputedPropertyGetters<T> = { [K in keyof T]: Ember.ComputedProperty<T[K], any> | ModuleComputed<T[K], any> | T[K] };
    type ComputedPropertySetters<T> = { [K in keyof T]: Ember.ComputedProperty<any, T[K]> | ModuleComputed<any, T[K]> | T[K] };

These kinds of workarounds aren't necessary with modules as the source of truth, as we no longer need the extra layer of subclassing for editors to 'do the right thing' — this eliminates a whole class of possible bugs in our own typings.

All that said, this change would break type augmentations that folks have written against Ember.*, and I haven't been able to come up with a way around that — as far as I've been able to discover, we have to make a choice as to whether the augmentation can be done on the namespaced classes or the module-sourced ones. We're in a sort of mixed world today, with entities that only exist at the type level (like ServiceRegistry) being augmented via a module import, but runtime entities (like Service) being augmented via the Ember namespace.

If we deliver this change incrementally, we're essentially breaking it into a series of smaller breaking changes and leaving type augmentation in an even more inconsistent state until it's fully rolled out. If we were to bolster the tests and then go with a big-bang release, that might be a good time to bump the @types/ember package version to 3, since we're pretty drastically out of sync with Ember's own versioning right now anyway.

can we close this PR, but keep the branch around so we can use it as the baseline for an incremental transition

I'll keep the branch around, and whoever's motivated is welcome to use it as inspiration, but it has an incredibly short shelf life without someone actively rolling in changes from other PRs (you can see it's already got conflicts again). I know @dwickern was also a proponent of this shift—maybe he'll have the chance pick it up.

[paulvanbrenk]

My $0.02..

I would definitely try to target Ember 3.x with these changes, since that's already a major version increase breaking changes should be expected. Further @types does have the ability to host major versions Side-by-side, potentially reducing the impact of the changes.