Abstract generic base class and model #2

sisp · 2019-08-20T17:06:51Z

I'm trying to implement a generic base class with a generic model prop but some shared logic among all subclasses (here the error getter):

import { computed } from 'mobx';
import { model, Model, prop } from 'mobx-keystone';

abstract class A<P> extends Model({
  value: prop<P>(), // Base class expressions cannot reference class type parameters. ts(2562)
}) {
  public abstract validate(value: P): string | undefined;

  @computed
  public get error(): string | undefined {
    return this.validate(this.value);
  }
}

@model('B')
class B extends A<string> {
  public validate(value: string): string | undefined {
    return value.length < 3 ? 'too short' : undefined;
  }
}

How would you implement this instead? Using the new ExtendedModel factory doesn't seem to help because I wouldn't be able to declare the generic prop and abstract validate method in A, so the error getter couldn't be implemented already in A.

Any help is much appreciated. :-)

The text was updated successfully, but these errors were encountered:

sisp · 2019-08-20T18:28:01Z

Perhaps the generic model prop is not necessary (besides the fact that this cannot work anyway) because declaring an abstract model prop is only a typing issue, so the abstract prop could be declared as a regular class property. Something like this could be a potential solution:

import { computed } from 'mobx';
import { ExtendedModel, model, Model, prop } from 'mobx-keystone';

abstract class A<P> extends Model({}) {
  public abstract value: P;

  public abstract validate(value: P): string | undefined;

  @computed
  public get error(): string | undefined {
    return this.validate(this.value);
  }
}

type BValue = string;

abstract class _B extends A<BValue> {}

// Argument of type 'typeof B' is not assignable to parameter of type 'ModelClass<BaseModel<any, any>>'.
//   Type 'B' is missing the following properties from type 'BaseModel<any, any>': $, typeCheck, [typeSymbol], [modelTypeKey] ts(2345)
@model('B')
class B extends ExtendedModel(
  // Argument of type 'typeof _B' is not assignable to parameter of type 'ModelClass<_B>'.
  //   Cannot assign an abstract constructor type to a non-abstract constructor type. ts(2345)
  _B,
  {
    value: prop<BValue>(),
  }
) {
  public validate(value: BValue): string | undefined {
    return value.length < 3 ? 'too short' : undefined;
  }
}

But ExtendedModel doesn't support abstract base classes. So a poor version of an abstract class at least gets the typing right, but doesn't enforce the implementation of the "abstract" value property and "abstract" validate method

import { computed } from 'mobx';
import { ExtendedModel, model, Model, prop } from 'mobx-keystone';

class A<P> extends Model({}) {
  public value!: P;

  public validate(value: P): string | undefined {
    throw new Error('not implemented');
  }

  @computed
  public get error(): string | undefined {
    return this.validate(this.value);
  }
}

type BValue = string;

@model('B')
class B extends ExtendedModel(class extends A<BValue> {}, {
  value: prop<BValue>(),
}) {
  public validate(value: string): string | undefined {
    return value.length < 3 ? 'too short' : undefined;
  }
}

Would you say this general approach makes sense? And could ExtendedModel support abstract classes?

sisp · 2019-08-20T18:35:47Z

I just realized the $ property is missing the generic prop when declaring a regular abstract class property.

xaviergonz · 2019-08-20T19:03:04Z

How about something like this

test("abstract-ish model classes", () => {
  function createA<P>() {
    class A extends Model({
      value: prop<P>(),
    }) {
      public validate(_value: P): string | undefined {
        return undefined
      }

      @computed
      public get error(): string | undefined {
        return this.validate(this.value)
      }
    }

    return A
  }

  @model("B")
  class B extends ExtendedModel(createA<string>(), {}) {
    public validate(value: string): string | undefined {
      return value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate("ho")).toBe("too short")
  expect(b.validate("long")).toBe(undefined)
  expect(b.error).toBe("too short")
})

sisp · 2019-08-20T19:09:28Z

I actually started with a factory very similar to your suggestion, but I'm currently relying on the instanceof operator (b instanceof A) when using findParent(..., parentNode => parentNode instanceof A) (in my real application, A::value is not just a primitive type but another model which needs to find its nearest parent node derived from A).

xaviergonz · 2019-08-20T19:14:41Z

Alternatively if you don't want a default implementation for validate you could do this:

test("abstract-ish model classes", () => {
  function createA<P>() {
    class A extends Model({
      value: prop<P>(),
    }) {
      public validate?(_value: P): string | undefined

      @computed
      public get error(): string | undefined {
        return this.validate!(this.value)
      }
    }

    return A
  }

  @model("B")
  class B extends ExtendedModel(createA<string>(), {}) {
    public validate(value: string): string | undefined {
      return value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate("ho")).toBe("too short")
  expect(b.validate("long")).toBe(undefined)
  expect(b.error).toBe("too short")
})

About the instanceof thing, why not add a symbol property to the base class

const isValidator= Symbol("isValidator")

class A ... {
  [isValidator] = true
}

// checking
parentNode => !!parentNode[isValidator]

xaviergonz · 2019-08-20T19:20:44Z

Or if you want to validate via instanceof that B is a valid instance of the string version of A then

test("abstract-ish model classes", () => {
  function createA<P>() {
    class A extends Model({
      value: prop<P>(),
    }) {
      public validate?(_value: P): string | undefined

      @computed
      public get error(): string | undefined {
        return this.validate!(this.value)
      }
    }

    return A
  }

  const StringA = createA<string>()

  @model("B")
  class B extends ExtendedModel(StringA, {}) {
    public validate(value: string): string | undefined {
      return value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate("ho")).toBe("too short")
  expect(b.validate("long")).toBe(undefined)
  expect(b.error).toBe("too short")
  expect(b instanceof StringA).toBe(true)
})

sisp · 2019-08-20T19:26:21Z

#2 (comment) could be a viable solution, thanks!

About #2 (comment): At least in my case I need to check whether b is an instance of any A, but that could be done using the symbol property as you suggested.

Would having ExtendedModel support abstract classes still make sense (and is it possible at all)?

xaviergonz · 2019-08-20T19:27:34Z

It is possible, I'm just trying to figure out the proper typings to make it work without TS dying on me :)

sisp · 2019-08-20T19:39:54Z

Awesome :-)

Another related variant of the above is giving me some headache, too, where the entire set of model props is passed to the factory:

import { computed } from 'mobx';
import { Model } from 'mobx-keystone';
import { ModelProps, ModelPropsToData } from 'mobx-keystone/dist/model/prop';

function createA<P extends ModelProps>(props: P) {
  class A extends Model(props) {
    public validate?(_value: ModelPropsToData<P>): string | undefined;

    @computed
    public get error(): string | undefined {
      return this.validate!(this.$);
    }
  }

  return A;
}

Not sure how to make this work ...

xaviergonz · 2019-08-20T20:17:45Z

Try changing ModelPropsToData

to this["$"] (didn't check, just a hunch)

sisp · 2019-08-20T20:26:36Z

Doesn't help unfortunately - the main problem is Model(props) with this error:

Base constructor return type 'BaseModel<ModelPropsToData<P>, { 1: OptionalFlat<ModelPropsToData<P>>; 0: _Merge<OptionalFlat<_Pick<ModelPropsToData<P>, { [K in keyof P]: P[K]["$hasDefault"] & K; }[keyof P] & keyof P>>, _Pick<...>>; }[Implements<...>]> & Pick<...>' is not an object type or intersection of object types with statically known members. ts(2509)

xaviergonz · 2019-08-21T07:33:06Z

Can't you do something like this?

  class Validable extends Model({}) {
    public validate?(): string | undefined

    @computed
    public get error(): string | undefined {
      return this.validate!()
    }
  }

  @model("B")
  class B extends ExtendedModel(Validable, { value: prop<string>() }) {
    public validate(): string | undefined {
      return this.$.value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.error).toBe("too short")
  expect(b instanceof Validable).toBe(true)

xaviergonz · 2019-08-21T07:35:26Z

Or if you really need validate to take the data as argument for some reason then

  class Validable extends Model({}) {
    public validate?(value: this["$"]): string | undefined

    @computed
    public get error(): string | undefined {
      return this.validate!(this.$)
    }
  }

  @model("B")
  class B extends ExtendedModel(Validable, { value: prop<string>() }) {
    public validate(value: this["$"]): string | undefined {
      return value.value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate({ value: "ho" })).toBe("too short")
  expect(b.validate({ value: "long" })).toBe(undefined)
  expect(b.error).toBe("too short")
  expect(b instanceof Validable).toBe(true)

xaviergonz · 2019-08-21T07:44:38Z

Or you can take a more functional approach while keeping the value computed (this is probably the approach I'd take to avoid subclassing):

   const validate = Symbol("validate")
  interface Validable {
    [validate](): string | undefined
  }

  function isValidable(obj: any): obj is Validable {
    return isModel(obj) && validate in obj
  }

  const validationResults = new WeakMap<Validable, IComputedValue<string | undefined>>()

  function getError(obj: Validable) {
    let computedResult = validationResults.get(obj)
    if (!computedResult) {
      computedResult = computed(() => obj[validate]())
      validationResults.set(obj, computedResult)
    }
    return computedResult.get()
  }

  @model("B")
  class B extends Model({ value: prop<string>() }) {
    public [validate](): string | undefined {
      return this.value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(isValidable(b)).toBe(true)
  expect(getError(b)).toBe("too short")

sisp · 2019-08-21T08:12:45Z

Or if you really need validate to take the data as argument for some reason then

  class Validable extends Model({}) {
    public validate?(value: this["$"]): string | undefined

    @computed
    public get error(): string | undefined {
      return this.validate!(this.$)
    }
  }

  @model("B")
  class B extends ExtendedModel(Validable, { value: prop<string>() }) {
    public validate(value: this["$"]): string | undefined {
      return value.value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(b.validate({ value: "ho" })).toBe("too short")
  expect(b.validate({ value: "long" })).toBe(undefined)
  expect(b.error).toBe("too short")
  expect(b instanceof Validable).toBe(true)

I really like this approach! :-) Once you've figured out how to allow abstract classes to be passed to ExtendedModel, this is perfect.

Or you can take a more functional approach while keeping the value computed (this is probably the approach I'd take to avoid subclassing):

   const validate = Symbol("validate")
  interface Validable {
    [validate](): string | undefined
  }

  function isValidable(obj: any): obj is Validable {
    return isModel(obj) && validate in obj
  }

  const validationResults = new WeakMap<Validable, IComputedValue<string | undefined>>()

  function getError(obj: Validable) {
    let computedResult = validationResults.get(obj)
    if (!computedResult) {
      computedResult = computed(() => obj[validate]())
      validationResults.set(obj, computedResult)
    }
    return computedResult.get()
  }

  @model("B")
  class B extends Model({ value: prop<string>() }) {
    public [validate](): string | undefined {
      return this.value.length < 3 ? "too short" : undefined
    }
  }

  const b = new B({ value: "hi" })
  expect(b.value).toBe("hi")
  expect(isValidable(b)).toBe(true)
  expect(getError(b)).toBe("too short")

I see how this can work, but at least to me it looks quite verbose and things are a bit spread across multiple places and not so nicely grouped within a class. Would you only take this approach to work around the subclassing problem or do you consider this good practice in general?

Thanks so much for these insightful discussions, by the way! You've done a terrific job with mobx-keystone, I'm so much more productive with it than with MST.

xaviergonz · 2019-08-21T09:01:12Z

I see how this can work, but at least to me it looks quite verbose and things are a bit spread across multiple places and not so nicely grouped within a class. Would you only take this approach to work around the subclassing problem or do you consider this good practice in general?

I just like it because I like to avoid subclassing as much as possible, but that's just personal preference :)

Thanks so much for these insightful discussions, by the way! You've done a terrific job with mobx-keystone, I'm so much more productive with it than with MST.

Thanks! That's the whole point of the project :D

sisp · 2019-08-29T19:11:31Z

I think #12 resolves this issue. Feel free to close it if you agree.

xaviergonz · 2019-08-29T19:49:15Z

True! closing

xaviergonz · 2019-08-29T19:49:41Z

Well actually reopening until I add better docs with the new patterns

sisp · 2019-12-04T22:02:59Z

When an abstract model class is extended in a factory function, Typescript infers the property's type as any although it should be possible to infer the actual type:

abstract class Base<T> extends Model({}) {
  public abstract value: T
}

function ExtendedBase<T>(defaultValue: T) {
  abstract class ExtendedBaseT extends Base<T> {
    public abstract value: T
  }

  return class extends ExtendedModel(ExtendedBaseT, {
    value: prop<T>(() => defaultValue)
  }) {}
}

class MyModel extends ExtendedBase('val') {}

const m = new MyModel({})
const v = m.value // type: any

However, without using an abstract class, type inference works fine:

abstract class Base<T> extends Model({}) {
  public abstract value: T
}

function ExtendedBase<T>(defaultValue: T) {
  class ExtendedBaseT extends Base<T> {
    public value!: T
  }

  return class extends ExtendedModel(ExtendedBaseT, {
    value: prop<T>(() => defaultValue)
  }) {}
}

class MyModel extends ExtendedBase('val') {}

const m = new MyModel({})
const v = m.value // type: string

Any idea what is causing this problem?

xaviergonz · 2019-12-09T11:01:32Z

It is because sadly in typescript there doesn't seem to be a way to infer the generic of abstract classes, so it is downcasted to any (it is possible with non abstract classes, that's why it works then)

There's a somehow ugly workaround though:
return class extends ExtendedModel(ExtendedBaseT as AbstractModelClass<ExtendedBaseT>, {
if the original class type is passed directly to the argument (rather than rely on infer) it will be picked up correctly.

See this commit: f17bca6

xaviergonz · 2019-12-09T11:51:56Z

This is the core problem btw:

function classFactory<T>() {
  class CS {
    x!: T
  }

  type InstanceViaNew<T> = T extends { new (...args: any[]): infer R } ? R : never
  type InstanceViaProto<T> = T extends { prototype: infer R } ? R : never

  type CS1 = InstanceViaNew<typeof CS>["x"] // ok, T
  type CS2 = InstanceViaProto<typeof CS>["x"] // not ok, any :(

  abstract class ACS {
    x!: T
  }

  type AC1 = InstanceViaNew<typeof ACS>["x"] // not ok, never (because abstract classes have no new) :(
  type AC2 = InstanceViaProto<typeof ACS>["x"] // not ok, any :(
}

xaviergonz · 2019-12-09T12:03:02Z

Just opened a TS issue: microsoft/TypeScript#35576

xaviergonz · 2019-12-09T12:41:01Z

I think I've found a more or less decent workaround while TS adds support for that:

#70

Basically the only requirement is to wrap the abstract class passed to ExtendsModel in a call to "abstractModelClass". It was made a requirement so people are not left wondering why sometimes it will work and sometimes it won't, but it is a small breaking change.

Therefore your example would be fixed with:
return class extends ExtendedModel(abstractModelClass(ExtendedBaseT), {

What do you think?

sisp · 2019-12-10T09:24:48Z

I think it's a good workaround until Typescript fixes the underlying problem. Thank you very much for your efforts, as always! :-)

Do you think it makes sense to open a separate issue about this problem with a reference to the TS issue you opened to track progress on this matter and remove the workaround again when it's not needed anymore?

xaviergonz · 2019-12-10T18:37:22Z

Good idea, just created the issue :)
The workaround should be out now in v0.28.0

xaviergonz · 2020-02-05T19:25:56Z

Closing since abstractModelClass is not needed anymore and there are docs about generics and inheritance

xaviergonz added the ❔ question General question label Aug 20, 2019

sisp mentioned this issue Aug 27, 2019

Subclassing with abstract class #9

Closed

xaviergonz closed this as completed Aug 29, 2019

xaviergonz reopened this Aug 29, 2019

xaviergonz closed this as completed Feb 5, 2020

Abstract generic base class and model #2

Abstract generic base class and model #2

Comments

sisp commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

sisp commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sisp commented Aug 20, 2019

Uh oh!

xaviergonz commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sisp commented Aug 20, 2019

Uh oh!

xaviergonz commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaviergonz commented Aug 20, 2019

Uh oh!

sisp commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaviergonz commented Aug 20, 2019

Uh oh!

sisp commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaviergonz commented Aug 20, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sisp commented Aug 20, 2019

Uh oh!

xaviergonz commented Aug 21, 2019

Uh oh!

xaviergonz commented Aug 21, 2019

Uh oh!

xaviergonz commented Aug 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sisp commented Aug 21, 2019

Uh oh!

xaviergonz commented Aug 21, 2019

Uh oh!

sisp commented Aug 29, 2019

Uh oh!

xaviergonz commented Aug 29, 2019

Uh oh!

xaviergonz commented Aug 29, 2019

Uh oh!

sisp commented Dec 4, 2019

Uh oh!

xaviergonz commented Dec 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

xaviergonz commented Dec 9, 2019

Uh oh!

xaviergonz commented Dec 9, 2019

Uh oh!

xaviergonz commented Dec 9, 2019

Uh oh!

sisp commented Dec 10, 2019

Uh oh!

xaviergonz commented Dec 10, 2019

Uh oh!

xaviergonz commented Feb 5, 2020

Uh oh!

sisp commented Aug 20, 2019 •

edited

Loading

sisp commented Aug 20, 2019 •

edited

Loading

xaviergonz commented Aug 20, 2019 •

edited

Loading

xaviergonz commented Aug 20, 2019 •

edited

Loading

sisp commented Aug 20, 2019 •

edited

Loading

sisp commented Aug 20, 2019 •

edited

Loading

xaviergonz commented Aug 20, 2019 •

edited

Loading

xaviergonz commented Aug 21, 2019 •

edited

Loading

xaviergonz commented Dec 9, 2019 •

edited

Loading