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

fix(ui): Adapt the UI to the latest API changes #224

Merged

Conversation

Etsija
Copy link
Contributor

@Etsija Etsija commented May 13, 2024

The PRs #157, #181 and #223 introduced some major changes to the generated UI API which all need to be adapted in the UI codebase.

To assist with further similar PRs and clearly separate manual changes from auto-generated ones, we do not use the "every commit must pass all checks" convention in this PR.

Please see the individual commits for details.

@Etsija Etsija force-pushed the update-ui-api-new branch from 6d98dca to 9325dc8 Compare May 13, 2024 10:39
@Etsija Etsija changed the title ui: Adopt the UI to the latest API changes fix(ui): Adopt the UI to the latest API changes May 13, 2024
@Etsija Etsija force-pushed the update-ui-api-new branch from 9325dc8 to a703bbb Compare May 13, 2024 10:47
@sschuberth
Copy link
Contributor

Note that IMO #223 should be merged first, and then the API code in here be regenerated afterwards, to reflect the new prettier formatting.

@Etsija
Copy link
Contributor Author

Etsija commented May 13, 2024

The "Build and test" check failing is because of a bug in openapi-react-query-codegen, which I reported, so let's hope it gets fixed soon.

A temporary fix would be adding

// @ts-nocheck

to the two files causing the error, but I'm a bit reluctant to do it, as changing manually a part of software which is auto-generated isn't really wise I think.

The failing test doesn't appear to affect the query client's functionality in any way. If the team verdict is I should do the manual additions to auto-generated files to get "build and test" passing, I will do so.

@mmurto
Copy link
Contributor

mmurto commented May 13, 2024

The "Build and test" check failing is because of a bug in openapi-react-query-codegen, which I reported, so let's hope it gets fixed soon.

A few options:

  1. add the @ts-nocheck manually, document it in the readme and wait for/fix the issue upstream,
  2. disable the rule in tsconfig, which also disables it for our code,
  3. package the API client in a separate package to not trigger the rule.

I'd say 1 is probably the best in short-term. If the manual addition of the headers is cumbersome, we could write a small bash script that is executed with the generator script.

@Etsija
Copy link
Contributor Author

Etsija commented May 13, 2024

The "Build and test" check failing is because of a bug in openapi-react-query-codegen, which I reported, so let's hope it gets fixed soon.

A few options:

1. add the `@ts-nocheck` manually, document it in the readme and wait for/fix the issue upstream,

2. disable the rule in tsconfig, which also disables it for our code,

3. package the API client in a separate package to not trigger the rule.

I'd say 1 is probably the best in short-term. If the manual addition of the headers is cumbersome, we could write a small bash script that is executed with the generator script.

Well it depends on how often the OpenAPI spec will change (I would guess it is still volatile), and how long does the bug fixing in upstream take. I can surely add those lines manually for some time.

@Etsija Etsija force-pushed the update-ui-api-new branch 4 times, most recently from 280b323 to 5e4c191 Compare May 14, 2024 07:15
@sschuberth
Copy link
Contributor

I know we had talked about this, but now that I see the diff of this PR I yet wonder whether it would be feasible to separate purely auto-generated code from the manual changes required.

While that would break with the every-commit-must-pass-all-checks-convention, it would more clearly show what manual changes were needed, which helps if anything similar needs to be done in the future again.

So I'd expected three commits currently:

  1. Auto-generated code
  2. Manual changes needed to ui/src/routes
  3. Manual changes for _queryKey

Etsija added 3 commits May 16, 2024 08:44
OpenAPI query client library update to v1 (PR eclipse-apoapsis#157) introduced major
changes to the structure and functionality of the query client.
Moreover, PR eclipse-apoapsis#181 introduced a fix to OpenAPI generation of nullable
values, which brought major changes to the Swagger JSON.

This commit regenerates the query client from scratch to adapt to these
changes. The command used for regenerating is:

    pnpm codegen

Signed-off-by: Jyrki Keisala <[email protected]>
There is a bug [1] in the codegen tool that causes it to generate unused
query keys for requests with no parameters. This commit renames the
unused query keys to start with an underscore '_' to pass the lint
checks for unused variables.

NOTE: This is not a lasting solution, as every regeneration of the query
client will revert these changes.

[1]: 7nohe/openapi-react-query-codegen#112

Signed-off-by: Jyrki Keisala <[email protected]>
The changed query client lead to some fixes needed in the UI code to
adapt to changes, which are listed here for further reference:

- to access the "message" and "cause" fields of the ApiError body in the
  error toast component, intermediate type casting of error.body to
  "any" was needed. NOTE: it should be investigated, why this has
  changed, as before we had direct access to error.body.message and
  error.body.cause in the UI

- earlier on, the service functions like
  ProductsService.getProductById() were called with passing parameters
  directly, but now the parameters need to be passed as objects

- The openapi-react-query-codegen uses openapi-ts as an underlying
  engine for code generation, and inline enums were broken in the latter
  [2]. While this has already been fixed in openapi-ts, the fix has not
  yet been taken into use in openapi-react-query-codegen [1]. Until the
  openapi-react-query-codegen is updated, the inline enums need to be
  accessed differently, as in

    z.nativeEnum(CreateRepository.type) needs to be changed into
    z.enum($CreateRepository.properties.type.enum)

  Note that when the library is updated, this change probably needs to
  be reverted back to its original form.

[1]: 7nohe/openapi-react-query-codegen#110
[2]: hey-api/openapi-ts#547

Signed-off-by: Jyrki Keisala <[email protected]>
@Etsija Etsija force-pushed the update-ui-api-new branch from 5e4c191 to 421ce45 Compare May 16, 2024 06:19
@Etsija Etsija changed the title fix(ui): Adopt the UI to the latest API changes fix(ui): Adapt the UI to the latest API changes May 16, 2024
@MarcelBochtler MarcelBochtler added this pull request to the merge queue May 16, 2024
Merged via the queue into eclipse-apoapsis:main with commit 0fd1858 May 16, 2024
10 checks passed
@sschuberth sschuberth deleted the update-ui-api-new branch May 16, 2024 10:11
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.

4 participants