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

Added openapi-typescript and openapi-typescript-fetch #28

Merged
merged 7 commits into from
Mar 4, 2022

Conversation

GlockTian
Copy link
Contributor

@GlockTian GlockTian commented Mar 4, 2022

Description

Allowing a file containing all type definitions for our API to be generated through yarn run generate.

Dependency added:

  • openapi-typescript
  • openapi-typescript-fetch

Fixes/resolves #22

Screenshots

Please include any screenshots or media that illustrates the changes made

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)
  • Improvement (non-breaking change which improves existing functionality)
  • This change requires a documentation update

Checklist:

Leave blank if not applicable

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

@j-chad j-chad added dx related to developer experience enhancement enhancement of existing features front-end related to front-end code labels Mar 4, 2022
@GlockTian GlockTian marked this pull request as ready for review March 4, 2022 04:03
@GlockTian GlockTian requested a review from a team as a code owner March 4, 2022 04:03
Copy link
Contributor

@wshe874 wshe874 left a comment

Choose a reason for hiding this comment

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

Looks very good. Just one suggestion to put typings.ts in a folder dedicated for interfaces.

@@ -26,7 +28,8 @@
"start": "react-scripts start",
"build": "react-scripts build",
"test": "react-scripts test",
"eject": "react-scripts eject"
"eject": "react-scripts eject",
"generate": "npx openapi-typescript http://localhost:4200/api-json --output ./src/typing.ts"
Copy link
Contributor

Choose a reason for hiding this comment

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

@GlockTian should we put typing.ts inside a folder that is dedicated for interfaces. i.e ./src/types/typing.ts Just in case if we have more interfaces later in the project.

Copy link
Contributor

Choose a reason for hiding this comment

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

also we should probably name it something a bit more descriptive. Maybe api-schema.ts?
I would also like to change the command from generate to something more descriptive

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe generate-api-types. bit wordy though

Copy link
Contributor

Choose a reason for hiding this comment

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

generate:api

Copy link
Contributor

Choose a reason for hiding this comment

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

what about generate-types:api I feel like just generate isn't descriptive enough

Copy link
Contributor

Choose a reason for hiding this comment

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

makes it sound as if the api is generated from here

Copy link
Contributor

Choose a reason for hiding this comment

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

generate:api-types

Idea being if we ever add other generators then the script can just be called with something like run-p generate:* to run all scripts in the glob

@frasermcc9
Copy link
Contributor

Looks good, I've got changes to the useApi hook that will use some typescript magic to use this stuff

Copy link
Contributor

@j-chad j-chad left a comment

Choose a reason for hiding this comment

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

@GlockTian you ticked that you have added documentation but I don't see any?

@GlockTian
Copy link
Contributor Author

@GlockTian you ticked that you have added documentation but I don't see any?

oh true, mistakes. will add them to the wiki

@j-chad
Copy link
Contributor

j-chad commented Mar 4, 2022

@GlockTian you ticked that you have added documentation but I don't see any?

oh true, mistakes. will add them to the wiki

and readme please

@frasermcc9 frasermcc9 requested a review from j-chad March 4, 2022 10:45
Copy link
Contributor

@j-chad j-chad left a comment

Choose a reason for hiding this comment

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

Lgtm

@GlockTian
Copy link
Contributor Author

Lgtm

Nice, should I do a squash & merge?

@frasermcc9
Copy link
Contributor

Lgtm

Nice, should I do a squash & merge?

Yes

@GlockTian GlockTian merged commit 298d1f4 into 701-T4:main Mar 4, 2022
@GlockTian GlockTian deleted the feature/openapi-typescript-fetch branch March 4, 2022 22:01
@frasermcc9 frasermcc9 mentioned this pull request Mar 5, 2022
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dx related to developer experience enhancement enhancement of existing features front-end related to front-end code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generate types for API requests
4 participants