Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[typescript-angular] Add option to generate tagged unions #7245

Merged
merged 2 commits into from
Feb 1, 2018
Merged

[typescript-angular] Add option to generate tagged unions #7245

merged 2 commits into from
Feb 1, 2018

Conversation

eriktim
Copy link
Contributor

@eriktim eriktim commented Dec 22, 2017 •
edited
Loading

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.

PR checklist

  • Read the contribution guidelines.
  • Ran the shell script under ./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\.
  • Filed the PR against the correct branch: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

This PR adds an option enabling Tagged union types for TypeScript. For the pet-example this outputs the following models:

// pet.ts

export type Pet = Cat | Dog;

// cat.ts

/**
 * A representation of a cat
 */
export interface Cat { 
    name: string;
    petType: 'Cat';
    /**
     * The measured skill for hunting
     */
    huntingSkill: Cat.HuntingSkillEnum;
}

export namespace Cat {
    export type HuntingSkillEnum = 'clueless' | 'lazy' | 'adventurous' | 'aggressive';
    export const HuntingSkillEnum = {
        Clueless: 'clueless' as HuntingSkillEnum,
        Lazy: 'lazy' as HuntingSkillEnum,
        Adventurous: 'adventurous' as HuntingSkillEnum,
        Aggressive: 'aggressive' as HuntingSkillEnum
    }
}

// dog.ts

/**
 * A representation of a dog
 */
export interface Dog { 
    name: string;
    petType: 'Dog';
    /**
     * the size of the pack the dog is from
     */
    packSize: number;
}

as opposed to

// pet.ts

export interface Pet { 
    name: string;
    petType: string;
}

// cat.ts

/**
 * A representation of a cat
 */
export interface Cat extends Pet { 
    /**
     * The measured skill for hunting
     */
    huntingSkill: Cat.HuntingSkillEnum;
}
export namespace Cat {
    export type HuntingSkillEnum = 'clueless' | 'lazy' | 'adventurous' | 'aggressive';
    export const HuntingSkillEnum = {
        Clueless: 'clueless' as HuntingSkillEnum,
        Lazy: 'lazy' as HuntingSkillEnum,
        Adventurous: 'adventurous' as HuntingSkillEnum,
        Aggressive: 'aggressive' as HuntingSkillEnum
    }
}

// dog.ts

/**
 * A representation of a dog
 */
export interface Dog extends Pet { 
    /**
     * the size of the pack the dog is from
     */
    packSize: number;
}

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx

@eriktim
[angular] Add option to generate tagged unions
23ecd28 
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.
@TiFu
Copy link
Contributor

TiFu commented Dec 22, 2017

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.

@kenisteward
Copy link
Contributor

kenisteward commented Dec 22, 2017 via email

@kenisteward
Copy link
Contributor

kenisteward commented Dec 22, 2017 via email

@eriktim
Copy link
Contributor Author

eriktim commented Dec 22, 2017

@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 x-discriminator-value instead of the mapping introduced in 3.0.

To name a few benefits of tagged unions:

  • Types are guaranteed and distinctable with tagged unions. That is 
    const cat: Cat = {
    name: 'Hachi',
    petType: 'Dog',
    huntingSkill: 'clueless'
}
    would be fine with inheritance, not with tagged unions. It's like making impossible state impossible;
  • For TypeScript, there is no difference when using generics or types, e.g. function clone<T extends Pet>(pet: T): T {...} will work in either case. There are no classes here;
  • As described by typescriptlang.org or even clearer in this blog post (see describePaymentMethod), it is very convient to switch over all types (and to ensure you actually process all types) and to have TypeScript know what type you are working with without adding type annotations.

@TiFu
Copy link
Contributor

TiFu commented Dec 23, 2017

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.
If this is added, it will need very good documentation describing exactly in which cases it breaks.

@wing328 wing328 added this to the v2.4.0 milestone Dec 27, 2017
@eriktim
Copy link
Contributor Author

eriktim commented Dec 27, 2017

@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.

@eriktim
Copy link
Contributor Author

eriktim commented Jan 24, 2018 •
edited
Loading

@wing328 Can you please help us out?

I was thinking we could also make the option allow two modes: one with only tagged unions and one where an additional type allows for any value, like

export type Pet = Cat | Dog | OtherPet;

// where

interface Cat {
  petType: 'cat';
  ...
}

interface Dog {
  petType: 'dog';
  ...
}

interface OtherPet {
  petType: string; // <-- this allows for any pet
  ...
}

Edit: Apparently, that doesn't seem to be possible (see microsoft/TypeScript#13467).

@wing328
Copy link
Contributor

wing328 commented Jan 25, 2018

@trenneman I'll take a look by this weekend and reply back

@wing328
Copy link
Contributor

wing328 commented Jan 28, 2018

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)

@eriktim
Copy link
Contributor Author

eriktim commented Jan 30, 2018

Thanks, @wing328. Would this option still be legitimate in OpenAPI 3.0 then?

@TiFu what do you think of @wing328 's suggestion? Does defaulting to false and adding documentation seem feasible to you?

@TiFu
Copy link
Contributor

TiFu commented Jan 31, 2018

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.

@wing328 wing328 merged commit 157e6b7 into swagger-api:master Feb 1, 2018
@kenisteward
Copy link
Contributor

kenisteward commented Feb 1, 2018 via email

@kenisteward
Copy link
Contributor

kenisteward commented Feb 1, 2018 via email

@TiFu
Copy link
Contributor

TiFu commented Feb 1, 2018

since sub 3 doesn't support descriptors
@kenisteward Descriptors or discriminators? The latter is included in Open API Spec 2

Does 3.0.0 clarify the issues with non-abstract base types? My understanding is that it still doesn't address this issue.

@eriktim
Copy link
Contributor Author

eriktim commented Feb 1, 2018

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.

@eriktim eriktim deleted the typescript-angular-tagged-unions branch February 1, 2018 19:25
@kimamula
Copy link

@wing328 Is this to be included in the next release?

In addition to the concern described above, I found another problem regarding readOnly.
readOnly in OpenAPI "means that it MAY be sent as part of a response but MUST NOT be sent as part of the request", while declaring properties as readonly in TypeScript has no effect on preventing them from being sent as part of the request.
Therefore, the change introduces unnecessary restriction only to annoy users.

@spike-rabbit spike-rabbit mentioned this pull request Oct 28, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
Client: TypeScript Enhancement: General
Projects
None yet
Milestone
v2.4.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@eriktim @TiFu @kenisteward @wing328 @kimamula