-
Notifications
You must be signed in to change notification settings - Fork 383
Decouple ui/* from api/* via openapi types #4725
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
base: fe/feature/RI-7039-replace-eui
Are you sure you want to change the base?
Conversation
…into feature/resolve-ts-issues
Code Coverage - Integration Tests
|
Code Coverage - Backend unit tests
Test suite run success2940 tests passing in 286 suites. Report generated by 🧪jest coverage report action from 5c05cfb |
Code Coverage - Frontend unit tests
Test suite run success4802 tests passing in 631 suites. Report generated by 🧪jest coverage report action from 5c05cfb |
@@ -172,7 +174,6 @@ | |||
], | |||
"moduleNameMapper": { | |||
"src/(.*)": "<rootDir>/$1", | |||
"apiSrc/(.*)": "<rootDir>/$1", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
was this unused?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think so
Great job on this, using an OpenAPI contract to keep services in sync is a solid and widely used approach, especially when different tech stacks are involved or when the code lives in separate repos. In those situations, generating types from Swagger docs makes it easier to keep services aligned and follow the correct communication protocol. That said, in our case, we're working in a monorepo, and both the backend and frontend use TypeScript. I don't want to sound too negative, but adding a (semi-automatic) code generation tool here feels like extra complexity and might hide the real issue that we don't have a single source of truth for our types. Instead of adding another layer, I think it would be more effective to focus on fixing the type mismatches directly. We could do this by adjusting how the frontend uses the types, or better yet, by introducing a shared types package (or something similar) so both sides rely on the same definitions. Another thing to consider is that OpenAPI doesn't always match perfectly with the actual TypeScript code, so I think it's better to stick to the code interfaces as the main source of truth. That said, if there are plans to move away from the monorepo and split the services, then generating types from Swagger would absolutely make sense, and it will probably be the best approach. I'm not against it overall, just think we should focus on solving the core issue first. So, I'll be happy to chat more about this so we can agree on which is the best way to reduce these TypeScript errors and resolve the type mismatches between the BE and the FE. |
I don't think BE types should be used at all on FE, besides in the apiService.
Since we're passing via the API, using the same types is a lie for anything that is not supported by JSON and converting from json to buffer for example is a waste since we don't use it as buffer anywhere |
…into feature/resolve-ts-issues
Issue
Currently we have a lot of typescript issues on the client side. Many of them are due to references to the nestjs api and/or mismatch between api contracts.
What this PR does
It addresses decoupling the
ui/*
fromapi/*
be leveraging auto generated types based on openapi (swagger).It shrinks the tsc errors by about 50% (from ~2500 to ~1300) and sets the path for further ts fixes.
Review guide
This PR appears large at first but that's because of the large amount of autogenerated types. Do not review the
ui/src/api-client
folderNotes
generate-client
script needs to be rerun in order to generate the new types for the client. Not sure if this can be automated.api/clinet/api.ts
)