Ember-cli-typescript: [@types/ember bug] - ember types are broken in TypeScript 3.1

[mike-north]

Which package(s) does this problem pertain to?

• [x] @types/ember
• [x] @types/ember-data

What are instructions we can follow to reproduce the issue?

npm install -g typescript@latest
tsc -v      # make sure it's a v3.x (i.e. Version 3.1.0-dev.20180810)
git clone https://github.com/DefinitelyTyped/DefinitelyTyped
cd DefinitelyTyped
npm install
cd types/ember
tsc

Now about that bug. What did you expect to see?

I expected tsc to compile the ambient types and test files successfully

What happened instead?

test/array-ext.ts:16:32 - error TS2345: Argument of type 'ComputedProperty<Fix<Person & { name: string; }> | undefined, Fix<Person & { name: string; }> | undefined>'is not assignable to parameter of type 'Person'.
  Property 'name' is missing in type 'ComputedProperty<Fix<Person & { name: string; }> | undefined, Fix<Person & { name: string; }> | undefined>'.

16 assertType<Person | undefined>(array.get('firstObject'));

NOTE

Switching to a TypeScript 2.x

npm install -g typescript@2
tsc

will compile the tests successfully and without error.

[mike-north]

Interestingly, TS 3.0.1 appears to work, while 3.1.0-dev breaks our types

[jamescdavis]

I've also tripped on this: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/28114

Specifically: https://travis-ci.org/DefinitelyTyped/DefinitelyTyped/builds/415969505#L3391

[mike-north]

This issue is blocking any new changes to @types/ember. In a few days if nobody has addressed this, I will start to work on a fix.

My fix may involve walking back a significant amount of the existing computed property and Fix<T> stuff (depending on what's actually breaking, and what needs to change).

[BryanCrotaz]

I looked at fixing it on #27993 but it's way beyond my basic TS skills.

[mike-north]

@BryanCrotaz this is why I'm giving the original authors a chance to jump in and fix it before I try a more heavy-handed solution.

There is some significant complexity around the current implementation of mixins/computed properties/Ember.get, and my approach may end up being a delete/rewrite operation for those parts of the code. Obviously this is a non-trivial effort, but with very little documentation around the original intent and strategy, it may be the most reliable way to refactor toward a fix. Normally I wouldn't jump in to do this, but it's a master branch is broken kind of situation that blocks forward progress.

[chriskrycho]

@mike-north one option to consider here, along with some of the other pieces we've discussed, is taking this as an opportunity to make a breaking change which jumps minimum supported TS version—in line with our official policy which is "(at least) latest two versions" we could bump to 2.8 and start leaning on conditional types. My and @dfreeman's initial experiments when 2.8 came out suggested we could pretty simplify things quite a bit around computed properties and getters using conditional types and infer. Happy to talk about it a bit more this coming week.

[dwickern]

My brief testing suggests that TS 2.8 infer will fix this problem. I ran into some issues integrating it which I think we can overcome. If we could also simplify the types, that would be wonderful.

[mike-north]

Sounds like this is something that has already been brought to @andy-ms's attention
https://github.com/Microsoft/TypeScript/issues/26120
After poking around a bit, it very much seems like the change which may have broken our types ( https://github.com/Microsoft/TypeScript/pull/26063 ) is the likely culprit

First, all recent versions of TS agree on this

([] as Person[]).firstObject // regarded as ComputedProperty<Person, Person> ✅

Where things get interesting is the type of the .get function

TypeScript 3.0.1

Ember.Observable.get<{
    length: number;
    toString: any;
    toLocaleString: any;
    pop: any;
    push: any;
    concat: any;
    join: any;
    reverse: any;
    shift: any;
    slice: any;
    sort: any;
    splice: any;
    unshift: any;
    indexOf: any;
    lastIndexOf: any;
    every: any;
    ... 74 more ...;
    frozenCopy: any;
}, "firstObject">(this: ComputedPropertyGetters<...>, key: "firstObject"): Person | undefined

TypeScript 3.1.0-dev.20180821

Ember.Observable.get<Person[], "firstObject">(this: (Person | Ember.ComputedProperty<Person, any> | ComputedProperty<Person, any>)[], key: "firstObject"): Ember.ComputedProperty<Person | undefined, Person | undefined>

If you look at the this: for each of the two types above, I believe what we're seeing is that when the array is used with the ComputedPropertyGetters<T> mapping type, we used to get

// ComputedPropertyGetters<Person[]> ⤵
Ember.ComputedProperty<Person[], any> | ModuleComputed<Person[], any> | Person[]

and we now get

// ComputedPropertyGetters<Person[]> ⤵
(Ember.ComputedProperty<Person, any> | ModuleComputed<Person, any> | Person)[]

@dwickern I don't see how infer will help us here, unless you're using it to substantially change the way computed properties are described. AFAIK what we're seeing here is an intentional result of https://github.com/Microsoft/TypeScript/pull/26063 and the new way that arrays pass through mapping types.

[dwickern]

Before TS 2.8 this was the only way to pull the underlying T out of a ComputedProperty<T>:

type ComputedPropertyGetters<T> = { [K in keyof T]: ComputedProperty<T[K]> | T[K] };

function get<T, K extends keyof T>(obj: ComputedPropertyGetters<T>, key: K): T[K];

It was not very reliable before (Microsoft/typescript#20280) and now there is this new interaction with arrays and unions (Microsoft/TypeScript#26120). The equivalent implementation using infer doesn't have these problems:

type UnwrapComputedPropertyGetter<T> = T extends ComputedProperty<infer U> ? U : T;

function get<T, K extends keyof T>(obj: T, key: K): UnwrapComputedPropertyGetter<T[K]>;

I pushed my WIP to a branch here:
https://github.com/DefinitelyTyped/DefinitelyTyped/compare/master...dwickern:ember-infer

[mike-north]

@dwickern This context from your original intent is super helpful. I believe that I have a working fix built on top of your branch. I just need to propagate everything through and add some more test cases.

[mike-north]

I ran into an interesting situation where it looks like TypeScript may be discarding some information.

Currently our Ember.ComputedProperty type makes no use of the generics it receives. I'm guessing that the original intent was to sort of "tag" the CPs with information that can be extracted later.

If we simplify this to the typical Boxed type

// kind of like our Ember.ComputedProperty
type Box<T, U = T> = { value: any };
// a ComputedProperty<string> that's clobbered with a string via .extend() or .create()
type MaybeBoxed<T> = Box<T> & T;

// kind of like @dwickern's approach to getting the value type from
//   something that **might** be a CP (or might not)
type UnBox<S> = S extends Box<infer T> ? T : S;

// a concrete example for us to think about
type MaybeBoxedString = Box<string> & string;

type Unboxed = UnBox<MaybeBoxedString>; // {} 🚨  -- would be nice if it were a string!

This problem seems to be solved by using any generic anywhere

type Box<T, U = T> = { value: any, __do_not_touch_this: T };
type MaybeBoxed<T> = Box<T> & T;

type UnBox<S> = S extends Box<infer T> ? T : S;

type MaybeBoxedString = Box<string> & string;

type Unboxed = UnBox<MaybeBoxedString>; // string ✅

Thankfully the type in question is a class, so we can use a private field for this and no harm is done

[dwickern]

Ah, yes, the generic types need to be used somewhere. Otherwise a ComputedProperty<foo> is structurally identical to a ComputedProperty<bar>. I'm surprised it ever worked.

[chriskrycho]

Ahhhh, that's why that was falling over that way. I had never gotten far enough down the rabbit hole to figure it out, but it drove nuts when I first spiked this with infer.

[mike-north]

PR opened ~, but I'm having some trouble getting the DefinitelyTyped tests to regard TS 2.8 as our new minimum supported version. Locally, I get stuck with some eslint rule blowing up.~

[mike-north]

Fix for a variety of ember types has been merged