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

Removes generated API client from repo, replacing with package import #1618

Draft
wants to merge 3 commits into
base: nextjs
Choose a base branch
from

Conversation

jonkafton
Copy link
Contributor

What are the relevant tickets?

Technical debt.

Description (What does it do?)

Removes the generated API client from the repository, replaced by importing @mitodl/open-api-axios from NPM.

Re-exports v1 and v2 for type imports.

How can this be tested?

The main workspace should build and run successfully after installing dependencies.

yarn install
yarn workspace main build
yarn workspace main dev
docker compose up

Additional Context

The API client distribution is published to NPM, though also committed to this repo at the generated client inside the api workspace, ./frontends/api/src/generated.

This is not necessary as it can be imported normally as a dependency. If we do not pin the dependency (use a ranged ^ version) we discourage breaking API changes within our existing v1 and v2 APIs.

@jonkafton jonkafton added the Needs Review An open Pull Request that is ready for review label Sep 27, 2024
@jonkafton jonkafton marked this pull request as draft September 27, 2024 20:13
@jonkafton
Copy link
Contributor Author

jonkafton commented Sep 27, 2024

Needs review, but leaving in draft mode for now to better understand the original reasoning for committing the generated code to the frontend.

Some const "enums" needed to be copies in as were not exported from the latest package:
https://github.com/mitodl/mit-open/pull/1618/files#diff-ef71d7f13c6339b04b97efadc036c226a1702e698feec26807566ed9bc97e8e1R7-R36.

Also to understand whether we should pin the version, whether we can rely on API changes being backwards compatible and otherwise what the process is for coordinating releases.

Do we need to replace the CI job to check the import is update with the OpenAPI schema (failing on this branch now that the generated code has been removed). Does any of this make sense when the API source code also lives in this repo? - the front end is independent from the API in terms of deployment, but they currently deploy together in the same pipeline.

@rhysyngsun
Copy link
Contributor

The historical reason for generating it in the project itself is because it was the easiest way to do it.

As far as backwards compatibility, this github action should be checking for that: https://github.com/mitodl/mit-open/blob/main/.github/workflows/openapi-diff.yml but at the end of the day it's upon engineers making backend changes to ensure that they're building things in a backwards compatible manner. That should mandate at least some minimal testing of the frontend prior to the new client updates being available.

@abeglova
Copy link
Contributor

I'm fine with this but it will make adding new api fields that are used in the ui slower - there will need to be a backend change that gets released before the ui change can be put up for review. If the change is not backward compatible, such as renaming an api key or changing the format of a field that is already in use there will need to be at least 3 releases to make the change.

Probably worth it for eliminating confusing mismatches between the api and the api version in course-search-utils

@rhysyngsun
Copy link
Contributor

@abeglova at this point we shouldn't be doing breaking changes because we should be treating this as a public API. OCW will use this at some point too if it isn't already. So generally this change is going to push us into having to make new functionality backwards compatible or version the API.

@ChristopherChudzicki
Copy link
Contributor

https://github.com/mitodl/mit-open/blob/main/.github/workflows/openapi-diff.yml is currently disabled, by the way.

As far as breaking changes... I think some amount of breaking change should still be acceptable in our APIs in the near future. It certainly isn't ideal, but some ETL stuff is still a bit in flux... e.g., we recently

  • added a format: synchronous | asyncrhonous
  • renamed learning_format: online | in-person | hybrid | offline to delivery based on feedback from some course providers.

I'm not suggesting we should take breaking changes lightly—e.g., we could preserve both fields for a short time, to allow updating some of our internal consumers (like the OER CSV generator). But in the scenario above, having both format and learning_format (for different things) would have been quite confusing. I am (slightly) concerned that this is going to happen more in the near future, and having a separate API endpoint for each time this arises seems cumbersome.

I DO support think using the published API client would be a good idea. It doesn't preclude us from making breaking changes, but it would require us to do it more gracefully (e.g., preserving old field in a rename).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants