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

Inheritance Issues #109

Closed tomitrescak closed 4 years ago

tomitrescak commented 4 years ago

Hello, first, let me thank you for AWESOME package. Made my life so muche easier. I just have and issue with inheritance from Generic Models, it seems to short circuit something in Typescript. Example:

@model('my/BaseModel')
export class BaseModel extends Model({
  items: prop<string[]>(() => []),
}) {}

@model('my/ChildModel')
export class ChildModel extends ExtendedModel(BaseModel, {}) {
   foo() {
      this.items.push(); // <- LIFE IS GOOD, this.items is available
   }
}

This stops working, the only deifference is adding the generic parameter to base class name

@model('my/BaseModel')
export class BaseModel<T> extends Model({
  items: prop<string[]>(() => []),
}) {}

@model('my/ChildModel')
export class ChildModel extends ExtendedModel(BaseModel, {}) {
   foo() {
      this.items.push(); // <- LIFE IS BAD, this.items is NOT available
   }
}
Amareis commented 4 years ago

Hey-hey, there is section about inheritance in docs: https://mobx-keystone.js.org/models#inheritance Just look at very bottom line.

xaviergonz commented 4 years ago

Hi, sadly you need to use a factory pattern so typescript gets along with generics on a base model. You can see an example here https://mobx-keystone.js.org/models#factory-pattern

Anyway I'll take a look to see if it that particular use case can be simplified somehow

tomitrescak commented 4 years ago

@xaviergonz thanks! I was exploring the factory pattern, but it brings unnecessary overhead with creating whole inheritance hierarchies for each generic instance. I managed to do this but removing inheritance, and using interfaces. It led to somE duplicate code calls, but nothing too terrible. Still, would be fantastic if we could make this work.

xaviergonz commented 4 years ago

After this PR (https://github.com/xaviergonz/mobx-keystone/pull/111) is merged it should be easier to do inheritance with abstract / generic base classes.

For abstract base classes it will just work now (before abstractModelClass had to be used) For generic base classes it will work (although it will assume the generics to be unknown).

If you want to extend a generic class, then you may want to use modelClass in order to specify the exact generic like this:

class X extends ExtendedModel(modelClass<SomeGenericClass<string>>(SomeGenericClass), { ... }) { ... }
xaviergonz commented 4 years ago

Released as v0.37.0

tomitrescak commented 4 years ago

Amazing! Can’t wait to try this;) will refactor my code afterwards and get rid of some boilerplate interfaces I had to use to make it work previously :) thanks for your support!

xaviergonz commented 4 years ago

Just a note of caution, if you want to share the generic from the child class to the base class then the factory pattern is still needed (I just raised a TS proposal to address this: https://github.com/microsoft/TypeScript/issues/36406 )

So this still won't work:

// note how T from X is reused in SomeGenericClass<T>
class X<T> extends ExtendedModel(modelClass<SomeGenericClass<T>>(SomeGenericClass), { ... }) { ... }

In other words, it will work only when the generic of the base class is known and fixed.

farism commented 4 years ago

Is there a way to define additional properties when using the factory pattern? Something like

function createListModel<T extends Item>(additionalProperties: ModelProps) {
  class ListModel extends Model({
    items: prop<ObjectMap<T>>(() => objectMap()),
    selectedRef: prop<Ref<T> | null>(null),
    ...additionalProperties
  }) {
    // methods
  }

  return ListModel
}

export class PersonList extends createListModel<PersonModel>([], {
  someCustomProp: prop<string>()
}) {

}

This actually compiles but I get no intellisense for someCustomProp. If I try changing the types around

e.g.

function createListModel<T extends Item, TProps extends ModelProps>(additionalProperties: TProps) {

I run into

Base constructor return type 'BaseModel<ModelPropsToPropsData<{ items: OptionalModelProp<ObjectMap<T>, ObjectMap<T>>; selectedRef: OptionalModelProp<Ref<T> | null, Ref<T> | null>; } & TProps>, Optional<...>, ModelPropsToInstanceData<...>, Optional<...>> & Pick<...>' is not an object type or intersection of object types with statically known members.
xaviergonz commented 4 years ago
function createListModel<T extends object>() {
  class ListModel extends Model({
    items: prop(() => objectMap<T>()),
    selectedRef: prop<Ref<T> | null>(null),
  }) {
    // methods
  }

  return ListModel
}

class PersonList extends ExtendedModel(createListModel<object>(), {
  someCustomProp: prop<string>()
}) {
}
farism commented 4 years ago

Hey that's neat, I didn't even think to use a factory model with ExtendedModel. Works nicely, thanks!