-
Notifications
You must be signed in to change notification settings - Fork 470
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
Emit TypeScript string unions instead of enums by default #1750
base: master
Are you sure you want to change the base?
Conversation
@grantila: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Apollo Contributor License Agreement here: https://contribute.apollographql.com/ |
I think this is great. For me it makes sense that this is the default behaviour but pub potentially better to make string unions opt in instead to not introduce a breaking change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a TypeScript language perspective, this is a big improvement. 👍
+1 for this. It can be an option for now and be made the default on next breaking change? |
What is the status of this? Can it get merged? |
I would love to have string unions instead of enums. Hope this gets merged soon 🤞 |
@RyanCavanaugh hi |
would love to have this merged, the main reason is I have different Enums with the same values and would love o avoid "casting" them |
Any movement on this? it would be huge for our use case, since we have server + client side sharing of code, we are getting collisions like: enum ServerEnum {
VALUE = "VALUE",
}
enum GqlGenerated {
VALUE = "VALUE"
}
query.valueField as GqlGenerated === ServerEnum.VALUE // type error since enums dont overlap |
Any reason this has not been merged? |
It's been a while so let me take a minute to explain why the current behavior is really suboptimal from a developer experience perspective. Let's say you write two different queries that both pull GitHub's if (prFromQuery1.state === prFromQuery2.state) { ... because an inherent behavior of string enums is that they're designed to prevent these sorts of comparisons from occurring. This isn't desirable at all; it should be legal to compare two values from two different queries if they have some overlap. TypeScript will still detect mistakes here - if you have two queries that produce unions that are fully disjoint, you'll get an error. String literal unions are just a much more accurate representation of what's going on here -- the underlying string values in GraphQL schemas are intended to be nonopaque, but string enums assume a certain level of opaqueness that defeats common patterns you would want to validly write in GraphQL code. |
Would anyone mind if I take this over and get it merged in? The restriction to enums has made things difficult for my team when dealing with unit test and type checking our mocked data. We have naturally moved towards just not typing mocks at all, which has become a pain. Some teams don't use typescript for type checking. Typescript can be a useful forced code documentation tool and track the flow of data throughout the application. My team has this mindset. For us, we just need to know that the field is equal to any given string. The enum provides no value and instead has hindered us as a whole. Having the option to use type aliases instead of enums works better for our development workflow. |
We are in the same boat and want to use string unions as well instead of enums. However, why change the default behavior? This might break certain workflows of people who rely on enums as default behavior. Couldn't this just be an optional flag, similar to |
It would be great to have an option to generate either enums of union types. I personally prefer union types in the majority of the cases and would like to see this happening. What is stopping from moving this forward? |
What is the status of this? Can it get merged? |
In the mean time, I started rethinking type conversions completely and built (relatively newly released) typeconv which converts between TypeScript, GraphQL, JSON Schema and Open API (in any order). Soon it'll also be able to also convert to/from suretype (released two days ago). typeconv produces string unions, not by default but always. This is because enums in GraphQL and enums in TypeScript are two semantically different things, and aren't convertible two-way. Sure you technically can convert GraphQL enums to TypeScript enums, but not always the other way around, because; what would |
I would like to see this merged as well. To proceed with this someone with write access on this PR propably needs to resolve the merge conflicts and then check if that one CircleCI test still fails. |
Hi! |
As someone pretty new to TypeScript I didn't know about this, but you can convert an string enum to a string union like so: type WeekdayType = `${Weekday}`; Note that this only works in TS version at least TS4.1 Posted just in case that helps out anyone else here. All that said, I would love to see this merged or added as a CLI option to choose enums vs unions. What's blocking this? |
I describe a situation where this PR would be really helpful in microsoft/TypeScript#39127 (comment) . What's the status of this PR? |
@RyanCavanaugh @gabrielferreiraa Both of you approved this PR and no one commented anything against it, please resolve the conflicts and merge. |
@sharshuv-quotient The people who approved the PR are not maintainers of the repo and do not have permission to merge. |
For the people still following this PR: I commented last year that I would like to see this getting merged, but I do no longer think that this will happen. The readme states that this whole project is going to be deprecated.
Instead we migrated to GraphQL Code Generator in our project some months ago. If it helps, someone else described some GraphQL Code Generation options that should produce output similar to |
This fixes #1044
*Make sure changelog entries note which project(s) has been affected. See older entries for examples on what this looks like.