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

feat: Support entity type mappings #206

Merged
merged 27 commits into from
Feb 21, 2024
Merged

Conversation

jimmycallin
Copy link
Contributor

  • I have added automatic tests where applicable
  • The PR title is suitable as a release note
  • The PR contains a description of what has been changed
  • The description contains manual test instructions

Changes

Suggestion for alternative implementation of type mapping that don't couple ts-schema-generator and ftrack-api.

Should be as simple as:

import TypeMapping from "./__generated__/schema.ts"
import { Session } from @ftrack/api

const session = new Session<TypeMapping>(...);

const user = await session.query<"User">("...");

This allows easier extension of the types in case they are wrong or the user want to do something weird

Test

@jimmycallin jimmycallin requested a review from a team as a code owner February 15, 2024 23:47
export function getStatuses(
session: Session,
export function getStatuses<TEntityTypeMap extends Record<string, any>>(
session: Session<TEntityTypeMap>,
Copy link
Contributor Author

@jimmycallin jimmycallin Feb 15, 2024

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

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)

Copy link
Contributor

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can look at the test/session.test.ts and toy with it a bit, seem to be working:
image

@ffMathy
Copy link
Contributor

ffMathy commented Feb 16, 2024

I love where this is going. Way better. Great work!

Copy link
Contributor

@gismya gismya left a 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.

gismya
gismya previously approved these changes Feb 21, 2024
@gismya
Copy link
Contributor

gismya commented Feb 21, 2024

Needs to be tested by integrating in our code base before releasing, but my initial tests all look good.

@jimmycallin jimmycallin changed the title feat: Type mapping alternative feat: Support entity type mappings Feb 21, 2024
@jimmycallin jimmycallin merged commit 9cab49f into main Feb 21, 2024
6 checks passed
@jimmycallin jimmycallin deleted the feature/more-type-extensibility branch February 21, 2024 12:46
@ffMathy
Copy link
Contributor

ffMathy commented Feb 21, 2024

This makes me so happy! Thank you!

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.

3 participants