[@types/ember bug] - Ember.set doesn't work w/ ES6 classes

[mike-north]

trafficstars

Which package(s) does this problem pertain to?

• [x] @types/ember

What are instructions we can follow to reproduce the issue?

Failing test case

class Foo extends Ember.Object.extend({
    a: Ember.computed({
        get() { return ''; },
        set(key: string, newVal: string) { return ''; }
    })
}) {
    b = 5;
    baz() {
        let y = this.b; // $ExpectType number
        let z = this.a; // $ExpectType ComputedProperty<string, string>
        this.b = 10;
        this.get('b').toFixed(4); // $ExpectType string
        this.set('a', 'abc').split(','); // 💥 $ExpectType string[]
        this.set('b', 10).toFixed(4); // 💥 $ExpectType string

        this.setProperties({ b: 11 }); // 💥
        // this.setProperties({ b: '11' }); // 💥 $ExpectError
        this.setProperties({ // 💥
            a: 'def',
            b: 11
        });
    }
}

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

I expected to be able to set properties on my ember objects with no type errors

[mike-north]

This has something to do with the new UnwrapComputedPropertySetters stuff breaking when combined with pulling types off of this. I spent several hours digging into this, and feel like I'm at a 1 !== 1 situation.

@dfreeman @dwickern @jamescdavis If you could poke at this sometime soon, I would love to find a near-term solution that doesn't involve weakening the types -- I just have run out of time trying to figure it out.

Until we can arrive at some root cause and fix it, I'll add the failing test case above and relax the types around this.set a bit, so we're no longer blocking anyone.

It's clear, one factor that contributed to this problem making it into a release is very thin test coverage in some areas. Part of my debugging process involved creating dozens of new tests -- particularly around some of the tricker areas of the types (computed properties, Ember.Object.create, Ember.Object.extend, the Observable interface, etc...`)

[mike-north]

The PR related to this issue was just merged. This means we no longer have type safety when using set() with settable computed properties.

[dfreeman]

I spent a little time on this this evening and I think either I have a fundamental misunderstanding of some aspect of the type system or there's a bug in the compiler around this types. I would expect this code to typecheck, but it doesn't:

declare const brand: unique symbol;

type Wrapped<T> = { [brand]: T };
type Unwrap<T> = T extends Wrapped<infer U> ? U : T;

declare function set<T, K extends keyof T>(obj: T, key: K, value: Unwrap<T[K]>);

class Foo {
    prop: Wrapped<string>;

    method() {
        // Argument of type '"hi"' is not assignable to parameter
        // of type 'Unwrap<this["prop"]>'.
        set(this, 'prop', 'hi');
    }
}

// Works as expected
set(new Foo(), 'prop', 'hi');

[ef4]

If I understand the situation correctly, it's not only:

we no longer have type safety when using set() with settable computed properties.

Rather, we no longer have type safety when using set at all, even if no computed properties are involved:

class Example extends Controller {
  title = "hello";
  anAction() {
    this.set('title', 42); // <- should be a type error: found number, expected string
  }
}

Removing this line fixes my example, but I don't know what else it would break.

[mike-north]

@ef4 which version of the types, and which version of typescript are you using? this and keyof this are areas of the types that are extremely sensitive to breaking compiler changes -- I need to dig in and figure out if this is a new problem

[ef4]

@types/ember 3.1.0 and typescript 3.5.2.

[chriskrycho]

As far as I can tell, this remains an issue on the TS side, but is largely irrelevant since every use of this.get() and this.set() in Ember can be safely replaced with a normal ES property access or assignment statement, including in Ember Data. Closing as “not planned” accordingly.