Generic models

[sisp]

I've been working around the limitations of generic models for a while, but I keep arriving at the same obstacles. I'm wondering whether there is any way to further improve generic models, so creating model classes is closer to what I would write in plain Typescript independent of mobx-keystone.

The main problem is that I can't declare a generic model class with generic props:

class M<V extends number | string | boolean> extends Model({
  value: prop<V>() // TS ERROR
}) {}

Current solutions:

1. Use a model class factory and create model classes for all the generic types. This can lead to combinatorial explosion when composing models because I need to create as many models as I have combinations of generic types.
2. Use an abstract base class where generic props are plain abstract class properties, inherit from the base class and make the abstract class properties real model props with concrete types. This also leads to combinatorial explosion when composing models.

With runtime types, it seems the equivalent of generics is a factory, so there may not be a solution in that case. But when using prop<T>() there are no runtime types and I'm wondering whether there is a way to get better support for generic models in that case.

[xaviergonz]

A wild idea is that you could cache the "generic" class and reuse it

let cachedClass: any;

function createA<P>() {
  class A extends Model({
    value: prop<P>(),
  }) {
  }

  if (!cachedClass) {
    cachedClass = A
  }

  return cachedClass as any as A
}

// these two should use the same class, but their typing is ok
const StringA = createA<string>()
const NumberA = createA<number>()

The downside is that you'd create a new class per "generic", but it should be quickly garbage collected

[sisp]

Thanks for this interesting idea! 🙂

I'm trying to avoid materializing actual classes (or just differently typed aliases) for different "generics". Perhaps a slightly extended example makes my struggles clearer. This is a simplified example of what I'd like to achieve:

@model("M")
class M<V extends number | string | boolean> extends Model({
  value: prop<V>() // TS ERROR
}) {}

@model("A")
class A extends Model({
  x: prop<M<string>>()
}) {}

@model("B")
class B extends Model({
  y: prop<M<number | boolean>>()
}) {}

Here, M<T> is a generic model (a container if you will) and the models A and B all have a prop of type M but with different constraints on the generic T. This is purely a matter of typing, the transpiled JS code is identical. I would like to make sure that when I'm accessing the prop of any instance of A or B I have proper static type-checking (and IntelliSense in VS Code) about the type of value of M.

Currently, it seems I need to create a class for each variant of M (or an alias with your idea above), e.g.:

abstract class BaseM<T extends number | string | boolean> extends Model({}) {
  abstract value: T
}

// NOTE: Just for getting concretely typed `M`s, we have different registered models
// which is redundant.

@model("MString")
class MString extends ExtendedModel(modelClass<BaseM<string>>(BaseM), {
  value: prop<string>()
}) {}

@model("MNumberOrBoolean")
class MNumberOrBoolean extends ExtendedModel(modelClass<BaseM<number | boolean>>(BaseM), {
  value: prop<number | boolean>()
}) {}

@model("A")
class A extends Model({
  x: prop<MString>()
}) {}

@model("B")
class B extends Model({
  y: prop<MNumberOrBoolean>()
}) {}

Or with your idea which results in only one registered model "M":

let cachedClass: any

function createM<T extends number | string | boolean>() {
  class M extends Model({
    value: prop<T>()
  }) {}

  if (!cachedClass) {
    cachedClass = model("M")(M)
  }

  return cachedClass as any as M
}

const MString = createM<string>()
type MString = InstanceType<typeof MString>

const MNumberOrBoolean = createM<number | boolean>()
type MNumberOrBoolean = InstanceType<typeof MNumberOrBoolean>

@model("A")
class A extends Model({
  x: prop<MString>()
}) {}

@model("B")
class B extends Model({
  y: prop<MNumberOrBoolean>()
}) {}

Perhaps this could be a workaround:

abstract class BaseM<T extends number | string | boolean> extends Model({}) {
  abstract value: T
}

@model("A")
class A extends Model({
  x: prop<BaseM<string>>()
}) {}

@model("B")
class B extends Model({
  y: prop<BaseM<number | boolean>>()
}) {}

// The concrete subclasses for all "generics" still need to be created once
// `A` and `B` need to be instantiated.

The pattern of this workaround would be:

1. Create an abstract model class with generics that extends Model({}) (no model props specified yet).
2. If needed, create subclasses with generics by extending this base class without using ExtendedModel (so still no model props are specified).
3. Create a specific model subclass (without generics) for each "generic" ...
   1. ... using ExtendedModel(modelClass<...>(...), { ... }) and specify all model props there with the correct concrete types which leads to several registered models, or
   2. ... using your idea which leads to only one registered model. The downside of this approach is that the factory needs to be created for every generic model (slightly redundant code - maybe a higher-order/wrapper function could avoid repeating code) and that instanceof checks aren't easily done because there are only specific classes available (although they are all identical, so actually it's just one).
4. For typing props of other models, use the abstract base class (created in 1.) or an abstract subclass (created in 2.). For instantiating models, use the specific model subclasses (created in 3.).

[sisp]

Your TS proposal microsoft/TypeScript#36406 would solve this problem more elegantly, but there has been no activity.

[sisp]

Maybe something like this:

function createModel<F extends () => ModelClass<AnyModel>>(
  modelType: string,
  factory: F
): F {
  let cachedClass: any

  return (() => {
    const clazz = factory()

    if (!cachedClass) {
      cachedClass = model(modelType)(clazz)
    }

    return cachedClass
  }) as any
}

test("cached model factory with generics", () => {
  abstract class BaseM<T extends number | string> extends Model({}) {
    abstract value: T
  }

  const createM = createModel("M", <T extends number | string>() => {
    class M extends ExtendedModel(BaseM, {
      value: prop<T>(),
    }) {}

    return M
  })

  const M1 = createM<string>()
  // eslint-disable-next-line @typescript-eslint/no-redeclare
  type M1 = InstanceType<typeof M1>

  const M2 = createM<number>()
  // eslint-disable-next-line @typescript-eslint/no-redeclare
  type M2 = InstanceType<typeof M2>

  expect(M1).toBe(M2)

  assert(
    _ as ModelData<M1>,
    _ as { [modelIdKey]: string; value: string } & { [modelIdKey]: string }
  )
  assert(
    _ as ModelData<M2>,
    _ as { [modelIdKey]: string; value: number } & { [modelIdKey]: string }
  )
})

[xaviergonz]

Nice, that way you don't even create new classes :)
Btw, you shouldn't need modelClass<BaseM<T>>(BaseM) but just BaseM in this case

[sisp]

True, I've updated the snippet above.

Do you think this could be an "officially recommended" pattern for mobx-keystone? And do you think it's worth adding the createModel function to the library? Not sure if modelType (and thus decorating the class) should be optional, i.e. whether this function makes much sense when it returns an undecorated model class.

[xaviergonz]

I'm not sure. On the one hand it is useful for people who rely on TS for type checking, but on the other hand it introduces a separation between type checked and untype checked patterns and also I don't know how widespread is the case where you need many generics forms from a safe base.

About decorating the class inside the function, I think it makes sense (better be reminded of the decorator when calling the function than forgetting about it and seeing it error in runtime).

[sisp]

Hm, in general would you say that generic models are rather an anti-pattern because they don't work with runtime type-checking anyway? Should factories be preferred over generics?

[sisp]

Regarding real generic models, if there was a way to dynamically create a class with generics, I think we could do this:

class M<T> extends Model(<U>() => ({ value: prop<U>() }))<T> {}

Here, Model(<U>() => ({ value: prop<U>() })) would have to return a class with the generic U and then the generic T from the child class gets passed to the dynamically created base class. But I'm not sure if that's possible with Typescript.

[sisp]

Check this out:

// Generic "A" seems to be required or else the returned constructor won't be generic
function Model<A extends never, R extends Record<string, any>>(fn: (...args: A[]) => R): { new(): R } {
  return null as any
}

function prop<T>(): T {
  return null as any
}

class M<T extends number | string> extends Model(<T extends number | string>() => ({
  v: prop<T>()
}))<T> {}

const m1 = new M()         // number | string
const m2 = new M<number>() // number
const m3 = new M<string>() // string

TS Playground

Might be related to microsoft/TypeScript#30215.

I think it makes a lot of sense that something like

class M<T> extends Model({ value: prop<T>() }) {}

does not work because Model({ value: prop<T>() }) can be called outside the context of subclassing, e.g.

const BaseM = Model({ value: prop<T>() }) // Where is `T` coming from?

class M<T> extends BaseM {} // What happens to `T` here?

so when we want a generic model, Model(...) needs to return a generic class. Since Model is all about defining model props (and setting up lots of internal things of course), we need to be able to define generic model props and make sure the returned class has those generics, too.

What do you think about this idea?

[xaviergonz]

Nice idea. I got it working for Model, but unfortunately I didn't manage to get it working for ExtendedModel so far when the class being extended is a generic one (since the parameter for extended model is the class itself rather than the class type, which is the one that includes generics).

I'll keep experimenting (unless you think a generic base Model is good enough)

[sisp]

Nice idea. I got it working for Model [...]

Awesome! 🙂

[...] but unfortunately I didn't manage to get it working for ExtendedModel so far when the class being extended is a generic one (since the parameter for extended model is the class itself rather than the class type, which is the one that includes generics).

What would your ExtendedModel(...) call look like with generics? I was thinking about something like:

class Child<T> extends ExtendedModel(
  <T>() => ({
    baseClass: modelClass<Parent<T>>(Parent),
    props: {
      other: prop<T>()
    }
  })
)<T> {}

We need both the base class and the new props to be able to access the same generics, so both must be specified in the same generic arrow function. With modelClass<Parent<T>>(Parent) I think the correct type should be specified.

[xaviergonz]

Good call! I got this working in the end

test("new pattern for generics", () => {
  @model("GenericModel")
  class GenericModel<T1, T2> extends Model(<U1, U2>() => ({
    v1: prop<U1>(),
    v2: prop<U2>(),
    v3: prop<number>(0),
  }))<T1, T2> {}

  assert(
    _ as ModelData<GenericModel<string, number>>,
    _ as { [modelIdKey]: string; v1: string; v2: number; v3: number }
  )
  assert(
    _ as ModelData<GenericModel<number, string>>,
    _ as { [modelIdKey]: string; v1: number; v2: string; v3: number }
  )

  const s = new GenericModel<string, number>({ v1: "1", v2: 2, v3: 3 })
  expect(s.v1).toBe("1")
  expect(s.v2).toBe(2)
  expect(s.v3).toBe(3)

  @model("ExtendedGenericModel")
  class ExtendedGenericModel<T1, T2> extends ExtendedModel(<T1, T2>() => ({
    baseModel: modelClass<GenericModel<T1, T2>>(GenericModel),
    props: {
      v4: prop<T2>(),
    },
  }))<T1, T2> {}

  const e = new ExtendedGenericModel<string, number>({ v1: "1", v2: 2, v3: 3, v4: 4 })
  expect(e.v1).toBe("1")
  expect(e.v2).toBe(2)
  expect(e.v3).toBe(3)
  expect(e.v4).toBe(4)
})

just double checking, would that be missing something?

[sisp]

Looks amazing, exactly what I've been looking for! 😄

[sisp]

@xaviergonz I'm really curious about your personal opinion and experience:

Hm, in general would you say that generic models are rather an anti-pattern because they don't work with runtime type-checking anyway? Should factories be preferred over generics?

-- #239 (comment)

Do you always use runtime type-checking? If so, why? And if not, why? Or when do you use the one or the other?

I think runtime types for specifying model props were popularized by MST (correct me if I'm wrong). I totally see that the chosen approach allows for deriving TS types automatically, so there is a single source of truth for both runtime and static types. But this pattern also requires using factories while when static type-checking is enough, generic models (especially with your PR) are sufficient, so factories are not needed (which is more efficient). I've been thinking whether runtime type-checking should be strictly an extension of only statically-typed models, so runtime types just come on top (somewhat related to #160).

[xaviergonz]

I don't really favor one or the other. In my case I use typed props since I moved from a MST project and it was easier to port that way.

For a new plan JS project I'd probably use typed props.
For a new TS project I'd probably use untyped props for stuff that doesn't come from backend and typed for stuff that comes from backend.

But that's just me :)

[sisp]

Thanks for sharing your thoughts! About using typed props when data comes from a backend: I assume your mobx-keystone model then has the same shape as the data coming from the backend and the runtime checks are for validation at the runtime boundary. What if the backend data shape differs from what would be a natural state representation in mobx-keystone? Would you decode the backend data using the runtime type-checkers offered by mobx-keystone outside and independent of a model and map this validated backend data to mobx-keystone models (i.e. use the type-checkers without tProps)? I know it's getting slightly off-topic, but I'm trying to figure out the real purpose of typed props especially in a TS project.

[xaviergonz]

About using typed props when data comes from a backend: I assume your mobx-keystone model then has the same shape as the data coming from the backend and the runtime checks are for validation at the runtime boundary.

Yes, sometimes they even share a common model (if the backend uses node js).

What if the backend data shape differs from what would be a natural state representation in mobx-keystone? Would you decode the backend data using the runtime type-checkers offered by mobx-keystone outside and independent of a model and map this validated backend data to mobx-keystone models (i.e. use the type-checkers without tProps)

I never face that issue since they always match 1:1 right now, but I guess it depends. If the backend and frontend are managed by different teams I'd type check them somehow in runtime for sure (just to make sure the contract doesn't get broken). If they are both managed by a same team and they both use a same common interface / MST model then probably it is not worth the trouble.

Btw, out in v0.58.0 :)

[sisp]

I see. Regarding separation of concerns, would you agree that it's sufficient to use only statically-typed models and when data from an external system is converted to model instances, runtime validation is performed to ensure the contract because the shape of that data is not known statically. Once the data has been validated, it is statically typed and model instances can be safely created. What I'm trying to say is: Validation at runtime boundaries seems to be a step that is independent of instantiating models and mutating those instances. With typed props, runtime checks are performed also when model actions are run, but at least in a properly typed TS project there are static guarantees, so is there actually any need to perform runtime checks when running model actions? It feels like validation at runtime boundaries is a separate step and should not be mixed with mutating model instances. Or am I missing something?

[sisp]

Closing since the actual issue was resolved in #242 and 9be6e0f and released with v0.58.{0,2}.

[xaviergonz]

I see. Regarding separation of concerns, would you agree that it's sufficient to use only statically-typed models and when data from an external system is converted to model instances, runtime validation is performed to ensure the contract because the shape of that data is not known statically. Once the data has been validated, it is statically typed and model instances can be safely created. What I'm trying to say is: Validation at runtime boundaries seems to be a step that is independent of instantiating models and mutating those instances. With typed props, runtime checks are performed also when model actions are run, but at least in a properly typed TS project there are static guarantees, so is there actually any need to perform runtime checks when running model actions? It feels like validation at runtime boundaries is a separate step and should not be mixed with mutating model instances. Or am I missing something?

Yeah, I guess you are right. but it is always nice that if you have to validate the data you don't have to use a different library with a separate model to do it so you can keep them easily in sync.

[sisp]

Yeah, I can understand that. Thanks for sharing your thoughts. I'm still trying to get a clear picture of how runtime checks (especially validation, i.e. not bailing out at the first error) can be integrated in an intuitive and general way, and handling generic models is one piece of the puzzle. I think I've made some progress, but that's a discussion to be continued in #160. 😉