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

Add low-level TS API client with generated types #4538

Merged
merged 33 commits into from
Sep 9, 2024

Conversation

sarayourfriend
Copy link
Collaborator

@sarayourfriend sarayourfriend commented Jun 21, 2024

Description

Based on my work exploring this concept in https://git.sr.ht/~sara/openverse-api-client.

There are major differences from that version:

  1. I have only included the TypeScript client in this PR. (Previous versions included a Python client).
  2. The TypeScript client uses openapi-fetch and it's TypeScript support package to generate TypeScript interfaces from the OpenAPI schema, instead of generating them with Python and Jinja2 templates.

I've already demonstrated the value of the JavaScript client for even our own frontend. The Python client, on the other hand, would be more useful for things like the Openverse Slack Reaction app (which isn't part of the project yet). For that reason, I think it is necessary to scope this only to the JS client. It also makes this easier to review, because the JS client is substantially simpler than the Python client. That's thanks to the excellence of the openapi-fetch and openapi-typescript packages. Nothing of that quality exists for Python. We will need to do much more work for a generated Python client, and it deserves to be its own pull request.

To briefly document the approach:

  • I've introduced one new package, @openverse/api-client. The package houses the generated types and exports a client wrapping those types.
  • The package also implements an openapi-fetch Middleware to handle the Openverse API authentication flow.
  • Package publication is not included, but the package's build script already includes checks to ensure the package builds and passes basic checks required for publication. A separate PR should follow to document and implement a manual publication process. We can turn that into an automated process later on.

Further improvements can be made to create higher level APIs that build upon this "low-level" version, as discussed by @obulat (who raised the question in the first place) and myself in the comments of this PR. Additionally, we could consider creating an intermediate package @openverse/types to house the generated OpenAPI schema types, as well as further support types like we use in the frontend's src/types directory, such as definitions of media types, and so forth. That package could also export specific components from components["schemas"], like a type Media = components["schemas"].Audio | components["schemas"].Image, and so forth.

Testing Instructions

Review the documentation for the package (and the information above) to understand the goals and approach. Then review the code. Due to the nature of the openapi-fetch package, there is actually relatively little code to review in this PR. (This was not the case in previous versions which also included a Python version.) Feel free to review the generated code to get a feeling for how it looks.

I've left comments on some changes in the PR that I hope will clarify things I anticipated may have questions asked about. Please feel free to ask for further clarification in any case, of course!

The main code to review carefully is the authentication middleware. This middleware is based on the strategy proven in our own frontend application, by the api-token Nuxt plugin. As you read the middleware code, review the test cases for it as well, in the tests included in the package.

You can also check out the branch and try the client out in the unit tests, to see how it feels to work with. Nothing is set in stone, and it is only the "low-level" version. As Olga and I have discussed in the comments on this PR, it's worth considering a future where we have higher level functions like client.image.search, and so on, which may have better ergonomics and discoverability than client.GET("/v1/images/", { params: { query: { q } } }).

We should aim to use the package inside our frontend (and elsewhere, if appropriate) as much as possible to exercise it and understand what kinds of higher level wrapper functions would be beneficial.

Checklist

  • My pull request has a descriptive title (not a vague title likeUpdate index.md).
  • My pull request targets the default branch of the repository (main) or a parent feature branch.
  • My commit messages follow best practices.
  • My code follows the established code style of the repository.
  • I added or updated tests for the changes I made (if applicable).
  • I added or updated documentation (if applicable).
  • I tried running the project locally and verified that there are no visible errors.
  • [N/A] I ran the DAG documentation generator (./ov just catalog/generate-docs for catalog
    PRs) or the media properties generator (./ov just catalog/generate-docs media-props
    for the catalog or ./ov just api/generate-docs for the API) where applicable.

Developer Certificate of Origin

Developer Certificate of Origin
Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

@sarayourfriend sarayourfriend added 🟩 priority: low Low priority and doesn't need to be rushed 🌟 goal: addition Addition of new feature 🕹 aspect: interface Concerns end-users' experience with the software 🧱 stack: infra Related to the Terraform config and other infrastructure labels Jun 21, 2024
@openverse-bot openverse-bot added 🧱 stack: api Related to the Django API 🧱 stack: frontend Related to the Nuxt frontend labels Jun 21, 2024
Copy link
Member

@dhruvkb dhruvkb left a comment

Choose a reason for hiding this comment

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

This project is very interesting and I am excited to review this PR once it is undrafted. I also want to discuss the use of the LGPL license because, while I agree with the choice, the reasons for it need to be documented somewhere.

@sarayourfriend
Copy link
Collaborator Author

I also want to discuss the use of the LGPL license because, while I agree with the choice, the reasons for it need to be documented somewhere

Totally! I'd forgotten I kept the LGPL from my original work. It's been a long overdue conversation for us to talk about when/if we'd go in line with WP foundation's GPL usage.

We can keep this MIT in the meantime for flexibility, though.

That's a good conversation for us to consider for the meetup, potentially (depending on how the maintainers all feel about it).

@sarayourfriend sarayourfriend marked this pull request as ready for review August 13, 2024 10:24
@sarayourfriend sarayourfriend requested review from a team as code owners August 13, 2024 10:24
@sarayourfriend sarayourfriend requested review from AetherUnbound, obulat, zackkrida and stacimc and removed request for a team August 13, 2024 10:24
Comment on lines -8 to +10
- text
# Non-code sentence level text OR headings
- summary
- heading
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Changes in this file I noticed while writing and editing the documentation for the new package. These should be more reliable and avoid false positives like variable identifiers inside of code blocks (which is what I ran into that prompted me to look into it).

"types": [file] # ESLint only accepts [javascript] by default.
language: system
pass_filenames: false
entry: bash -c 'pnpm run eslint --fix'
entry: just eslint
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See the note on the new just recipe for an explanation of why I've moved this from the root package.json to just. TL;DR: just has better default argument handling than package.json scripts, so it's more convenient!

Copy link
Member

Choose a reason for hiding this comment

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

Great idea!

},
"packageManager": "[email protected]",
"engines": {
"node": ">= 20.0.0 <21"
},
"devDependencies": {
"@types/node": "22.5.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@types shared by multiple libraries should ideally live at the workspace root, to ensure they have the same versions. Otherwise, you can run into nasty errors like type Plugin is not compatible with argument type Plugin, where each Plugin reference is just referencing a different version of the type by the same name from the same package. This only holds for devDependencies.

"license": "MIT",
"devDependencies": {
"esbuild": "^0.23.1",
"nock": "14.0.0-beta.11",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Nock only supports fetch as of version 14, which is still in beta. It works fine for these test cases, so I don't think it will be an issue to be pinned to a pre-release version for now.

Comment on lines +77 to +93
/**
* Determine if the current state of the client requires waiting for
* a token refresh to complete. This is distinct from whether
* the client should trigger a token refresh, because if an API token
* already exists, the refresh is eagerly requested before the current
* token expires. That means that if the request triggering the token refresh
* happens within the expiry threshold (see `expiryThreshold`), it
* can proceed with the existing token, as it is still valid.
*
* However, if the token is expired (or no token yet exists), the request
* must be held until the client has an API token.
*
* This is only relevant to clients configured with credentials. Non-credentialed
* clients bypass this process altogether.
*
* @returns Whether the client should wait.
*/
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Understanding this flow might be helped by reading the tests that exercise it, so check there first if you aren't following the approach.

As mentioned in the PR description, the same methodology is proven in the frontend's api-token plugin, except there it blocks the full SSR request until the token returns, rather than just individual API requests. The principle and effect is the same, and I managed to sort out an implementation that works without needing to rely on the third-party async-mutex library like we use in the frontend.


import type { paths } from "./generated/openverse"

export type { components } from "./generated/openverse"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Exporting components lets consumers of the package reference specific models. For example, to get the Image model (say, to define it in a store), you would use components["schemas"].Image, with components imported from @openverse/api-client.

Copy link
Member

Choose a reason for hiding this comment

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

I think the paths should be exported as well, for example to allow the production of types like:

    type Method = paths["/v1/images/{identifier}/"]["get"]
    type SuccessResponse = Method["responses"][200]
    type SuccessBody = SuccessResponse["content"]["application/json"]

which can be quite useful to consumers who want to create type-safe client.GET arguments outside of the function call, as an example use case.

Comment on lines 1 to 4
{
"extends": "./tsconfig.json",
"exclude": ["./tests/**", "./vitest.config.mts"]
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This tsconfig is used when building types for the package. The excludes ensure that only the types appropriate for distribution are included in the package's type output.

Comment on lines +113 to +116
"packages/eslint_plugin/index": "/packages/js/eslint_plugin/index.html",
"packages/eslint_plugin/analytics_configuration": "/packages/js/eslint_plugin/analytics_configuration.html",
"packages/eslint_plugin/no-unexplained-disabled-test": "/packages/js/eslint_plugin/no-unexplained-disabled-test.html",
"packages/openverse_attribution/index": "/packages/python/openverse_attribution/index.html",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I originally moved these into language-specific sub-paths to accommodate the two api-client packages (one @openverse/api-client and the other openverse-api-client). That's no longer present in this PR, but because we know we want a Python API client, this conflict will happen soon. In that case, I'd rather move these to language specific sub-paths sooner than later, thereby reducing the overall number of redirects we have to add in the future. This would also apply if we created a separate @openverse/attribution JS package, for example, and other things of that nature where we need both a JS and Python version.

@sarayourfriend sarayourfriend marked this pull request as ready for review August 30, 2024 05:00
Switching to rollup to match the tool used by our frontend; esbuuilds focus on bundling was over complicating aspects of building the project
Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

This is excellent, and looks great. I'm only blocking on the suggestion that we also export the path types to users. I think access to those will be important to any consumers writing type-safe applications using this client.

Really excellent work! I so look forward to when we implement this on the frontend and delete a lot of code.

Copy link
Member

@zackkrida zackkrida left a comment

Choose a reason for hiding this comment

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

Apologies, I meant to request changes. See my first comment for details.

@sarayourfriend
Copy link
Collaborator Author

Sounds good, thanks for the review @zackkrida! I'll add exporting the paths.

@sarayourfriend
Copy link
Collaborator Author

@zackkrida paths export added in 9da282d

@sarayourfriend sarayourfriend merged commit f61a869 into main Sep 9, 2024
50 checks passed
@sarayourfriend sarayourfriend deleted the add/api-client-generator branch September 9, 2024 23:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🕹 aspect: interface Concerns end-users' experience with the software 🌟 goal: addition Addition of new feature 🟩 priority: low Low priority and doesn't need to be rushed 🧱 stack: api Related to the Django API 🧱 stack: documentation Related to Sphinx documentation 🧱 stack: frontend Related to the Nuxt frontend
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

5 participants