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

[BUG] [Typescript] Impossible to use enum options #14569

Closed
6 tasks done
ksvirkou-hubspot opened this issue Jan 31, 2023 · 11 comments
Closed
6 tasks done

[BUG] [Typescript] Impossible to use enum options #14569

ksvirkou-hubspot opened this issue Jan 31, 2023 · 11 comments

Comments

@ksvirkou-hubspot
Copy link
Contributor

ksvirkou-hubspot commented Jan 31, 2023

Bug Report Checklist

  • Have you provided a full/minimal spec to reproduce the issue?
  • Have you validated the input using an OpenAPI validator (example)?
  • Have you tested with the latest master to confirm the issue still exists?
  • Have you searched for related issues/PRs?
  • What's the actual output vs expected output?
  • [Optional] Sponsorship to speed up the bug fix or feature request (example)
Description
export declare type FilterOperatorEnum = "EQ" | "NEQ" | "LT" | "GT";

I can't use any option of the enum

let operation = FilterOperatorEnum.EQ
openapi-generator version

6.2.1

OpenAPI declaration file content or url
"Filter": {
    "type": "object",
    "properties": {
        "operator": {
            "type": "string",
            "enum": [
               "EQ",
               "NEQ",
               "LT",
               "GT",
            ]
        }
    }
}
Generation Details

generate -i *.json -g typescript -o /dir --skip-validate-spec --additional-properties usePromises=true,supportsES6=true,platform=node

Related issues/PRs

#13509

Suggest a fix
@bodograumann
Copy link
Contributor

bodograumann commented Sep 7, 2023

Can we please label this issue as breaking change without fallback in v7, @wing328 ?
Strings and enums are completely different things in typescript. (Cf. https://github.com/OpenAPITools/openapi-generator/pull/14663/files#diff-669466c5a2b36b6286325ddca08226df4ed6f90dd04f101ff840bae691a984d1L23)

I don't really see the reason for this change though. If you want to use an option, you can just use the string, e.g. "EQ". Typescript will correctly make sure that only allowed options are used and assigning dynamic strings to such literal string unions is also prevented.
Also literal string unions are more ideomatic in typescript's structural typing.

One advantage of enums is the possibility of runtime checking.

When enums with the same values are defined in multiple APIs, string literals can be used in both places, while the material enums cannot. Whether that is a good thing or not depends on the specific use case.
E.g. in our situation we have shared domain types which are imported in mutliple API definitions, but for code generation purposes the imported types will be duplicated.

We will override the template here, but maybe this should be togglable with a generator argument?

@RichardBradley
Copy link

This seems to be a step backwards. As @bodograumann commented, string union properties are much more idiomatic and nicer to work with in Typescript than string enums.

As you can see in this section of the diff from the PR: https://github.com/OpenAPITools/openapi-generator/pull/14663/files#diff-093571dbeb012904d7da944caca6939594eae68044e29bb75b0584146fd02cda

        pet.status = 'available'

now needs to be written as:

        pet.status = petstore.PetStatusEnum.Available

which makes client code much harder to write and work with.

This should definitely be an option, and probably off by default :-(

@RichardBradley
Copy link

I came here looking to maybe submit a PR to change enum handling from string enums to string unions in the "typescript-rxjs" generator, because it would make our client much nicer and perhaps many others. I guess I won't attempt that, if you're going in the other direction on this generator.

Why are there 11 different "typescript*" generators, and how am I meant to pick between them? They appear to handle union types differently and handle enums differently.

@bodograumann
Copy link
Contributor

The "typescript" generator without any suffix is a rewrite and was supposed to replace all the others, but it seems there is not much pressure to actually do that now (@TiFu ?).
So I would suggest using that one for any new projects.

Regarding the enums I didn't get any feedback on my comment above from the maintainers, but it shouldn't be too hard to introduce a attribute to toggle between literal strings and enum objects.

@RichardBradley
Copy link

Thanks for your reply

The "typescript" generator without any suffix is a rewrite and was supposed to replace all the others, but it seems there is not much pressure to actually do that now (@TiFu ?).

An abandoned attempt at https://xkcd.com/927/ ?
Classic.

I can't see any documentation about what each of the 11 is for or why I might choose one over the other.
All 11 are listed here, but there's no summary of them, nor do each give an intro about what they are for and how they differ from the other 10.

If the "typescript (experimental)" generator is intended as a replacement for all the others, I might have expected it to have a summary of that goal in its docs, along with some discussion of the problems it intends to solve and why the other 10 are unsuitable.

As far as I can tell, the primary difference seems intended to be which HTTP client library is bundled by default.

However, they do also differ significantly in how they handle "AllOf" combined classes and enums, so I find this situation very frustrating.

@bodograumann
Copy link
Contributor

Yeah that reference is kinda apt.
I think the goal was to only have to support a single template and get rid of all the code duplication.
Unfortunately once the generator works well enough, there is little motivation to contribute anything in this direction or improve the documentation...

If you like I can work with you to bring the literal string enums back. Alternatively I can give you the template override that we are using to get it locally.

@TiFu
Copy link
Contributor

TiFu commented Mar 13, 2024

@bodograumann is correct. I had started this a few years back - with the goal of replacing all the code duplication. There even is an issue that documents this: #802 including motivation and the rough architectural plan.

With personal stuff taking over more and more of my time (such as a full-time job ;) truly missing the life of a college student haha), progress on this also stopped. Of course it's open source so if anyone feels like adding to this, they can always pick up where I have left off.

@ksvirkou-hubspot
Copy link
Contributor Author

@bodograumann
I am sorry for silence.
Can I add an option (a generator argument) for enum's type?
example --enumType=stringUnions( or literalStrings) --enumType=enum (enumObjects)
Default will be stringUnions.

@bodograumann
Copy link
Contributor

That would be greatly appreciated, @ksvirkou-hubspot !
I like the proposed argument scheme and would go for stringUnion vs enum as options.
Please let me know if you need any help.

@ksvirkou-hubspot
Copy link
Contributor Author

#18531
Could you check it, please?

@bodograumann
Copy link
Contributor

Still waiting on the next release. But I think it should be fine :-)
Thanks for adding this back, @ksvirkou-hubspot

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

4 participants