Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generic models with runtime type-checking #271

Open
sisp opened this issue Jun 28, 2021 · 7 comments
Open

Generic models with runtime type-checking #271

sisp opened this issue Jun 28, 2021 · 7 comments
Labels
🍗 enhancement New feature or request 🙏 help welcome A champion is welcome

Comments

@sisp
Copy link
Contributor

sisp commented Jun 28, 2021

In continuation of #239, I'd like to discuss an idea for generic models with runtime type-checking without using a class factory. In #242, support for truly generic models was added by means of a generic arrow function (without arguments) that enables returning generic props. I think that adding arguments to this generic arrow function, being the runtime types that correspond to the TS generics, would be the natural extension for having generic runtime type-checked models.

For instance:

  • Generic model (TS only):

    @model("myApp/GenericPoint")
    class GenericPoint<T> extends Model(<T>() => ({
      x: prop<T>(),
      y: prop<T>(),
    }))<T> {
      // ...
    }
    
    const point = new GenericPoint({ x: 1, y: 2 })
  • Generic model (TS + runtime type-checking):

    @model("myApp/GenericPoint")
    class GenericPoint<T> extends Model(<T>(valueType: TypeChecker<T>) => ({
      x: tProp(valueType),
      y: tProp(valueType),
    }))<T> {
      // ...
    }
    
    // The 2nd argument is an array of runtime types matching the arguments of the
    // arrow function above. In contrast to TS-only generic models, the runtime types
    // must be passed explicitly because they cannot be inferred.
    const point = new GenericPoint({ x: 1, y: 2 }, [types.integer])

    Note that TypeChecker<T> currently does not exist yet. TypeScript would derive the type of the 2nd argument of the constructor from the arguments of the arrow function and make sure compatible runtime type-checkers are provided in this array.

The sketched approach has some implications on snapshots and reconciliation. Model classes with "generic runtime types" are not uniquely/fully defined in contrast to non-generic model classes because here the same model class can be instantiated with different runtime types. This means the runtime types need to become snapshotable and the ones provided upon instantiation need to be stored in the snapshot along with the rest of the model snapshot.

For instance:

const pointSnapshot = getSnapshot(point)

could look like this:

{
  $modelType: "myApp/GenericPoint",
  $modelId: "...",
  $modelTypeCheckers: [
    {
      $checkerType: "mobx-keystone/types/integer<number>",
    }
  ]
}

Runtime types with arguments like types.or could be represented like this:

{
  $checkerType: "mobx-keystone/types/or",
  orTypes: [
    {
      $checkerType: "mobx-keystone/types/integer<number>",
    },
    {
      $checkerType: "mobx-keystone/types/string",
    }
  ]
}

Thus, runtime types like models would need to have globally unique type names and would need to be registered in a global registry, so that the reconciler can load a snapshot and instantiate the types from the snapshot again.

Refinements would require a globally unique name, so they can be stored in the registry, too. At the moment, the name is optional.

I'm curious about some feedback. 🙂

@xaviergonz
Copy link
Owner

It sounds like a good initial approach. The only part I'm not 100% sold is forcing people to add unique names when creating their own types, but I guess they could just be optional and throw in runtime if those types are being used for a generic model...

Also I'd make the second parameter to new an options object, just in case it grows with more stuff in the future. e.g.

const point = new GenericPoint({ x: 1, y: 2 }, { genericTypes: [types.integer] })

Would you be up to give a try at implementing it? :)

@xaviergonz xaviergonz added 🍗 enhancement New feature or request 🙏 help welcome A champion is welcome labels Jun 29, 2021
@sisp
Copy link
Contributor Author

sisp commented Jun 30, 2021

Great, thank you very much for your feedback! I'd be happy to give it a try. 🙂

@sisp
Copy link
Contributor Author

sisp commented Jul 13, 2021

This would be my suggested API for ExtendedModel:

  • Generic extended model (TS only):
    @model("myApp/Generic3dPoint")
    class Generic3dPoint<T> extends ExtendedModel(<T>() => ({
      baseModel: modelClass<GenericPoint<T>>(GenericPoint),
      props: {
        z: prop<T>(),
      },
    }))<T> {
      // ...
    }
  • Generic extended model (TS + runtime type-checking):
    @model("myApp/Generic3dPoint")
    class Generic3dPoint<T> extends ExtendedModel(<T>(valueType: TypeChecker<T>) => ({
      baseModel: modelClass<GenericPoint<T>>(GenericPoint),
      baseModelGenericTypes: [valueType],
      props: {
        z: tProp(valueType),
      },
    }))<T> {
      // ...
    }

What do you think, @xaviergonz?

@xaviergonz
Copy link
Owner

xaviergonz commented Jul 15, 2021

looks good to me :) Maybe it would be possible to extract valueType from the argument of new instead of using baseModelGenericTypes ?

I mean, so calling new would inject baseModelGenericTypes itself

@sisp
Copy link
Contributor Author

sisp commented Jul 15, 2021

I think this doesn't work in the general case. The mapping from the generic types of the extended class to those of the base class is not necessarily 1:1, so it must be possible to map the types explicitly. For instance, the extended class might only have one generic type while the base class has two, and perhaps the generic type of the extended class is assigned to both generic types of the base class. Or one generic type of the base class is hardcoded and the other is passed through from the extended class.

@xaviergonz
Copy link
Owner

lgtm then

@sisp sisp mentioned this issue Aug 19, 2021
4 tasks
@sisp
Copy link
Contributor Author

sisp commented Dec 18, 2021

Apologies for the inactivity here. A quick update: I've started some first steps of implementing this feature, but realized it's actually not so straightforward.

Conceptually, I've been wondering whether passing generic runtime types in the model definition

... extends ExtendedModel(<T>(valueType: TypeChecker<T>) => ({
  baseModel: modelClass<GenericPoint<T>>(GenericPoint),
  baseModelGenericTypes: [valueType],
  // ...
}))<T> {
  // ...
}

is (a) the right approach and (b) sufficient because there are at least two other places that I can see now where something similar is needed:

  • types.model(clazz) would need to be extended to something like types.model(clazz, { genericTypes: [...] })
  • fromSnapshot(type, snapshot) would need to be extended to something like fromSnapshot(type, snapshot, { genericTypes: [...] })

When comparing this approach with TS types, type aliasing like

type NumberPoint = GenericPoint<number>

isn't possible, but perhaps if it were, e.g.

const NumberPoint = modelClass(GenericPoint, { genericTypes: [types.number] })

the API would also become simpler:

  • ... extends ExtendedModel(<T>(valueType: TypeChecker<T>) => ({
      baseModel: modelClass(GenericPoint, { genericTypes: [valueType] }),
      // ...
    }))<T> {
      // ...
    }
  • types.model(modelClass(GenericPoint, { genericTypes: [types.number] }))
    
    // or
    
    const NumberPoint = modelClass(GenericPoint, { genericTypes: [types.number] })
    types.model(NumberPoint)
  • fromSnapshot(modelClass(GenericPoint, { genericTypes: [types.number] }), snapshot)
    
    // or
    
    const NumberPoint = modelClass(GenericPoint, { genericTypes: [types.number] })
    fromSnapshot(NumberPoint, snapshot)

modelClass would create a kind of model class alias at runtime with the passed generic runtime types already set. I imagine this could be done by creating a subclass of the model class and assigning the runtime types to a property of the class type/constructor. But then the constructors of the "aliased" class and the original class wouldn't be identical; not sure if that's a problem.

Instantiating a generic model could be done in three ways:

  1. const point = new GenericPoint({ x: 1, y: 2 }, { genericTypes: [types.number] })
    
    // somewhat equivalent to
    const point = new GenericPoint<number>({ x: 1, y: 2})
    Requires a dedicated extension of the model class constructor though which could be avoided by options 2 and 3.
  2. const NumberPoint = modelClass(GenericPoint, { genericTypes: [types.number] })
    const point = new NumberPoint({ x: 1, y: 2 })
    
    // somewhat equivalent to
    const NumberPoint = GenericPoint as GenericPoint<number>
    const point = new NumberPoint({ x: 1, y: 2})
  3. const point = new modelClass(GenericPoint, { genericTypes: [types.number] })({ x: 1, y: 2 })
    
    // somewhat equivalent to
    const point = new GenericPoint<number>({ x: 1, y: 2})
    although a bit ugly 😆

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request 🙏 help welcome A champion is welcome
Projects
None yet
Development

No branches or pull requests

2 participants