-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: adds support for client generics, improving type safety in comb… #401
base: master
Are you sure you want to change the base?
Conversation
…ination with model generator
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any updates on readme :/ How I as the end user would know how to utilize model generator with this?
@@ -18,15 +19,15 @@ interface IRichTextImageUrlRecord { | |||
newUrl: string; | |||
} | |||
|
|||
export class ElementMapper { | |||
export class ElementMapper<TClientTypes extends ClientTypes> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this class doesn't need to send the whole type of object TClientTypes
it just needs need type TClientTypes['contentItemType'].
It would be easier to test this class if you need to pass only the necessary stuff
Check also the other classes for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It doesn't, the only reason I passed all types is for consistency and ease of use. Do you think it's worth splitting?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As I understand the point of ease of use, I am not sure about consistency. These generic types are new after all, so they are only consistent with the new code you are writing :).
For me there are two options:
- Pass only the required type, which is more correct of system modelling. This improves the type dependency coupling.
- If I wanted to unit test only ElementMapper I would need to define the whole type to the generic. That would be bothering as the class really only needs type of
contentItemType
. So for me as a tester of ElementMapper would be nice to pass justElementMapper<TContentItemType>
- If I wanted to unit test only ElementMapper I would need to define the whole type to the generic. That would be bothering as the class really only needs type of
- If you just want to pass the object everywhere, I guess there is not as elegant workaround to satisfy both ends (ease of use and creating only part of the object)
ElementMapper<TClientTypes extends Pick<ClientTypes, 'contentItemType'>>
} | ||
|
||
export class ItemMapper { | ||
private readonly elementMapper: ElementMapper; | ||
export class ItemMapper<TClientTypes extends ClientTypes> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the same as the comment for element mapper
That's a fair point, I will have to update the readme, but currently, the model generator does not yet support this. I wanted to finalize this first. One idea I was toying with is to enforce the generic argument when creating a delivery client. This way developers would have to look at this & either use the default types or generate the models. It would also help with the discoverability of the model generator and therefore improve the developer experience across all projects using it. What do you think @IvanKiral ? |
This PR adds ability to provide generics to client such as:
The idea is that the
ClientTypes
would be generated by https://github.com/kontent-ai/model-generator-js