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
555 stars 25 forks source link

Generic refs #248

Closed sisp closed 3 years ago

sisp commented 3 years ago

How about making refs generic as well (if possible) like we made models generic (#239, #242, 9be6e0f386547c7b5a58eedd3566bbfa6a63fece)? This would be useful for creating refs of generic models, so the generics can be passed down, e.g.:

const myRef = rootRef("some model type", <T>() => ({
  getId(target: unknown): string | undefined {
    // ...
  },
  onResolvedValueChange(
    ref: Ref<GenericModel<T>>,
    newValue: GenericModel<T> | undefined,
    oldValue: GenericModel<T> | undefined
  ) {
    // ...
  }
}))

Of course the same for custom refs.

xaviergonz commented 3 years ago

Just wondering, how is that different from

const myRef1 = rootRef<GenericClass<any>>("some model type", {
  // ...
}))

const myRef2 = rootRef<GenericClass<number>>("some model type", {
  // ...
}))

?

I'm confused because with references you usually set the type as a type argument, not through inference.

sisp commented 3 years ago

Although the added value might not be as significant as for generic models, wouldn't it be nice to be able to have:

const genericModelRef = rootRef("GenericModel", <T>() => ({
 // ...
}))

const m = new GenericModel({ value: 1 }) // e.g. GenericModel<number> (inferred at construction time)
const mref = genericModelRef(m)          // e.g. Ref<GenericModel<number>> (inferred)

mref's type would be inferred as Ref<GenericModel<number>> instead of being fixed to, e.g., Ref<GenericModel<any>>, it would be a bit more precise in this case.

xaviergonz commented 3 years ago

And from what part of the options to rootRef would it infer the type from? (both are optional)

export interface RootRefOptions<T extends object> {
  /**
   * Must return the ID associated to the given target object, or `undefined` if it has no ID.
   * If not provided it will try to get the reference id from the model `getRefId()` method.
   *
   * @param target Target object.
   */
  getId?: RefIdResolver

  /**
   * What should happen when the resolved value changes.
   *
   * @param ref Reference object.
   * @param newValue New resolved value.
   * @param oldValue Old resolved value.
   */
  onResolvedValueChange?: RefOnResolvedValueChange<T>
}

Or you mean it should be inferred from the parameter to the constructor? though in that case I guess it wouldn't be type checked if you pass a wrong type.

const m = new GenericModel({ value: 1 }) // e.g. GenericModel<number> (inferred at construction time)
const mref = someOtherRef(m)          // would infer to Ref<GenericModel<number>>, but we wanted those kind of refs to only support other models
xaviergonz commented 3 years ago

Maybe a middle ground?

const genericModelRef = rootRef<GenericModel<any>>("GenericModel", {
 // ...
})

const ref = genericModelRef(new GenericModel({value: 1})) // creates a Ref<GenericModel<number>>
const invalidRef = genericModelRef(new SomeOtherModel()) // error since SomeOtherModel does not extend GenericModel<any>

don't know if it is even possible yet though

xaviergonz commented 3 years ago

Just checked, apparently it is :)

xaviergonz commented 3 years ago

Good?

test("generic typings", () => {
  @model("GenericModel")
  class GenericModel<T1, T2> extends Model(<U1, U2>() => ({
    v1: prop<U1 | undefined>(),
    v2: prop<U2>(),
    v3: prop<number>(0),
  }))<T1, T2> {}

  const genericRef = rootRef<GenericModel<any, any>>("genericRef")

  const ref = genericRef(new GenericModel({ v1: 1, v2: "2" }))
  assert(ref, _ as Ref<GenericModel<number, string>>)

  const genericRef2 = rootRef<GenericModel<string, number>>("genericRef2")

  // @ts-expect-error
  genericRef2(new GenericModel({ v1: 1, v2: "2" }))
})
xaviergonz commented 3 years ago

PR https://github.com/xaviergonz/mobx-keystone/pull/255

sisp commented 3 years ago

Perfect, that's an even better solution! Exactly what I was hoping to achieve. Thank you very much as always! 🙂

xaviergonz commented 3 years ago

out in 0.59.0