-
-
Notifications
You must be signed in to change notification settings - Fork 6.6k
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
[typescript-angular] drop support of angular below 6.0.0 #6360
Conversation
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
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.
Great job @macjohnny !
All looks good to me.
I only have one question re string enums.
Should we preserve some way to emit enums in namespaces?
With real string enums it's the only way to make two structurally identical enums inter-assignable. Which can be a handy workaround for OAS2 limitation (which doesn't allow to re-use a enum and requires to redefine enum for every field).
Example:
Query1:
properties:
- sort:
type: string
enum: [asc, desc]
Query2:
properties:
- sort:
type: string
enum: [asc, desc]
It emits two enums:
enum Query1SortEnum {
asc='asc',
desc='desc'
}
enum Query2SortEnum {
asc='asc',
desc='desc'
}
In TS, enums are not assignable to each other even if structurally compatible. So I can't do this:
let q1!: Query1;
let q2: Query2 = q1;
However, namespaces solve that. If the same enum (with the same name!) is defined in two different namespaces, TS allows assigning one to another. That's what we do in our project currently (with tweaked mustache templates)
namespace Query1 {
export enum Sort {
asc='asc',
desc='desc'
}
}
namespace Query2 {
export enum Sort {
asc='asc',
desc='desc'
}
}
Previosuly, with stringEnums=false
(default) it was not an issue at all, because they were not real enums.
Now we always emit enums, which can introduce this TS issue with OAS2-compliant specs.
Any thoughts?
This raises a philosophical question: Do we want to fix the shortcomings of OAS2 with an implementation that tries to be smart? In the case that you described, what happens if a new enum value is introduced only for |
You will be able to assign
I'm not a big proponent of that approach. I mostly wanted to point out that the change in the PR can have backward-incompatible outcome without a fallback even for angular 8/9 users. On the other hand, e.g. axios generator has never had such |
If it's a limitation only with OAS2, then I would suggest asking users to migrate to OAS3 spec instead. |
ea96716
to
ff1c72d
Compare
In order to minimize the breaking changes, I decided to revert the changes related to the string enums, since the functionality is already implemented and not too much of a burden. |
* master: [PS] Refactor the http signing auth with ecdsa support (OpenAPITools#6397) [Rust Server] Hyper 0.13 + Async/Await support (OpenAPITools#6244) [Rust] set supportAsync to true as the default (OpenAPITools#6480) [php-symfony] Set required PHP version ^7.1.3 (OpenAPITools#6181) update doc [csharp] Rename netstandard to netstandard1.3 (OpenAPITools#6460) feat: support deprecated parameters for typescript-axios generator (OpenAPITools#6475) [REQ][typescript-axios] useSingleRequestParameter should mark parameter optional if all properties are optional (OpenAPITools#6477) better struct alias in rust (OpenAPITools#6470) Migrate Go server samples to OAS 3 only (OpenAPITools#6471) [Rust][reqwest] add async support (OpenAPITools#6464) [codegen][python-experimental] Composed schema with additionalProperties (OpenAPITools#6290) [Java] Decommission Retrofit 1.x support (OpenAPITools#6447) remove scala oas3 samples (OpenAPITools#6446) [Java][jersey2] Fix RuntimeException when HTTP signature parameters are not configured (OpenAPITools#6457) [Java][jersey2] Improve http signature code comments (OpenAPITools#6463) [typescript-angular] drop support of angular below 6.0.0 (OpenAPITools#6360) [cli] new 'author template' command (OpenAPITools#6441) python-experimental updates ancestor + adds descendant discriminator tests (OpenAPITools#6417)
Remove support for old angular versions below 6.0.0, allowing to clean up the code base.
PR checklist
./bin/
(or Windows batch scripts under.\bin\windows
) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh
,./bin/openapi3/{LANG}-petstore.sh
if updating the code or mustache templates for a language ({LANG}
) (e.g. php, ruby, python, etc).master
,4.3.x
,5.0.x
. Default:master
.cc @TiFu (2017/07) @taxpon (2017/07) @sebastianhaas (2017/07) @kenisteward (2017/07) @Vrolijkx (2017/09) @macjohnny (2018/01) @topce (2018/10) @akehir (2019/07) @petejohansonxo (2019/11) @amakhrov (2020/02)