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.

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

keyOf enums + Enum classes #80

Closed
michahell opened this issue Apr 26, 2017 · 6 comments
Closed

keyOf enums + Enum classes #80

michahell opened this issue Apr 26, 2017 · 6 comments

Comments

@michahell
Copy link

michahell commented Apr 26, 2017

We use a custom made json-schem to TS script which creates TitleCasedEnums classes with public static string = 'enumValue' fields. This lacks actual Enum behaviour but does provide for easy enum accessibility. We were looking into enhancing this with the TS 2.1 keyof property.

Right now, we have something like this:

export interface ExampleSchema {
  firstName: string;
  lastName: string;
  /**
   * Age in years
   */
  age?: number;
  hairColor?: string;
}

export class HairColorEnum {
  public static BLACK: string = 'black';
  public static BROWN: string = 'brown';
  public static BLUE: string = 'blue';
}

This would already be a lot better:

export interface ExampleSchema {
  firstName: string;
  lastName: string;
  /**
   * Age in years
   */
  age?: number;
  hairColor?: (HairColorEnum.BLACK | HairColorEnum.BROWN | HairColorEnum.BLUE);
}

export class HairColorEnum {
  public static BLACK: string = 'black';
  public static BROWN: string = 'brown';
  public static BLUE: string = 'blue';
}

But this would be best:

export interface ExampleSchema {
  firstName: string;
  lastName: string;
  /**
   * Age in years
   */
  age?: number;
  hairColor?: keyof HairColorEnum;
}

export class HairColorEnum {
  public static BLACK: string = 'black';
  public static BROWN: string = 'brown';
  public static BLUE: string = 'blue';
}
@bcherny
Copy link
Owner

bcherny commented Apr 26, 2017

Hi @michahell!

Can you help explain the use case for modelling enums as classes? Today jstt supports 2 types of enums:

  1. Numerical enums compile to typescript enums
  2. String enums compile to typescript string literal union types

What is the advantage of your approach over (2)? What do you think the corresponding JSONSchema should look like?

@michahell
Copy link
Author

hello @bcherny !

The only advantage of my last proposed approach over (2) is that one can specify any of the enums by using static constants, instead of using (what I call) 'magic strings'.

I think that ^ is an improvement because:

  1. updating 'magic strings' in your whole codebase is error-prone, whereas using a static const makes this easier and less error prone.
  2. IFF you have an editor or IDE that provides code completion, you will be able to quickly get an overview of allowed (string) values because of the defined constants. I'm not sure if this also is the case when just using string union types, if it is, then this point is moot :)

@bcherny
Copy link
Owner

bcherny commented Apr 28, 2017

Those are both good points! I assume there should be a reverse mapping too (ie. 'black' -> 'BLACK')?

@michahell
Copy link
Author

Hmm... Haven't thought about reverse mapping, that could prove useful as well, but not sure how to do that off the top of my head

@bcherny
Copy link
Owner

bcherny commented May 2, 2017

@michahell A few notes, looking into this more:

  1. It is a bummer that VSCode and other IDEs do not autocomplete union types. This is an enhancement tracked in String literal union types are validated, but values are not suggested by IDEs microsoft/TypeScript#12891 - feel free to give it a thumbs up.
  2. Generating classes (as you proposed) has some unintended consequences, and is too permissive:

screen shot 2017-05-02 at 10 11 44 am

3. A third solution is simulating string enums using namespaces:

screen shot 2017-05-02 at 10 19 16 am

Finally, it looks like proper string enums will be available in TS2.4 - microsoft/TypeScript#15486.

Both the literal union (1) and class-based (2) solutions are lacking, and the namespace (3) solution seems like a hack. I'm going to close this issue in anticipation of TS2.4. Feel free to reopen if you have strong objections.

@bcherny bcherny closed this as completed May 2, 2017
@michahell
Copy link
Author

Agreed @bcherny! Hadn't looked at the TypeScript roadmap before properly, thanks for investigating!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants