-
Notifications
You must be signed in to change notification settings - Fork 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] Add option to generate tagged unions #7245
[typescript-angular] Add option to generate tagged unions #7245
Conversation
Using the option `taggedUnions` will create a union type for each parent type instead of extending interfaces. The union types are tagged by using the discriminator values. And also: * Add support for aliases; * Add support for read-only properties.
This solution assumes that the parent type is abstract and cannot be returned by the API. |
@TiFu you beat me to it. Also I thought discriminators weren't fully
supported for less than 3.0
…On Fri, Dec 22, 2017, 7:45 AM Tino Fuhrmann ***@***.***> wrote:
This solution assumes that the parent type is abstract and cannot be
returned by the API.
Is returning the parent class prohibited by the openAPI specification if a
discriminator field is present?
From reading the spec i'm not sure that is the case.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7245 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtfdPYRjMSRkQER7BGdUhMPWCqE1Sks5tC6RsgaJpZM4RK9mr>
.
|
Also what is the benefit of using unions ? If you have a base type wouldn't
it be preferred to actually have the type generated.
On Fri, Dec 22, 2017, 8:01 AM Kenneth Steward <[email protected]>
wrote:
… @TiFu you beat me to it. Also I thought discriminators weren't fully
supported for less than 3.0
On Fri, Dec 22, 2017, 7:45 AM Tino Fuhrmann ***@***.***>
wrote:
> This solution assumes that the parent type is abstract and cannot be
> returned by the API.
> Is returning the parent class prohibited by the openAPI specification if
> a discriminator field is present?
> From reading the spec i'm not sure that is the case.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#7245 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AMPLtfdPYRjMSRkQER7BGdUhMPWCqE1Sks5tC6RsgaJpZM4RK9mr>
> .
>
|
@TiFu this indeed assumes the parent type is abstract and as far as I understand this is what the spec suggests: "If the discriminator value does not match an implicit or explicit mapping, no schema can be determined and validation SHOULD fail." But even if this was not the case, you can always decide not to enable tagged unions. @kenisteward I know the support for discriminators has been improved in the 3.0 spec. Nevertheless they are already there in 2.0, at least for Spring & Elm (and probably others). Currently, this still requires the vendor extension To name a few benefits of tagged unions:
|
Correct me if I'm wrong, but the part you quoted does not prohibit an API Endpoint returning a Pet (just a Pet, not a subclass of Pet). Maybe @wing328 can clarify the interpretation of the OpenAPI spec.. If this is indeed the case I wouldn't feel very comfortable adding this option to the TypeScript code generator. Generating invalid code due to an option is very bad from a user standpoint, because it violates the contract defined by the code generator: valid json in, valid code out. |
@TiFu to me the spec seems to describe just that having this generic Pet is not allowed. Also, having a strict set of sub-types makes the most sense to me. But I agree it is not specified very clearly. What do you think, @wing328 ? If this is not the case I agree it should be made very clear what the impact of enabling this option is. However, I do feel it can be a great addition to handling your remote data with TypeScript. |
@wing328 Can you please help us out?
Edit: Apparently, that doesn't seem to be possible (see microsoft/TypeScript#13467). |
@trenneman I'll take a look by this weekend and reply back |
My take is to add the option given that it's defaulted to false and as you guys have pointed out, the support of discriminator in OpenAPI 2.0 was not very mature (but that should not prevent us from adding additional feature to the generator) |
I am fine with that. @kenisteward what about you? We just have to remember, that we might have to remove it again for OpenAPI Spec 3.0. Let's see if William knows more about that. |
Looks like I was late to the party :( @TiFu I think I would have preferred
with everything under 3 not having the feature since sub 3 doesn't support
descriptors
This should have been a feature written for 3.0.0 with that support in mind
…On Thu, Feb 1, 2018, 4:21 AM William Cheng ***@***.***> wrote:
Merged #7245 <#7245>.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#7245 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AMPLtWiXJB7HKQtRxJKxDR_PHdVYNBwFks5tQYH_gaJpZM4RK9mr>
.
|
With 3.0.0 we wouldn't even need an option because it should be known as
default when you have a descriminator
…On Thu, Feb 1, 2018, 7:54 AM Kenneth Steward ***@***.***> wrote:
Looks like I was late to the party :( @TiFu I think I would have preferred
with everything under 3 not having the feature since sub 3 doesn't support
descriptors
This should have been a feature written for 3.0.0 with that support in
mind
On Thu, Feb 1, 2018, 4:21 AM William Cheng ***@***.***>
wrote:
> Merged #7245 <#7245>.
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> <#7245 (comment)>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AMPLtWiXJB7HKQtRxJKxDR_PHdVYNBwFks5tQYH_gaJpZM4RK9mr>
> .
>
|
Does 3.0.0 clarify the issues with non-abstract base types? My understanding is that it still doesn't address this issue. |
That's a quick merge all of the sudden. I do not know the details of 3.0, but I agree this should only be in if it complies to that spec. And if so, we probably want to add the option there as well. |
@wing328 Is this to be included in the next release? In addition to the concern described above, I found another problem regarding |
Using the option
taggedUnions
will create a union type for each parenttype instead of extending interfaces. The union types are tagged by using
the discriminator values.
And also:
PR checklist
./bin/
to update Petstore sample so that CIs can verify the change. (For instance, only need to run./bin/{LANG}-petstore.sh
and./bin/security/{LANG}-petstore.sh
if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates). Windows batch files can be found in.\bin\windows\
.3.0.0
branch for changes related to OpenAPI spec 3.0. Default:master
.Description of the PR
This PR adds an option enabling Tagged union types for TypeScript. For the pet-example this outputs the following models:
as opposed to
cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx