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

Allow sorting in enums to be disabled #6935

Closed
andbjer opened this issue Oct 29, 2021 · 21 comments
Closed

Allow sorting in enums to be disabled #6935

andbjer opened this issue Oct 29, 2021 · 21 comments

Comments

@andbjer
Copy link

andbjer commented Oct 29, 2021

Describe the bug

My schema contains an enum for DayOfWeek:

enum DayOfWeek {
  SUNDAY
  MONDAY
  TUESDAY
  WEDNESDAY
  THURSDAY
  FRIDAY
  SATURDAY
}

When GraphQL Code Generator creates a typescript type for this enum, it will sort the values alphabetically like below:

export enum DayOfWeek {
  Friday = 'FRIDAY',
  Monday = 'MONDAY',
  Saturday = 'SATURDAY',
  Sunday = 'SUNDAY',
  Thursday = 'THURSDAY',
  Tuesday = 'TUESDAY',
  Wednesday = 'WEDNESDAY'
}

This is an issue if you would like to retrieve the index value of the enum:

const day = Object.values(DayOfWeek).indexOf(obj.dayOfWeek);

If the object's day of week is Sunday I would expect day to be 0, instead it is 3.

I've tried to look through all of the options for the typescript-plugin, but I have not found an option for disabling sorting on enums.

To Reproduce
Steps to reproduce the behavior:

https://codesandbox.io/s/graphql-code-generator-enum-issue-7788u

  1. My GraphQL schema:
type Query {
    user(id: ID!): User!
}

type User {
    id: ID!
    username: String!
    email: String!
    planStartDay: DayOfWeek
}

enum DayOfWeek {
  SUNDAY
  MONDAY
  TUESDAY
  WEDNESDAY
  THURSDAY
  FRIDAY
  SATURDAY
}
  1. My GraphQL operations:
query user {
    user(id: 1) {
        id
        username
        email
        planStartDay
    }
}
  1. My codegen.yml config file:
schema: schema.graphql
documents: document.graphql
generates:
  types.ts:
    plugins:
      - typescript
      - typescript-operations

Expected behavior
I would expect the enum values to keep the same index position as in the schema, or at least an option to disable the alphabetical sorting.

Environment:

  • OS: macOS 11.6
  • "@graphql-codegen/add": "3.1.0"
  • "@graphql-codegen/cli": "2.2.1"
  • "@graphql-codegen/typescript": "2.2.4"
  • "@graphql-codegen/typescript-operations": "2.1.8",
  • NodeJS: v17.0.1

Additional context

@ardatan
Copy link
Collaborator

ardatan commented Oct 30, 2021

We apply sorting not only enum keys but also fields, o ject definitions etc. Currently there is no option to disable sorting for enum values specifically but instead you can disable all kind of sorting with sort: falseconfiguration option.

@andbjer
Copy link
Author

andbjer commented Nov 1, 2021

@ardatan thank you! Is that behaviour documented anywhere? I tried specifying generates.config.sort: false and generates.sort: false but that did not work for me. Only thing that worked was specifying sort: false on the root level in codegen.yml but that isn't listed as an option here.

I still think it would be nice to have an option to disable sorting for enums. It's easier to browse through the code when it's sorted alphabetically, but it would be great if there was an option that for maintaining the index positioning of the enum values.

@n1ru4l
Copy link
Collaborator

n1ru4l commented Nov 5, 2021

@andbjer One could argue whether enum values are actually meant to be used like that. The ECMA specification does not clearly state that each js engine has to implement Object.values (aka enumeration order of objects) the same way (https://stackoverflow.com/a/52706191).

From a client perspective, an alternative approach could be to have a field on your Query root type that exposes a list of DayOfWeek that is in the correct order and that the client can query to get the guaranteed order.

type Query {
  daysOfWeek: [DayOfWeek!]!
}

I really think this is simply too uncertain to support it as a separate config option.

@smmoosavi
Copy link

I saw this doc and my expectation is sort: false should work like this:

schema: http://localhost:3000/graphql
generates:
  schema.graphql:
    plugins:
      - schema-ast
    config:
      commentDescriptions: true
      sort: false

also, it says default is false

sort
type: boolean default: false

@smmoosavi
Copy link

I checked this code, sort config is not used in the transformSchemaAST function, which is weird

@ardatan
Copy link
Collaborator

ardatan commented Nov 8, 2021

@smmoosavi Thank you for the report! We have updated the documentation now. sort flag is used in graphql-tools.

@smmoosavi
Copy link

@ardatan
Can use send the new documentation link?

@tvvignesh
Copy link
Contributor

@ardatan Can use send the new documentation link?

https://www.graphql-code-generator.com/plugins/schema-ast

image

Default updated to true. You can manually set it to false to disable sorting

@andbjer
Copy link
Author

andbjer commented Dec 13, 2021

@n1ru4l I was not aware that this could differ from browser to browser. Then I guess the solution you propose is probably the best option.

@ha-akelius
Copy link
Contributor

@ardatan Can use send the new documentation link?

https://www.graphql-code-generator.com/plugins/schema-ast

image

Default updated to true. You can manually set it to false to disable sorting

it didn't work for me :(

could you please provide a complete codegen.yml

this is my codegen.yaml, I don't know what I am missing

schema: "schema.graphql"
generates:
  ./src/app/graphql/models/types.ts:
    documents:
      - "./src/app/graphql/**/*.ts"
    plugins:
      - "schema-ast"
      - add:
          content: "/* eslint:disable */\n/* tslint:disable */"
      - "typescript"
      - "typescript-operations"
    config:
      sort: false
      skipTypename: true
      namingConvention:
        enumValues: upper-case#upperCase

@gtschieder
Copy link

gtschieder commented Apr 6, 2022

@ha-akelius, try specifying on the sort:false on the root level, like so:

schema: "schema.graphql"
config:
      sort: false
generates:
(...)

That worked for me, whereas specifying at the output level did not.

@deweve
Copy link

deweve commented Aug 5, 2022

Hello, we will rolleback to older version to not have the new sort method.
We've tried to disabled the sort and it work however, changing one query (ex: add one field to a query) is modifying the all structure of the file.

The Query A is now at ligne 2000 instead of line 100, even if it was not modified. It becomes very difficult to work like that in a team because it produce merge conflict every time the graphql.tsx is generated.

I think we need the option to disable sort on enum. I understand that browsers does not return values in the same order, but we did not had the problem.

So either we have sortEnum option or the logic in our project has to be changed. For the moment will rolleback on the version of the lib.

PS : I know this is an open source project. If you indicates me where the logic is implemented we could considerate to developp this option.

@coopbri
Copy link

coopbri commented Jan 19, 2023

@ha-akelius, try specifying on the sort:false on the root level, like so:

schema: "schema.graphql"
config:
      sort: false
generates:
(...)

That worked for me, whereas specifying at the output level did not.

Is this supposed to still work/work for code-based configuration? In my GraphQL codegen config file I have:

const codegenConfig = {
  ...
  config: {
    sort: false
  }
  ...
}

which has no effect on the enum sorting.

@ha-akelius
Copy link
Contributor

@coopbri
it is working for me, part of my file:

import { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: 'schema.graphql',
  config: {
    sort: false,
  },
...
}

@coopbri
Copy link

coopbri commented Jan 19, 2023

@coopbri
it is working for me, part of my file:

import { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: 'schema.graphql',
  config: {
    sort: false,
  },
...
}

Thanks for confirming @ha-akelius! I have the same config so I must have another conflict. That helped me confirm something else is causing sorting to stay enabled for me, I'll do some more testing today.

@ekfn
Copy link
Contributor

ekfn commented Feb 9, 2023

One could argue whether enum values are actually meant to be used like that. The ECMA specification does not clearly state that each js engine has to implement Object.values (aka enumeration order of objects) the same way (https://stackoverflow.com/a/52706191).

I believe ECMA specifies the order https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/for...in#description

The traversal order, as of modern ECMAScript specification, is well-defined and consistent across implementations. Within each component of the prototype chain, all non-negative integer keys (those that can be array indices) will be traversed first in ascending order by value, then other string keys in ascending chronological order of property creation.

@falkenhawk
Copy link

falkenhawk commented Mar 17, 2023

Unfortunately, it is probably out of control of graphql-code-generator, as it is calling lexicographicSortSchema from graphql through @graphql-tools/load - there

and enum values are being sorted there (which is clearly wrong, enum values should not be reordered as it conveys change of their internal integer values):
https://github.com/graphql/graphql-js/blob/main/src/utilities/lexicographicSortSchema.ts#L144-L150

At the beginning of that sortNamedType function, it can be seen that sorting is being disabled for "scalar" and "introspection" types, probably for a reason. So imo, "enum" type should be also added to that condition. But such change might be difficult to push through graphql-js defence lines. Anyone willing to submit a PR there? 😅 (see: reasoning in graphql/graphql-js#1199 (comment) 🙈)

Meanwhile, I used patch-package to patch graphql for my needs. This fixes the issue:

@@ -0,0 +1,12 @@
diff --git a/node_modules/graphql/utilities/lexicographicSortSchema.js b/node_modules/graphql/utilities/lexicographicSortSchema.js
index ea884b6..49ab981 100644
--- a/node_modules/graphql/utilities/lexicographicSortSchema.js
+++ b/node_modules/graphql/utilities/lexicographicSortSchema.js
@@ -97,6 +97,7 @@ function lexicographicSortSchema(schema) {
   function sortNamedType(type) {
     if (
       (0, _definition.isScalarType)(type) ||
+      (0, _definition.isEnumType)(type) ||
       (0, _introspection.isIntrospectionType)(type)
     ) {
       return type;

@mmuejde
Copy link

mmuejde commented May 31, 2023

@coopbri
it is working for me, part of my file:

import { CodegenConfig } from '@graphql-codegen/cli';

const config: CodegenConfig = {
  schema: 'schema.graphql',
  config: {
    sort: false,
  },
...
}

Thanks for confirming @ha-akelius! I have the same config so I must have another conflict. That helped me confirm something else is causing sorting to stay enabled for me, I'll do some more testing today.

Its also worked for me!!!

@hugomn
Copy link

hugomn commented Jun 18, 2023

Unfortunately, it is probably out of control of graphql-code-generator, as it is calling lexicographicSortSchema from graphql through @graphql-tools/load - there

and enum values are being sorted there (which is clearly wrong, enum values should not be reordered as it conveys change of their internal integer values): https://github.com/graphql/graphql-js/blob/main/src/utilities/lexicographicSortSchema.ts#L144-L150

At the beginning of that sortNamedType function, it can be seen that sorting is being disabled for "scalar" and "introspection" types, probably for a reason. So imo, "enum" type should be also added to that condition. But such change might be difficult to push through graphql-js defence lines. Anyone willing to submit a PR there? 😅 (see: reasoning in graphql/graphql-js#1199 (comment) 🙈)

Meanwhile, I used patch-package to patch graphql for my needs. This fixes the issue:

@@ -0,0 +1,12 @@
diff --git a/node_modules/graphql/utilities/lexicographicSortSchema.js b/node_modules/graphql/utilities/lexicographicSortSchema.js
index ea884b6..49ab981 100644
--- a/node_modules/graphql/utilities/lexicographicSortSchema.js
+++ b/node_modules/graphql/utilities/lexicographicSortSchema.js
@@ -97,6 +97,7 @@ function lexicographicSortSchema(schema) {
   function sortNamedType(type) {
     if (
       (0, _definition.isScalarType)(type) ||
+      (0, _definition.isEnumType)(type) ||
       (0, _introspection.isIntrospectionType)(type)
     ) {
       return type;

None of the options worked for me, and considering this reply here, should the issue be reopened?

@kbrooks
Copy link

kbrooks commented Sep 15, 2023

I ran into this same issue, and I'm having a problem where disabling the sorting of the whole generated schema causes the generated types to order themselves randomly, in a way that can change between each run of codegen. We commit our generated code to our repo and have a CI step that fails if codegen changes anything, which is broken on newer versions due to this randomness.
This is caused by #4919, which was solved by adding the lexicographic ordering being complained about here.
Can we add a feature to disable sorting only on generated enums?

@kbrooks
Copy link

kbrooks commented Sep 16, 2023

It's possible to disable the sorting with a patch on the graphql package, which has solved by issue, but it would be nice for the option to be fully supported.
[email protected]

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

No branches or pull requests