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

Fragment enum fields aren't compatible with Type enum fields #2326

Closed
jkillian opened this issue Aug 9, 2019 · 6 comments
Closed

Fragment enum fields aren't compatible with Type enum fields #2326

jkillian opened this issue Aug 9, 2019 · 6 comments
Labels
plugins waiting-for-release Fixed/resolved, and waiting for the next stable release

Comments

@jkillian
Copy link

jkillian commented Aug 9, 2019

Describe the bug
In the generated TypeScript types for a GraphQL schema, enum fields are typed as field: EnumType. However, in fragments, they're typed as a union of string literals: field: "VAL1" | "VAL2"; This means that the two types are not assignable to each other which can cause a variety of challenges in application code.

The issue is illustrated in this screenshot:
image

Note: I mentioned the same issue here, but decided it would be cleaner to just open a new issue than tack on to a long pre-existing one. Sorry for the noise!

To Reproduce
Steps to reproduce the behavior:

  1. My GraphQL schema:
schema {
  query: Query
}

type Query {
  me: User!
}

enum Role {
  USER,
  ADMIN,
}

type User {
  role: Role!
}
  1. My GraphQL operations:
fragment RoleFragment on User {
  role
}
  1. My codegen.yml config file:
generates:
  graphqlTypes.ts:
    config:
      avoidOptionals: true
      dedupeOperationSuffix: true
      namingConvention:
        enumValues: change-case#constantCase
      noSchemaStitching: true
      operationResultSuffix: "Result"
      maybeValue: "T | undefined | null"
      preResolveTypes: true
    plugins:
      - "typescript"
      - "typescript-operations"
  1. Generated code (slightly trimmed for conciseness):
export type Query = {
  __typename?: "Query";
  me: User;
};

export enum Role {
  USER = "USER",
  ADMIN = "ADMIN"
}

export type User = {
  __typename?: "User";
  role: Role;
};
export type RoleFragment = { __typename?: "User"; role: "USER" | "ADMIN" };

Expected behavior

The generated fragment should use the enum type instead of being a union of literals:

- export type RoleFragment = { __typename?: "User"; role: "USER" | "ADMIN" };
+ export type RoleFragment = { __typename?: "User"; role: Role };

Environment:

  • OS: macOS Mojave
  • @graphql-codegen/...: 1.5.0
  • NodeJS: v10.11.0
@dotansimha
Copy link
Owner

Hi @jkillian !
The idea behind preResolveTypes is to use primitive types where possible. We recently changed enums to be a union of string in order to avoid using the generated enum.
Can you try to set enumsAsTypes: true? I think it should solve that.

@dotansimha dotansimha added the waiting-for-answer Waiting for answer from author label Aug 11, 2019
@jkillian
Copy link
Author

Hi @dotansimha, thanks for the response!

Can you try to set enumsAsTypes: true? I think it should solve that.

Yes, I think this would work, but we have many places in our codebase where we reference the enums, using the example from above, things like, Role.ADMIN, so refactoring would be a bit unfortunate.

The idea behind preResolveTypes is to use primitive types where possible.

I'd actually argue that using an enum is basically using a primitive type. I think the output in these cases should depend only on on enumAsTypes and not on preResolveTypes. If enumAsTypes is false, than use enum everywhere, if it's true, then use the string literals everywhere.

preResolveTypes is a great feature! Makes the generated code much easier to read. It just seems like things are working inconsistently right now when enumsAsTypes is false.

Let me know if all this makes sense!

@jkillian
Copy link
Author

There's actually a third option, which would be to export both a type and an enum-like const. This would let literals be used in types, but also let people programmatically reference values via Role.ADMIN. The generated code would look something like this:

export const Role = {
  USER: 'USER',
  ADMIN: 'ADMIN'
} as const;
export type Role = keyof typeof Role;

I actually think that's kind of a nice option! It gives you the best of both worlds in TypeScript in my opinion.

@dotansimha
Copy link
Owner

@jkillian fixed in: #2385

@dotansimha dotansimha added the waiting-for-release Fixed/resolved, and waiting for the next stable release label Aug 16, 2019
@dotansimha
Copy link
Owner

Fixed in 1.6.0

@jkillian
Copy link
Author

Amazing! I look forward to trying it out in a few days. Thank you again for all your hard work on the project @dotansimha!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
plugins waiting-for-release Fixed/resolved, and waiting for the next stable release
Projects
None yet
Development

No branches or pull requests

2 participants