Skip to content

Conversation

@bbovenzi
Copy link
Contributor

@bbovenzi bbovenzi commented Jul 18, 2022

Use openapi-typescript to generate types for all rest API data. This keeps a single source of truth vs manually writing all the data endpoint types.

Also, found a bug where map_index was missing from the TaskInstance schema


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code changes, an Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in a newsfragment file, named {pr_number}.significant.rst or {issue_number}.significant.rst, in newsfragments.

@boring-cyborg boring-cyborg bot added area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jul 18, 2022
Copy link
Member

@pierrejeambrun pierrejeambrun left a comment

Choose a reason for hiding this comment

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

Looking nice. Just a few suggestions.

For the camel case naming they recommend keeping it as snake case in their readme. (project goals section) Maybe we should do that and get completely rid of camel case.

If we still want to enforce camel case, as a last resort we could use the template litéral + conditionnal type recursion, and we could define this: (add this to the generated file)

type SnakeToCamelCase<word extends string> =
  word extends `${infer firstWord}_${infer rest}`
    ? `${firstWord}${Capitalize<SnakeToCamelCase<rest>>}`
    : word;

type KeysToCamelCase<T> = {
  [K in keyof T as SnakeToCamelCase<string & K>]: T[K]
}

Then we only need to wrap our generated type like so: (in the generation step)

export type UserCollectionItem = KeysToCamelCase<components['schemas']['UserCollectionItem']>;
export type User = KeysToCamelCase<components['schemas']['User']>;
export type UserCollection = KeysToCamelCase<components['schemas']['UserCollection']>;
export type ConnectionCollectionItem = KeysToCamelCase<components['schemas']['ConnectionCollectionItem']>;

image

});
};

function generate(file) {
Copy link
Member

Choose a reason for hiding this comment

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

Should we also push the licence directly, or are we counting on the pre-commit hook to do that for us ?

@bbovenzi
Copy link
Contributor Author

@pierrejeambrun Good idea to add a KeysToCamelCase. I think we may want to switch to snake_case in the future. But that's a large refactor with a large surface area to introduce bugs so I think converting the types to camelCase is fine for now.

Also, fixed spelling errors and auto-adding the license to the generated file.

@bbovenzi bbovenzi marked this pull request as ready for review July 19, 2022 11:34
@pierrejeambrun
Copy link
Member

pierrejeambrun commented Jul 19, 2022

Good job on the nested type 👍

Should we add the type-generation step to the static-checks (pre-commit + CI)? What I mean by that is, if someone update the API, these changes should be reflected in the ts types. (And potentially linting error would arise)

Plus manually having to regenerate the types each time we pull from main in case the open api spec is updated is not ideal.

@bbovenzi bbovenzi requested a review from potiuk as a code owner July 19, 2022 13:20
Copy link
Member

@jedcunningham jedcunningham left a comment

Choose a reason for hiding this comment

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

Looks good, though there are some static check failures to clean up.

@bbovenzi
Copy link
Contributor Author

#25167 will handle the pre-commit hook to auto-generate the type file

@bbovenzi bbovenzi merged commit 38d6c28 into apache:main Jul 20, 2022
@bbovenzi bbovenzi deleted the generate-api-ts branch July 20, 2022 09:34
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues type:misc/internal Changelog: Misc changes that should appear in change log

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants