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

Generate a TypeScript API from our OpenAPI spec #457

Merged
merged 63 commits into from
Aug 31, 2023
Merged

Conversation

danvk
Copy link
Collaborator

@danvk danvk commented Aug 30, 2023

This PR sets up opeanapi-typescript to generate types from our OpenAPI spec. I also looked at openapi-typescript-codegen but it generates an enormous number of files and classes for each API, which isn't a good fit for how we work.

There are a few complications:

  1. We actually have several APIs (settings, fs, projects, etc.). Having separate TS files for each of these would be a pain. To get a single TypeScript file, I had to merge all our OpenAPI specs.
  2. The resulting API schema types are all defined in one big, nested interface. You can see what that looks like here. This isn't quite as nice as having all these types as distinct, top-level interfaces like we do now. I toyed with forwarding all the types, but this doesn't look as nice in your editor and you lose things like JSDoc. So I decided to keep our existing json2ts types (api-types.ts). Which leads us to…
  3. I wanted to patch api-paths.ts to use the types from api-types.ts. I thought this would be find and replace, but they generate type names in ever so slightly different ways. So I wound up setting up a Python script to do some normalization and patch api-paths.ts.

So now we have types for requests / response and the various paths in our API available in TypeScript.

There's also openapi-fetch, which generates a runtime API for you with nice types. You can even pass your own fetch function to it. Unfortunately, it's typed as typeof fetch, which means that we'd need to make a web fetch-compatible version of Tauri's fetch. Yuck. I tried going down this path a bit but didn't like where I wound up.

Instead, I dropped openapi-fetch and decided to add types to the existing universalGet / universalPost / etc. APIs. This does mean some reinventing the wheel for what openapi-fetch does, but it's nice that we get to reuse the code that @cguedes already set up in #425.

There are still quite a few type assertions here because our API (in Python) is missing quite a few types.

In any event, you get nice types!
image

And autocomplete! (These are just the paths with POST endpoints.)
image

Still to do:

  • Write some unit tests for Typed API
  • Add the other HTTP methods (PUT, PATCH, DELETE)
  • Update remaining API calls

cguedes and others added 30 commits August 28, 2023 15:41
 - return project name
 - ensure storage folder (and references.json) exists
 - always create directory for new projects
 - support return project files for desktop
- This will check check if the file exists in the project
- Remove dependency on tauri/fs APIs
- Adopt HEAD in project filesystem to check if file exists
- Add (initial) support to the SAMPLE project
- set project path for the sample project
@cguedes
Copy link
Collaborator

cguedes commented Aug 31, 2023

@danvk everything looks nice to me. That TS code on src/api/typed-api.ts is 🔥

@cguedes
Copy link
Collaborator

cguedes commented Aug 31, 2023

@danvk with the combined swagger spec we can now use this code to access the full api documentation via http://127.0.0.1:8000/docs.

api = FastAPI(swagger_ui_parameters={"url": "/combined.openapi.json"})


@api.get("/combined.openapi.json")
def read_openapi_json_file():
    return FileResponse("combined.openapi.json")

image

CC @gjreda

@gjreda
Copy link
Collaborator

gjreda commented Aug 31, 2023

@cguedes I don't think that will be necessary once I complete #415. See more here: https://fastapi.tiangolo.com/tutorial/bigger-applications/#check-the-automatic-api-docs

@cguedes
Copy link
Collaborator

cguedes commented Aug 31, 2023

@cguedes I don't think that will be necessary once I complete #415. See more here: https://fastapi.tiangolo.com/tutorial/bigger-applications/#check-the-automatic-api-docs

So I think @danvk won't need to merge the open API specs into a single file.

@danvk
Copy link
Collaborator Author

danvk commented Aug 31, 2023

That does seem like a nicer way to get a combined OpenAPI but I think I'll leave that for another PR. We can remove my merging code once we do that.

@danvk danvk marked this pull request as ready for review August 31, 2023 17:21
@danvk danvk requested a review from cguedes August 31, 2023 17:29
cguedes
cguedes previously approved these changes Aug 31, 2023
Copy link
Collaborator

@cguedes cguedes left a comment

Choose a reason for hiding this comment

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

This looks amazing 💪
Only added some notes to remove commented code.

src/api/__tests__/typed-api.test.ts Show resolved Hide resolved
src/api/chat.ts Outdated Show resolved Hide resolved
src/api/completion.ts Outdated Show resolved Hide resolved
src/api/projectsAPI.ts Outdated Show resolved Hide resolved
src/api/projectsAPI.ts Outdated Show resolved Hide resolved
src/api/projectsAPI.ts Show resolved Hide resolved
src/api/rewrite.ts Outdated Show resolved Hide resolved
src/api/typed-api.ts Show resolved Hide resolved
src/settings/settingsManager.ts Outdated Show resolved Hide resolved
@danvk
Copy link
Collaborator Author

danvk commented Aug 31, 2023

Oh, also, test coverage goes up because:

  1. I'm excluding api-types.ts, which vitest previously counted as 285 uncovered lines.
  2. typed-api.ts counts as 102 fully-covered lines.

Test coverage should probably ignore lines of code that only exist in type space (i.e. type declarations), but I'm not sure if vitest has a setting for that.

@danvk danvk merged commit 255a870 into main Aug 31, 2023
10 checks passed
@danvk danvk deleted the openapi-typescript branch August 31, 2023 18:51
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