-
Notifications
You must be signed in to change notification settings - Fork 9
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(missing-kind-type): convert auto generated enums to union types #137
Conversation
CLA Assistant Lite bot: I have read the CLA Document and I hereby sign the CLA You can retrigger this bot by commenting recheck in this Pull Request |
@@ -26,7 +26,7 @@ | |||
"prepare": "npm run build", | |||
"prepublishOnly": "npm test && npm run lint", | |||
"graphql:codegen": "graphql-codegen --config graphql-codegen.yml", | |||
"swagger:codegen": " openapi --input https://raw.githubusercontent.com/cowprotocol/services/main/crates/orderbook/openapi.yml --output src/order-book/generated --exportServices false --exportCore false" | |||
"swagger:codegen": "openapi --input https://raw.githubusercontent.com/cowprotocol/services/main/crates/orderbook/openapi.yml --output src/order-book/generated --exportServices false --exportCore false --useUnionTypes" |
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 is the main change. See what the option does in the docs
Everything else is adapting to the consequences of it.
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.
Oh no! do we really need to do this?
We've been using the enums with no issues before.
Is the options 2 possible?
|
It requires changes to the openapi specs in the services repo instead. |
Part of the fix to address cowprotocol/cow-sdk#134 This is the second approach as suggested here ferdikoomen/openapi-typescript-codegen#1424 The first approach was implemented on cowprotocol/cow-sdk#137 It's a breaking change on the SDK side and creates a lot of noise. This change will allows us to fix the issue with minimal changes
Attempt #2, which requires services repo changes: |
# Summary Part of the fix to address cowprotocol/cow-sdk#134 This is the second approach as suggested here ferdikoomen/openapi-typescript-codegen#1424 The first approach was implemented on cowprotocol/cow-sdk#137 It's a breaking change on the SDK side and creates a lot of noise. This change will allows us to fix the issue with minimal changes Describe context of what this change does, why it is needed and how it is accomplished ### Test Plan There are no effective changes on the code per se, and the resulting schema doc should be equivalent. I did test it on the target repo (SDK) and the generated the types with the changes here. See [this PR](https://github.com/cowprotocol/cow-sdk/pull/138/files)
Superseded by #138 |
Summary
Closes #134
Converting openapi enums to union types
This is a breaking change and will have cascading effects on consumers, as the enums will be removed!
Context
We auto-generate the types based on the orderbook/openapi spec.
The OrderQuoteSide is defined here
It uses kind.sell for some options and kind.buy for another.
We use openapi cli tool to generate the corresponding TS types
The resulting type can be seen here
It does NOT include the enum
buy
.It's a limitation of the lib used for generating the types. See this issue.
This change applies the suggested workaround:
Here's a write up about this option.
Test