xaviergonz / mobx-keystone

A MobX powered state management solution based on data trees with first class support for Typescript, support for snapshots, patches and much more
https://mobx-keystone.js.org
MIT License
549 stars 25 forks source link

Better type checking on model constructor #178

Closed Amareis closed 4 years ago

Amareis commented 4 years ago

https://codesandbox.io/s/mobx-keystone-typings-yo2rs?file=/src/index.tsx:0-179

Currently there no ts error if some object with exta fields is passed to model constructor, but there is runtime error, which even don't report about these extra fields. It's not really good when model created with some external type (such as server data), I spent hour, trying to understand about it.

Amareis commented 4 years ago

There is some tricks to do so https://fettblog.eu/typescript-match-the-exact-object-shape/ But it's not working with keystone, because ModelInstanceCreationData<Mod> has no exact shape.

xaviergonz commented 4 years ago

Blame typescript :/

function f(data: { value: number }) { }

const tt = { value: 0, a: 1 }
f(tt) // ok

f({value: 0, a: 1}) // error, a should not be there

same happens if you use new with an object vs new with a literal. Maybe you could try to type t

const t: SnapshotInOf<Mod> = { value: 0, a: 1 }; // a should not be there
new Mod(t); //runtime error

Edit: Thanks for the heads up, I'll check the article and see if I can use that :D

Amareis commented 4 years ago

I think the runtime error should be more specific, for pure js users it will be helpful.

Also maybe there is some ts trick to make compilation error more detailed (not just turn arg into never). I try to dig into it.

Amareis commented 4 years ago

Maybe you could try to type t

It's not really working for me, actually, in WebStorm:

const m: ModelInstanceCreationData<Message> = {}  //m has any type
Amareis commented 4 years ago

If open BaseModel.d.ts in codesandbox, there is ts errors:

A computed property name in a class property declaration must refer to an expression whose type is a literal type or a 'unique symbol' type.ts(1166)
Cannot find name 'propsDataTypeSymbol'.ts(2304)

image

Amareis commented 4 years ago

Also, in ts 3.9 there is ts-expect-error feature, it can be really good for typings tests!

xaviergonz commented 4 years ago

It's not really working for me, actually, in WebStorm:

If open BaseModel.d.ts in codesandbox, there is ts errors:

Could you update to 0.45.5 and then restart your ide?

Amareis commented 4 years ago

Oh, that's sweet! All is working now and ts even catch some errors.

Amareis commented 4 years ago
@model("Mod")
class Mod extends Model({
    value: tProp(types.number)
}) {}

type Exact<T, Shape> = T extends Shape ? Exclude<keyof T, keyof Shape> extends never ? T : Shape & { [K in Exclude<keyof T, keyof Shape>]: never } : Shape

function create<T>(data: Exact<T, ModelInstanceCreationData<Mod>>) {
    return new Mod(data as any)
}

const t = { value: 0, a: 1 }

create(t) //compilation error

new Mod(t); //runtime error
TS2345: Argument of type '{ value: number; a: number; }' is not assignable to parameter of type '{ value: number; } & { a: never; }'.
   Type '{ value: number; a: number; }' is not assignable to type '{ a: never; }'.
     Types of property 'a' are incompatible.
       Type 'number' is not assignable to type 'never'.

Edited: better version

xaviergonz commented 4 years ago

it works as a function, but it is kind of hard to make it work within a constructor (since apparently constructors do not support generic types in TS) :-/

xaviergonz commented 4 years ago

I think the best thing that could be done is provide a .createExact function (or similar) within the type whenever that checking is wanted e.g.

const t = {value: 0, a: 1}
const m = Mod.createExact(t) // error about a

would that be good enough?

terrysahaidak commented 4 years ago

is this similar to create in mst?

Amareis commented 4 years ago

Yep, that's really not easy in ts. My better assumption is:

interface Type<T> {
    $$identityType: T;
}

declare function numberType(): Type<number>

type Props = {
    [key: string]: Type<number>
}

type PropsData<T extends Props> = {[K in keyof T]: T[K] extends Type<infer S> ? S : never}

type Exact<T, Shape> = T extends Shape ? Exclude<keyof T, keyof Shape> extends never ? T : Shape & { [K in Exclude<keyof T, keyof Shape>]: never } : Shape

interface ICheckedModel<T extends Props> {
    new<K=PropsData<T>>(data: Exact<K, PropsData<T>>): CheckedModel
    //  ^ K has default value
}

abstract class CheckedModel {}

declare function make<T extends Props>(props: T): ICheckedModel<T>

class MD<T> extends make({
    value: numberType(),
})<T> { //                 <----  notice that type parameter
}

new MD(t) //compilation error!

It's working, but there is some additional typings required. It can be implemented as different function or added to existing Model declaration. Since K has default value, without explicit type parameter in superclass expression, old behavior is preserved.

Amareis commented 4 years ago

But, maybe better is just allow excess properties? Or maybe make it configurable, just as with typechecking. I don't think this make any problems.

xaviergonz commented 4 years ago

Yeah, I think it is better to just allow/ignore excess properties when using typed objects.

Amareis commented 4 years ago

Thank you!