-
Notifications
You must be signed in to change notification settings - Fork 0
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
Migrate to OpenAPI Generator #26
Conversation
The Swagger Codegen project was forked a few years back and the majority of the community moved over to the forked version [1]. This explains why we were facing some odd issues and a lack of support for the Open API specification 3.1 Moving to this more maintained version of the codegen project will immediately result in some bug fixes (specifically, `object` will be mapped to `object` instead of `any`). It will also allow us to use 3.1 in our api specification. Eventually this change may allow us to leverage community-defined typescript templates instead of our own custom templates. Issue #25 [1] https://github.com/OpenAPITools/openapi-generator/blob/master/docs/qna.md
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.
This seems like it's going to be very cool!
In addition to the any
→ object
fix you noted, I saw these large changes. Just want to verify with you that they're improvements, and not regressions.
- All
createdAt
fields are now emitted asstring
instead ofDate
. I'm pretty sure this is what you intended anyway, but it's not what was happening in the Swagger version. Example:--- typescript-swagger/src/types/ApplicationForm.ts 2024-05-22 17:02:03 +++ typescript-openapi/src/types/ApplicationForm.ts 2024-05-22 16:09:28 @@ -8,7 +8,7 @@ opportunityId: number; readonly version: number; fields: Array<ApplicationFormField>; - readonly createdAt: Date; + readonly createdAt: string; } export type WritableApplicationForm = Writable<ApplicationForm>
- All
*Bundle
types didn't used to emit atotal
; now they do. Example:--- typescript-swagger/src/types/ApplicationFormBundle.ts 2024-05-22 17:02:03 +++ typescript-openapi/src/types/ApplicationFormBundle.ts 2024-05-22 16:09:28 @@ -1,13 +1,11 @@ import { ApplicationForm, } from './ApplicationForm'; -import { - Bundle, -} from './Bundle'; import { Writable } from './Writable'; export interface ApplicationFormBundle { entries: Array<ApplicationForm>; + total: number; } export type WritableApplicationFormBundle = Writable<ApplicationFormBundle>
CreatedBy
seems to have been renamedGetBulkUploadsCreatedByParameter
.
Numbers 1 and 2 look like clear fixes. Number 3 I don't know about.
Even though it's a fix, Number 1 is effectively a breaking change for us on the front-end. (When we started using the real SDK types, we had to use Date
objects for our mock data's createdAt
fields, like this.)
Leaving this as a comment so you can let me know if any of this means you want to tweak things on this PR before merging.
@slifty My review above was actually written under a bit of pressure to finish my day, and I understated how useful this will be. I'm really excited about it! Also, the bit about it being a breaking change wasn't meant to be dismissive -- I think it's perfectly fine that the front end will need to change to adapt to this. I was just noting it so you could decide if that changed the rollout/versioning strategy. (We're pre-1.0 so I assume not, just FYI though.) |
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.
Going ahead and approving this because I think all the items I brought up in my previous comment-only review are either (1) improvements that came along with an improved generator, or (2) unrelated things that should be resolve in a follow-up PR.
Ooooooo really appreciate the deep dive -- our lack of test suite for this makes changes to the sdk much more fraught. I did NOT realize all those fixes -- the I'll ping you offline to see if we can maybe briefly pair on this, or alternatively I'll try to figure it out and circle back here. That last thing I'd like to avoid, since |
This may be famous last words, but re-reading the above it looks like everything is all set at this point, and the next step is to update the openAPI spec. That may culminate in the need for more changes here, but we'll cross that bridge if we get there. |
This PR moves the sdk tools to use OpenAPI Generator instead of Swagger Codegen. OpenAPI Generator is a community fork, and is much more actively maintained. This move will allow future SDK builds to benefit from some bug fixes in the spec mapping to types, as well as support version 3.1 in future (see PhilanthropyDataCommons/service#854)
Resolves #25
You should be able to test this by following the README to generate the SDK -- check out the
PlatformDataProviderResponse
type and notice howdata
is anobject
rather thanany
🎉