[TypeScript][Angular] represent swagger enums as union of literal types

[bedag-moo]

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: master for non-breaking changes and 3.0.0 branch for breaking (non-backward compatible) changes.

Description of the PR

This PR changes the angular-typescript generator to emit a union of literals for swagger enums, e.g.:

export type StatusEnum = 'open' | 'inProgress' | 'resolved' | 'closed';

or

export type ResponseCode = 200 | 404;

enabling their easy use in angular templates, structural subtyping
among enums (in particular, different instances of the same enum
are now mutually assignable), improving type safety by preventing
incorrect widening, and permitting numeric enum values
(albeit without descriptive names)

Fixes #6206, #5146, #3500

[bedag-moo]

The Travis failure seems to be an infrastructure issue unrelated to this PR:

[ERR!] GitHub API: GitHub rate limit reached.
To increase the limit use GitHub authentication.
See: https://github.com/DefinitelyTyped/tsd#tsdrc

[bobvanderlinden]

This change is very much appreciated. I'd like to copy my JSON example for the schema into typescript and have it checked. At the moment this isn't possible because all enum values need to be changed to their reference even though it's structurally the same and the server would send that exact JSON as well.

In addition, this would also solve problems with numerical (or other) enums.

That said, this PR at the moment contains a number of unrelated changes, which might lessen motivation to merge for the Swagger maintainers.

[bobvanderlinden]

There seems to be many changes that were made by your editor/linter. It would help if those were removed from the PR (or made in a separate PR).

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

This change seems unrelated.

[bobvanderlinden]

Hmm, I just saw that this PR was made against 2.3.0, but this is actually a breaking change. Existing implementations that rely on the TypeScript codegen will break. I'm guessing you need to make the PR against 3.0.0.

[bedag-moo]

It's a breaking change for an unreleased feature, so I thought the earlier release was fine as well. (TypeScript support in 2.2.x is pretty much nonexistent AFAIK), but of course I'd be happy to rebase if a maintainer asked.

As you can see from the commit history, the "unrelated changes" were made by the shell scripts the PR checklist instructed me to run. If the maintainers don't want that, they can cherry-pick my first commit instead (and update the PR checklist).

[bobvanderlinden]

You're absolutely right. Sorry for the confusion. Tested it locally on an existing project and it is working wonderfully. Hopefully this PR will be merged soon.

[bedag-moo]

@wing328: Hm ... why did you change the base branch? This PR builds upon other work in the 2.3.0 branch that (as far as I know) has not yet been merged to master. Do you mean to tell me to rebase on master?

As an aside, is there anything I can do to accelerate the review process?

[wing328]

@bedag-moo as explained in another thread, 2.3.0 is now the master after we released stable version v2.2.3

Please rebase on the latest master to help resolve the merge conflicts.

Also thanks @bobvanderlinden for testing this PR.

[bedag-moo]

I manually merged my changes (a rebase didn't work as the files had been moved), and regenerated the samples.

[bobvanderlinden]

@bedag-moo Could you give this another look?

[bedag-moo]

@bobvanderlinden: What do you want me to look at?

[bobvanderlinden]

Sorry, meant to mention @wing328 . That's what I get replying from mobile ;)

@wing328 could you give this another look?

[wing328]

cc @TiFu @taxpon @sebastianhaas @kenisteward @Vrolijkx for review.

[sebastianhaas]

I'd vote for the style-related changes I've pointed out- apart from that this looks good to me :) Thanks for the PR

[sebastianhaas]

Please don't remove the newline in the end. (https://stackoverflow.com/questions/729692/why-should-text-files-end-with-a-newline)

[sebastianhaas]

See above.

[sebastianhaas]

I'm afraid you accidentally inserted an i there 🙈

[bedag-moo]

Oops, indeed. Fixed.

[wing328]

Tests passed via https://travis-ci.org/swagger-api/swagger-codegen/builds/284619142

[bedag-moo]

Because the typescript compiler gives compilation errors when you try to do the stuff I describe there. In particular, check out the following code:

enum IssueStatusEnum {
  Open = 'open',
  InProgress = 'inProgress',
  Resolved = 'resolved',
  Closed = 'closed',
}

// used in a different place in the api, but conceptually the same enum
enum TransitionTargetEnum {
  Open = 'open',
  InProgress = 'inProgress',
  Resolved = 'resolved',
  Closed = 'closed',
}

const badgeType: {[K: IssueStatusEnum]: string} = {
  Open: "danger",
  InProgress: "warning",
  Resolved: "info",
  Closed: "success",
}

let t: TransitionTargetEnum;
t = IssueStatusEnum.Open;

which gives

Error:(41, 20) TS1023:An index signature parameter type must be 'string' or 'number'.
Error:(49, 1) TS2322:Type 'IssueStatusEnum.Open' is not assignable to type 'TransitionTargetEnum'.

[topce]

@bedag-moo
Can you try to compile with TypeScript version 2.4 or later?

[topce]

@bedag-moo
Can you try in playground ?
and send me link
https://www.typescriptlang.org/play/

[bedag-moo]

I did compile with Typescript 2.4, and if you copy the above code to the playground you can see the errors yourself.

[topce]

@bedag-moo
code bellow compiles

enum IssueStatusEnum {
  Open = 'open',
  InProgress = 'inProgress',
  Resolved = 'resolved',
  Closed = 'closed',
}

// used in a different place in the api, but conceptually the same enum
enum TransitionTargetEnum {
  Open = 'open',
  InProgress = 'inProgress',
  Resolved = 'resolved',
  Closed = 'closed',
}

const badgeType: {[K: IssueStatusEnum]: string} = {
  Open: "danger",
  InProgress: "warning",
  Resolved: "info",
  Closed: "success",
}

let t: TransitionTargetEnum | IssueStatusEnum;
t = IssueStatusEnum.Open;```

[topce]

@bedag-moo
let t: TransitionTargetEnum | IssueStatusEnum;

[topce]

@bedag-moo @wing328 @sebastianhaas
On positive side of this pull request is that generated js code
is more efficient !
So in my opinion this could be not default option in code generation

[topce]

@bedag-moo
const could be replace by function
that take enum parametar and return string

[bedag-moo]

... and how would you implement that function without sacrificing clarity or type safety?

[topce]

@bedag-moo
switch case by enum or union of enums
not sure why you have two enums with different name !?
maybe there is some more elegant solution should not be too difficult

[topce]

@bedag-moo
for example

`function getBadgeType1(issueStatusEnum: IssueStatusEnum): string {
  switch (issueStatusEnum) {
    case IssueStatusEnum.Open: {
      return "danger";
    }
    case IssueStatusEnum.InProgress: {
      return "warning";
    }
    case IssueStatusEnum.Resolved: {
      return "info";
    }
    case IssueStatusEnum.Closed: {
      return "success";
    }
  }
}`

[topce]

@bedag-moo @sebastianhaas @bobvanderlinden

If you wanted not to break compatibility with previous version
it was possible by still keeping old enums with cast any
and add new type with union of string literals
and finally create new type as union of those types
With this done in model in api use new union type
So if end user pass old Enum it is OK
if user pass string it also pass.
When migrating to angular 5 just remove any in enum

[topce]

@bedag-moo @bobvanderlinden @sebastianhaas @wing328
for example

type IssueStatusType = 'open' | 'inProgress' | 'resolved' | 'closed'; 
enum IssueStatusEnum {
  Open = 'open',
  InProgress = 'inProgress',
  Resolved = 'resolved',
  Closed = 'closed'
}
type IssueStatus = IssueStatusType | IssueStatusEnum ;
// export all types so user use what she want
// use in rest api calls IssueStatus 

Just trying to help ;-)
Good work guys !!!

[bobvanderlinden]

@topce Thanks for the clarification. My main issue was that I couldn't use JSON as test fixtures because string wasn't accepted as a value for an enum. Like in this example.

This PR was a good alternative, as swagger defines possible string or int values and doesn't really define an enum. These concepts are unfortunately not the same in TypeScript.

Your example illustrates a good workaround to keep everyone happy. Even though the implementation doesn't look that pretty to me, it does seem like a good solution for swagger-codegen for the moment.

That said, it is dawning on me that this is probably a deficiency of TypeScript itself. TypeScript should be able to assign a constant 'string' to an enum MyString { StringValue = 'string' }, but at the same time do not allow arbitrary values to be assigned. Would all of you agree that this is a desired feature in TypeScript? In that case I'd gladly make an issue.

EDIT: I found a matching issue on TypeScript, but it is currently closed unfortunately.

[topce]

@bobvanderlinden Interesting. Thank you for explanation!
If TS team does not wont to fix it workaround would be

enum ExampleEnum {
    ExampleValueA = 'a',
    ExampleValueB = 'b'
}

type ExampleType = 'a' | 'b';

type Example = ExampleType | ExampleEnum; 

const example: Example = 'a';

[bobvanderlinden]

I found that it isn't really a good workaround when the rest of your code uses ExampleEnum or ExampleType. You probably do not want to use Example in all of your code because it removes the ability to (for instance) do exhaustive switch-cases.

Additionally, the behavior of string-enums and string-unions being differently typed is intended (see this new post). It's unfortunate, but we'll have to deal with that.

That being said, @RyanCavanaugh mentioned here that the intent of string-enums is to be able to change its underlying value without changing all of the code that relies on it. In the case of Swagger and this PR always need to change the code when the swagger definition changes, so I'd say string-union is appropriate and string-enum is not.

[topce]

I put in my previous comments pros and cons for both solution.
I understand that @bedag-moo and @bobvanderlinden
value more simplicity then compatibility with previous version .
I respect your opinion and arguments .

[kenisteward]

@bobvanderlinden isn't the purpose of a enum so you don't mistype or misasign values?

I have never come into a case where I've had an enum and wanted to assign the value myself instead of just using Enum.Value

Especially with intellisense it's easier to do enum.value than to type in 'somevalue' and have it tell you that it's wrong.

I hate that I missed this whole conversation. In the case of enums, I think it is much better practice to just do Enum.Value

Please let me know why you would like to do myenum:Enum = 'EnumVal' instead.

I would definitely be more inclined to accept this change if you could still do Enum.Value.

@sebastianhaas Do you have any opinion on this?

[kenisteward]

I've found a better solution that i'll be adding soon that will allow for both and backwards comparability to 1.8

[kenisteward]

@bedag-moo @bobvanderlinden @topce @Vrolijkx @TiFu @taxpon
I made improvements on this in #6788 Now we have full backward compatibility with those who use enums in the MS prescribed way as well as those who want to set it via the value itself (which you can technically do with regular enums. But this makes it to where we should be able to use any string type in the future as well.

type MyStringEnum = "member1" | "member2";

const MyStringEnum = {
    Member1: "member1" as MyStringEnum,
    Member2: "member2" as MyStringEnum
};

// implicit typing example
let myVariable = MyStringEnum.Member1; // ok
myVariable = "member2";                // ok
myVariable = "some other value";       // error, desired

// explict typing example
let myExplicitlyTypedVariable: MyStringEnum;
myExplicitlyTypedVariable = MyStringEnum.Member1; // ok
myExplicitlyTypedVariable = "member2";            // ok
myExplicitlyTypedVariable = "some other value";   // error, desired

this allows us to not have to check for typescript as everyone by now should be at least past 1.8. This gives the best of both worlds.

Since this is relatively harmless and requires less programming that determining what typescript they have (we can force a peer dependency of 1.8 or higher) I think i prefer this over true string enums even though I use true string enums in app development. Here compatibility would be better expecially because no one should change this code anyway.

Credit: https://stackoverflow.com/a/35257367 David Sherret.

TS Playground

[bobvanderlinden]

I'm not sure that's actually the desired behavior. From typescripts perspective that enum now has 4 possible values instead of 2. A switch will always need a default: even though this will never be used at runtime.

I still think this is a defect of typescript itself, but as mentioned in microsoft/TypeScript#17690 it is the intended behavior.

As for swagger, it's a choice of 3 evils. I'm not sure which one is best. If backwards compatibility is imported I'd say to revert this PR. The new suggestion will mess up switch statements in existing projects.

I still like hat we can use example json structures in tests and mocks. Even though auto completion does not always work nicely, the typechecker will still inform you about typos.

It's just a preference, opinions of more people are very welcome.

[bedag-moo]

Meta: Isn't it rather late to discuss this after merging the PR? I filed an issue about the proposed change back in July (#6206) and waited weeks before implementation, then I implemented it and it spents months in review, got merged, and now we start talking?

But since all interested parties are now here, let me reiterate the reasons for this change:

1. In swagger, enums are an anonymous collection of possible values. Typescript union types are an anonymous collection of possible values. Typesscript enums are named types, and assignment compatibility is checked by name rather than the set of possible values. To preserve assignment compatibility for different uses of an enum in the swagger contract, swagger-codegen would have to reuse the typescript enum declaration. This would require a major extension of swagger-codegen, because such duplicate elimination is not currently supported by the code generation engine (as far as I know).
2. union types offer equivalent compile time type safety to enums
3. union types offer code completion in visual studio code (if you trigger code completion within the string literal, it will suggest possible values), but not (yet?) in IntelliJ.
4. union types can be used as index types, but enum types can not:

   const colorOf: {[S in Issue.StatusEnum]: string} = {
     open: 'danger',
     closed: 'success',
     resolved: 'info',
     inProgress: 'warn'
   }
   

   which can be used like

   const color = colorOf[issue.status];
   

   such simple lookup tables are therefore only possible for union types.
5. union types can be easily used in templates, i.e.

   <div [ngSwitch]="issue.state">
     <div *ngSwitchCase="'open'"> 
   

   while enum types additionally require the enum to be imported and published in a component field so we can use it in the template.

Any revision of this PR should continue to support these 5 properties (assignability, type safety, code completion, support for index types, ngSwitch). If we can additionally improve code completion, and not break backwards compatibility, I'm all ears :-)

@kenisteward : That's an interesting idea, because it preserves the above properties while emulating the previous api. (Bob, I could not reproduce your statement about 4 possible values; the playground says there is a variable and a type named MyStringEnum, but typescript seems to treat these as separate declarations?)

Thinking of switch statements however, I did find a small disadvantage of Keni's pattern, because typescript seems unable to verify exhaustiveness for non-literal switch cases:

function indirect(e: MyStringEnum) {  // "not all code paths return a value" under -noImplicitReturn
    switch (e) { 
        case MyStringEnum.Member1:
            return 1;
        case MyStringEnum.Member2:
            return 2;
    }
}

function direct(e: MyStringEnum) { // does not error under -noImplicitReturn
    switch (e) {
        case "member1":
            return 1;
        case "member2":
            return 2;
    }
}

playground link

But as switches over actual enum types are not considered exhaustive either (without a default), this merely mirrors the previous state.

Whether the improved backwards compatibility and code completion is worth the added complexity in the generator is a matter of taste; I am fine with either solution as long as my precious union types are not rolled back ;-)

[kenisteward]

@bedag-moo My apologizes for not fully analyzing this sooner. I only commented here to keep the conversation in an organized area when I could have just opened a new issue to append changes to this.

1. In swagger, enums are an anonymous collection of possible values. Typescript union types are an anonymous collection of possible values.

This may be true, but when creating these clients we need to think of how people will (and want) to consume them. Generally, when a person thinks of an enum, (I think) they assume that they are going to use:

let e: Enum = Enum.Value

and not

let e: Enum = 'value'

because of this it would be important to support actual enums or at least the facade of being one

2 union types offer equivalent compile time type safety to enums

truth

3. union types offer code completion in visual studio code (if you trigger code completion within the string literal, it will suggest possible values), but not (yet?) in IntelliJ.

OH.....MY.....GOD how have i missed this freaking feature! lol. This is great! Instantly just enabled this! However, because i'm not sure if all IDE's implement this support but I know that the ones that support TS do support enum completion that is why I say we should still support let e: Enum = Enum.Value

4 union types can be used as index types, but enum types can not

This works perfectly for me with an enum. Not sure why it isn't working for you.
TS Playground

5 union types can be easily used in templates

So enums are just objects when they are translated. Thinking of it in that light, you can't expose an object to the template unless it is a property of that component. That's just Angular specification.

But to that note you can do exactly the same thing for an enum in the template.

// somewhere else
export class SomeType {
   someValue: Enum
   constructor() {
    // sets someValue to what you want it to be
   }
}
// in some componnent
@input() templateDataValue: SomeType;

// in some component template
<div [ngSwitch]="templateDataValue.someValue">
  <div *ngSwitchCase="'open'"> 

Did you have some runtime error or other issue?

Thinking of switch statements however,

I did the same thing using the original type union and it also didn't force a default. I also didn't get any errors when I took out a whole value.

Note: I'm using 2.5.2

Edit: I didn't have returns set at all so it wasn't giving me the error. I reproduced the same thing that both you and bob said in the sense that you are required to provide a default if using noImplcitReturns

[kenisteward]

@bobvanderlinden This is true. If there is an issue for that why not just program it to return one of the values by default? ie

  // Enum has 3 values
  b (v: Enum) {
    switch (v) {
        case Enum.Value3
             return something();
        case Enum.Value2
            return something2();
       default:
            return something3();
    }
  }

The code is strongly typed already so you know it should be one of those values (unless someone did something nefarious like cast to any and back.

Otherwise implement the default as throwing an error cause it's a value that it shouldn't be which is best practices anyway.

[bedag-moo]

1 In swagger, enums are an anonymous collection of possible values. Typescript union types are an anonymous collection of possible values.

This may be true, but when creating these clients we need to think of how people will (and want) to consume them.

Certainly, and I would not violate expectations lightly, but how else can we make different uses of the same enum assignment compatible? I think assignment compatibility is a justified expectation as well, and since it is related to correctness and type safety, it seems more important than a syntactic difference.

4 union types can be used as index types, but enum types can not

This works perfectly for me with an enum. Not sure why it isn't working for you.

Oops, I had tried with {[K: MyEnum]: string} rather than {[K in MyEnum]: string}. The latter indeed works flawlessly, though it is slightly surprising that its maps enum values rather than names. But that's good enough for me :-)

5 union types can be easily used in templates

But to that note you can do exactly the same thing for an enum in the template.

Oh, you're right. It seemed so obvious that since const e: Enum = 'open' was illegal, so would e === 'open', but apparently the compiler is more lax with comparisions and switch statements, so this works even in ahead-of-time compilation :-)

[topce]

@bedag-moo I report problem (breaking compatibility} same day i spotted it
Better late than never
As I explained in previous comments I would prefer to use enums instead of strings or numbers
Use cases that you mention were not relevant for project I am working on
I am more on same page as @kenisteward
but I see your and @bobvanderlinden have different opinion and I respect that