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 refs #248

Closed
sisp opened this issue May 10, 2021 · 9 comments
Closed

Generic refs #248

sisp opened this issue May 10, 2021 · 9 comments
Labels
🍗 enhancement New feature or request 👨‍💻 has PR A PR that potentially fixes the issue exists 📑 merged to master Feature done and merged to master 🎈 released A fix that should close the issue has been released

Comments

@sisp
Copy link
Contributor

sisp commented May 10, 2021

How about making refs generic as well (if possible) like we made models generic (#239, #242, 9be6e0f)? 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 xaviergonz added the 🍗 enhancement New feature or request label May 11, 2021
@xaviergonz
Copy link
Owner

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
Copy link
Contributor Author

sisp commented May 13, 2021

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
Copy link
Owner

xaviergonz commented May 13, 2021

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
Copy link
Owner

xaviergonz commented May 13, 2021

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
Copy link
Owner

Just checked, apparently it is :)

@xaviergonz
Copy link
Owner

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 xaviergonz added the 👨‍💻 has PR A PR that potentially fixes the issue exists label May 13, 2021
@xaviergonz
Copy link
Owner

PR #255

@sisp
Copy link
Contributor Author

sisp commented May 14, 2021

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

@xaviergonz xaviergonz added 🎈 released A fix that should close the issue has been released 📑 merged to master Feature done and merged to master labels May 14, 2021
@xaviergonz
Copy link
Owner

out in 0.59.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🍗 enhancement New feature or request 👨‍💻 has PR A PR that potentially fixes the issue exists 📑 merged to master Feature done and merged to master 🎈 released A fix that should close the issue has been released
Projects
None yet
Development

No branches or pull requests

2 participants