I never fully wrapped my head around how the previous approach was working here, but I think this is backwards now? It looks like the existing tests for create aren't super extensive, but given how the Getters wrapper moved from the source to the return value in e.g. the signature of get, I think I would expect a similar change here?

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

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

I.e. the instance type of the this value here can have ComputedProperty properties, but arg1 shouldn't, right?

With TS 2.8 I think we can declare it more simply:

static create<Class extends typeof Ember.CoreObject, Arg1>(
    this: Class,
    arg1: Arg1 & ThisType<Arg1 & InstanceType<Class>>
): InstanceType<Class> & Arg1;

I'll call out a couple of things that are missing compared to the original declaration.

• EmberInstanceArguments<T> verified that the arguments passed to create() matched the types on the class. E.g. Object.extend({ x: 42 }).create({ x: 'foo' }) would not typecheck (although it does work at runtime-- x is a string).

• Fix<T> worked around Implement typed computed property access typed-ember/ember-typings#32. I suspect it's no longer needed.

I think it would be useful for us to take a pass at create and the associated code as a follow-on effort and see what we can clean up there (as well as fleshing out the test coverage).

In the meantime, though, hopefully there's a low-lift way to keep things working roughly as-is with UnwrapComputedPropertyGetters to get this PR landed?

In the future I also want us to stop &ing InstanceType<Class> & Arg1. I don't believe collisions are handled properly.

{ // InstanceType<Class>
   foo: string
} & { // .create args
   foo: number
} = {
  foo: string & number
}

it should be 

{
   foo: number
}

Maybe something like

type Mix<A, B> = B & Pick<A, Exclude<keyof A, keyof B>>;

example

Agreed that &ing together the types is incorrect there (and doubtless there are other places we're doing that as well that should be fixed), but shouldn't code like this be a type error regardless?

const MyClass = EmberObject.extend({
  foo: 'hello'
});
MyClass.create({ foo: 5 });

Seems like (roughly) the moral equivalent of

class MyClass {
  constructor(public foo: string = 'hello') {}
}
new MyClass(5);

@mike-north I'm not positive I'm in a better position than you are w.r.t. the current mechanics of create, but I'm happy to take a pass later today and see if I can get the tests passing if @dwickern doesn't beat me to it.

We can add the EmberInstanceArguments constraint back to my previous example.

static create<
  Class extends typeof Ember.CoreObject,
  Arg1 extends EmberInstanceArguments<UnwrapComputedPropertySetters<InstanceType<Class>>>
>(
    this: Class,
    arg1: Arg1 & ThisType<Arg1 & InstanceType<Class>>
): InstanceType<Class> & Arg1;

Should Person.create({ fullName: 'string' }) be allowed?

Unless it complicates the typing significantly, I believe it should, as long as the Set type on the computed is compatible. It's a little weird in this specific example since the computed isn't actually set up to work with set, but that's not currently something we try to capture in the type system (though authors could do that explicitly with Computed<string, never>)

I'll give you both commit access to my fork. Unfortunately, I'm out of time to spend on this for a few days

The Ember.Object.create issue is now resolved. Positive and negative test cases added to reflect this for 0-4 arguments passed to .create

What is this for? If it's needed, could you add a comment describing why?

UnwrapComputedPropertyGetters should be wrapping the return type here rather than obj, shouldn't it?

I think this is another inversion.

function set<T, K extends keyof T, V extends UnwrapComputedPropertySetter<T[K]>>(
  obj: t,
  key: K,
  value: V
): V;

Can you guys contribute some test cases that validate the correct behavior?

@dfreeman I don't believe that this is correct. Current tests fail w/ your suggested implementation, and pass with the current code

It seems to work either way 🤷‍♂️

Would it be more correct for set to return the get type?

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

^ this works

❤️

Is there a story on these commented type assertions? Based on the assertions in test/create.ts I'd expect p4.get('fullName') and p4.fullName both to have type number, but I'm not sure what the aim is here.

The line you're commenting on is the interesting. Currently (and in prior releases of these types) the type of fullName is string && ComputedProperty<string, string>.

This is not an accurate representation of what's happening with Ember.Object.create and Ember.Object.extend -- these methods align with the behavior of Object.assign where target object properties are usually replaced.

In this case, the value should just be string. We'll resolve this when we apply the change described here as a future enhancement.