[typescript] Unify `stringEnums` across all typescript generators

[simon-abbott]

Currently a few of the typescript generators have an option for choosing if enums are generated as first-class enums, or as unions of strings. This PR expands that option to be standard across all the typescript family of generators.

Note: this is a breaking change with a fallback (add the stringEnums option).

Fixes #12718

PR checklist

• [x] Read the contribution guidelines.
• [x] Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• [x] Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh ./bin/configs/*.yaml
  ./bin/utils/export_docs_generators.sh
  

  (For Windows users, please run the script in Git BASH) Commit all changed files. This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master. These must match the expectations made by your contribution. You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*. IMPORTANT: Do NOT purge/delete any folders/files (e.g. tests) when regenerating the samples as manually written tests may be removed.
• [x] File the PR against the correct branch: master (upcoming 7.6.0 minor release - breaking changes with fallbacks), 8.0.x (breaking changes without fallbacks)
• [x] If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

TypeScript committee members: @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) @davidgamero (2022/03) @mkusaka (2022/04)

[simon-abbott]

can you change the default to be what it is now so there is no breaking change?

Not without having to re-duplicate the stringEnums option. Currently typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs have a stringEnums option which defaults to false (so it uses a string union + POJO map). The rest of the typescript- generators use string enums by default (effectively stringEnums = true). So if I set the global default to true it will break typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs, but if it have it false it will break the rest of them. We could solve this issue by having typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs override stringEnums to have a different default, but then it also gets messy with it being a boolean flag.

If you have suggestions on how to do this in a non-breaking way while taking all of the above into account I'm all ears, but I'm struggling to find a satisfactory solution.

[macjohnny]

i guess that the typescript-angular and typescript-fetch ones are the most popular variants judging from PR frequency, so I would make stringEnums=true the default to keep the non-breaking change behavior for those.

[simon-abbott]

i guess that the typescript-angular and typescript-fetch ones are the most popular variants judging from PR frequency, so I would make stringEnums=true the default to keep the non-breaking change behavior for those.

I think you meant stringEnums=false? Changing it to true would be a breaking change for typescript-angular and typescript-fetch.

[ksvirkou-hubspot]

Hi everyone If you merge this PR , it will be necessary revert Enum Types PR to 8.*

[macjohnny]

@simon-abbott yes sorry i meant stringEnums=false. can you also make sure to consolidate with https://github.com/OpenAPITools/openapi-generator/pull/18531? what do you propose as the way forward?

[simon-abbott]

What do you mean by "the way forward"?

EDIT: RE #18531: I'm happy to implement whatever the community agrees is best, I just want all the typescript-* family of generators to have this feature. I'm not sold on any particular naming or default other than trying to minimize breaking changes. Just as long as there is a way to choose between enums and string unions I'm happy.

[macjohnny]

@simon-abbott so my question was how you suggest to consolidate with https://github.com/OpenAPITools/openapi-generator/pull/18531.

[simon-abbott]

Ah of course. That makes sense. I can see three options (feel free to add more if you see any, and @ksvirkou-hubspot feel free to weigh in as well!):

1. Clobber #18531 entirely and use a boolean stringEnums option.
   • Pros: full unity (all generators have the same options) while also keeping best compatibility with what we have now (so fewer breaking changes than option 2).
   • Cons: will render #18531 entirely redundant, and will be a breaking change for all typescript-* generators except typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs.
2. Embrace #18531 and replace all usage of stringEnums with enumType.
   • Pros: better API design (imo) – having enum options makes it clear what the choices are without needing to spell it out as much, and thus makes it easier to maintain config files without needing to check the docs.
   • Cons: will be a fully breaking change for every use of typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs that currently specifies a value for stringEnums.
3. Go back to each generator defining this property on its own and have typescript be an outlier.
   • Pros: zero breaking changes (can have each generator set a different default, which fixes the concerns from this comment).
   • Cons: causes fragmentation; different generators have different options with different defaults.

I personally am leaning towards option 1 targeting 7.7.0 as it is the best blend of backwards compatibility and unification. That being said, I do also prefer the semantics and DX (developer experience) of option 2.

One other option would be to do a hybrid approach:

1. Dedupe this PR to just add stringEnums to all the other typescript-* generators without hoisting, and set defaults to cause no backwards compat issues
2. Put up a new PR targeting 8.x that hoists the value, changes it to be enumType, and sets a single default (or some subset thereof).

Thoughts?

[macjohnny]

@simon-abbott thanks for investigating!

Dedupe this PR to just add stringEnums to all the other typescript-* generators without hoisting, and set defaults to cause no backwards compat issues

can this be done in a way that is compatible with https://github.com/OpenAPITools/openapi-generator/pull/18531?

Put up a new PR targeting 8.x that hoists the value, changes it to be enumType, and sets a single default (or some subset thereof).

if there is no urgency, probably this one would be nicest?

@ksvirkou-hubspot what are your thoughts?

[simon-abbott]

can this be done in a way that is compatible with #18531?

Yeah. What I'd do to maximize compatibility going forwards is:

1. Backport #18531 to 7.7.0
2. Add a new enumType option (instead of stringEnums) to all typescript-* generators that don't currently have enumType
3. Leave all existing stringEnums flags alone
   • I might also take a crack at adding enumType alongside the existing stringEnums flag. That way we could smoothly deprecate stringEnums rather than it being a single hard changeover. If that's too much work I won't do it though.

That would leave us with a mix of stringEnums (defaulting to false) for typescript-angular, typescript-axios, typescript-fetch, and typescript-nestjs, and enumType (defaulting to enum) for all the other typescript-* generators. This also means this change would not be breaking, which is a plus!

Then after that is merged we can make a new PR targeting 8.x that hoists enumType to AbstractTypeScriptClientCodegen.java and sets a single default (I'm leaning towards stringUnion being the default for compatibility and because TS enums are not great, but that should be discussed in the future PR).

If there are no complaints against this approach (and the rest of my workload allows) I'll try to get started on these changes next week. @macjohnny should I update this PR with the changes? Or close it and make a new one? The diff will mostly be a single variable name change, but it's still technically a different goal.

[macjohnny]

@simon-abbott sounds great, please go ahead! you can just update this PR and let me know when it is ready to review.

[ksvirkou-hubspot]

@simon-abbott thanks for investigating!

Dedupe this PR to just add stringEnums to all the other typescript-* generators without hoisting, and set defaults to cause no backwards compat issues

can this be done in a way that is compatible with #18531?

Put up a new PR targeting 8.x that hoists the value, changes it to be enumType, and sets a single default (or some subset thereof).

if there is no urgency, probably this one would be nicest?

@ksvirkou-hubspot what are your thoughts?

Sounds good

[simon-abbott]

If the rest of my workload allows

It did not. This is still on my to-do list though, but if someone else wants to take over before I get a chance to get back to it feel free.