-
Notifications
You must be signed in to change notification settings - Fork 2
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: Support entity type mappings #206
Conversation
For later extensibility.
Co-authored-by: Lars Johansson <gismya@gmail.com>
export function getStatuses( | ||
session: Session, | ||
export function getStatuses<TEntityTypeMap extends Record<string, any>>( | ||
session: Session<TEntityTypeMap>, |
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.
this should automatically get the entitytypemap inferred from the provided session, make sure it works so you don't have to specify it explicitly
@@ -109,13 +112,13 @@ export function query(expression: string): QueryOperation { | |||
* @param {string} expression API query expression | |||
* @return {Object} API operation | |||
*/ | |||
export function search({ | |||
export function search<TEntityType>({ |
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.
TEntityType is automatically inferred from the options type, so no need to specify it explicitly (please verify)
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.
That's clever if it works. Needs to be tested. The only issue I see is a lack of verification of the TEntitityType in the Options.
@@ -48,25 +48,29 @@ const ENCODE_DATETIME_FORMAT = "YYYY-MM-DDTHH:mm:ss"; | |||
* @class Session | |||
* | |||
*/ | |||
export class Session { | |||
export class Session< | |||
TEntityTypeMap extends Record<string, any> = DefaultEntityTypeMap, |
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 have a feeling that the extends Record<string,any>
will break all the type safety, setting everything to any even if we specify a TEntityTypeMap
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 love where this is going. Way better. Great work! |
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.
Seems to be working well. Operations work slightly less well with this structure than the module augmentation one, but I prefer this way of doing it anyways. We just need to add some documentation on how to use it.
Needs to be tested by integrating in our code base before releasing, but my initial tests all look good. |
This makes me so happy! Thank you! |
Changes
Suggestion for alternative implementation of type mapping that don't couple ts-schema-generator and ftrack-api.
Should be as simple as:
This allows easier extension of the types in case they are wrong or the user want to do something weird
Test