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

improved generics #242

Merged
merged 1 commit into from
May 4, 2021
Merged

improved generics #242

merged 1 commit into from
May 4, 2021

Conversation

xaviergonz
Copy link
Owner

No description provided.

@xaviergonz
Copy link
Owner Author

@sisp could you please review it?

@codecov
Copy link

codecov bot commented May 3, 2021

Codecov Report

Merging #242 (ff21f7b) into master (e77e359) will increase coverage by 0.05%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #242      +/-   ##
==========================================
+ Coverage   90.42%   90.47%   +0.05%     
==========================================
  Files         170      170              
  Lines        4875     4892      +17     
  Branches      761      765       +4     
==========================================
+ Hits         4408     4426      +18     
+ Misses        450      449       -1     
  Partials       17       17              
Impacted Files Coverage Δ
src/model/Model.ts 100.00% <0.00%> (ø)
src/modelShared/prop.ts 100.00% <0.00%> (ø)
src/dataModel/DataModel.ts 100.00% <0.00%> (ø)
src/dataModel/utils.ts 53.57% <0.00%> (+3.57%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77e359...ff21f7b. Read the comment docs.

@sisp
Copy link
Contributor

sisp commented May 4, 2021

Looks great in my opinion!

The only thing I noticed is that the generic types must be provided explicitly when instantiating a model, they aren't inferred.

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

const s = new GenericModel({ v1: "1", v2: 2, v3: 3 }) // GenericModel<unknown, unknown>

I'm not sure if it's possible to get those generics inferred though.

@xaviergonz
Copy link
Owner Author

I'm not sure if it's possible to get those generics inferred though.

No idea, if you find a way let me know. It seems you've really looked into it :)

@xaviergonz xaviergonz merged commit a262305 into master May 4, 2021
@sisp
Copy link
Contributor

sisp commented May 4, 2021

Thank you very much for your efforts on improving generic models! 🙏 This is a great improvement in my opinion. If I find a way to have the generics inferred, I'll let you know.

@xaviergonz
Copy link
Owner Author

If I find a way to have the generics inferred, I'll let you know.

Please do, I'm really curious :)

@xaviergonz xaviergonz deleted the generics branch May 4, 2021 20:30
@sisp
Copy link
Contributor

sisp commented May 7, 2021

@xaviergonz It seems the conditional IsOptionalValue<TPropValue, string, never> in MaybeOptionalModelProp<TPropValue> used in the overload function prop<TValue>(): MaybeOptionalModelProp<TValue> is preventing inference of the generics. Just for testing, when I hardcode the conditional as string

 export type MaybeOptionalModelProp<TPropValue> = ModelProp<
   TPropValue,
   TPropValue,
-  IsOptionalValue<TPropValue, string, never>
+  string
 >

then inferring the generics works:

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

const s = new GenericModel({ v1: "1", v2: 2, v3: 3 })
assert(s, _ as GenericModel<string, number>) // OK



@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({ v1: "1", v2: 2, v3: 3, v4: 4 })
assert(e, _ as ExtendedGenericModel<string, number>) // OK

I'm just not sure yet how to fix it. If you have an ingenious idea, I'm all ears. 😄

@xaviergonz
Copy link
Owner Author

Thanks for the pointers. Generics should now be inferable in v0.58.2 :D

@sisp
Copy link
Contributor

sisp commented May 8, 2021

Incredible, I don't know how long it would have taken me to find that solution! Thank you very much (again), you're a genius! 😃

@sisp sisp mentioned this pull request May 8, 2021
@xaviergonz
Copy link
Owner Author

xaviergonz commented May 8, 2021

My only skill is to be able to throw random attempts to fix it at the wall for long enough until one works 😉

@sisp
Copy link
Contributor

sisp commented May 8, 2021

random -> educated guessing

I'd say 😉.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants