dimaMachina / graphql-eslint

ESLint parser, plugin and set rules for GraphQL (for schema and operations). Easily customizable with custom rules. Integrates with IDEs and modern GraphQL tools.
https://the-guild.dev/graphql/eslint
MIT License
798 stars 104 forks source link

let `require-id-when-available` validate when fragment is present #89

Closed bennypowers closed 3 years ago

bennypowers commented 4 years ago

consider

fragment a on A {
  id
  aField
}
# import "./a.fragment.graphql"
query AQuery {
  as { ...a }
}

Field "id" must be selected when it's available on a type. Please make sure to include it in your selection set!eslint@graphql-eslint/require-id-when-available

require-id-when-available should allow fields which contain (only?) fragment spreads. Bonus points if the fragment themselves validate as having id

dotansimha commented 4 years ago

@bennypowers this is currently blocked by : https://github.com/dotansimha/graphql-eslint/issues/25 , I'll try to work on that during the weekend :)

bennypowers commented 4 years ago

Thanks! don't work too hard 😄

tvvignesh commented 3 years ago

@bennypowers @dotansimha Was facing the same issue when I have the field id on a fragment. I would assume it is the same error as this. Is it?

1
fragment authMethodFields on AuthMethod {
  id
  authKey
  verificationStatus
}

query queryAuthMethod ($authMethodFilter: AuthMethodFilter!) {
    queryAuthMethod(filter: $authMethodFilter) {
      ...authMethodFields
    }
}

Thanks. I will work without fragments now to overcome this.

dotansimha commented 3 years ago

@tvvignesh this issue is currently blocked because I didn't have the time to implementing the siblings operations support. Since ESLint runs only on a single file every time, we can't really know that's in that fragment, so we don't know if there is id field there. I started a PR with parser performance improvements required for this, hope to have progress on that next week :) This feature should allow us to do so much more with graphql-eslint :)

tvvignesh commented 3 years ago

@dotansimha Thanks for the clarification. Don't boil yourself. I tagged just cause I wanted to know if I had to create a new issue or my error was related to the same issue. Currently, since I am just in the initial development phase, this is not a blocker for me (I can move to fragments later). Thanks.

dotansimha commented 3 years ago

@bennypowers @tvvignesh can you please try @graphql-eslint/eslint-plugin@0.5.0-alpha-c082a44.0? You need to specify where your operations file are, I might try to improve that by reading it from eslintrc file, but not 100% sure yet.

{
  "files": ["*.graphql"],
  "parser": "@graphql-eslint/eslint-plugin",
  "plugins": ["@graphql-eslint"],
  "parserOptions": {
    "operations": ["./src/**/*.graphql"],
    "schema": "./schema.graphql"
  }
}
tvvignesh commented 3 years ago

@dotansimha Hi. Just tried upgrading and adding operations (did not add schema since it is anyways taken from my .graphqlrc file same as before) and I got this error in every GraphQL file I opened

e1

This is how I tried specifying the options:

"overrides": [
    {
      "files": ["*.gql"],
      "parser": "@graphql-eslint/eslint-plugin",
      "plugins": ["@graphql-eslint"],
      "rules": {
        "@graphql-eslint/avoid-operation-name-prefix": [
          "error",
          { "keywords": ["get"] }
        ],
        "@graphql-eslint/input-name": [
          "error",
          { "checkInputType": true }
          ],
          "@graphql-eslint/description-style": [
          "error",
          { "style": "block" }
          ],
          "@graphql-eslint/naming-convention": [
          "error",
          {
            "FieldDefinition": "camelCase",
            "ObjectTypeDefinition": "PascalCase",
            "leadingUnderscore": "allow"
          }
          ],
          "@graphql-eslint/no-anonymous-operations": ["error"],
          "@graphql-eslint/require-deprecation-reason": "error",
          "@graphql-eslint/no-case-insensitive-enum-values-duplicates": ["error"],
          "@graphql-eslint/no-operation-name-suffix": ["error"],
          "@graphql-eslint/require-id-when-available": ["error",
          {
            "fieldName": "id"
          }
          ],
          "@graphql-eslint/validate-against-schema": ["error"],
          "@graphql-eslint/require-description": [
            "error",
            { "on": ["ObjectTypeDefinition", "FieldDefinition"] }
          ]
      },
      "parserOptions": {
        "operations": ["./packages/modules/**/src/sdk/fragments/*.gql","./packages/modules/**/src/sdk/mutations/*.gql","./packages/modules/**/src/sdk/queries/*.gql"]
      }
    }
  ]

You need to specify where your operations file are, I might try to improve that by reading it from eslintrc file, but not 100% sure yet.

Regarding this, do you mean .graphqlrc ?

tvvignesh commented 3 years ago

Also, one more issue I guess you might hit (not sure) by defining operations like this is that, there is no scope for that, unlike .graphqlrc where I can specify them within a project whereas this is global. So, if there are multiple projects in a single workspace like what I have, then I am not sure how that can be handled here.

dotansimha commented 3 years ago

Yeah, actually I aim to allow simple setup, without scopes via this kind of configuration (so you specify it directly under parserOptions) and with graphql-config you can get scoped environments.

Regarding this, do you mean .graphqlrc ?

I mean, to use "files": ["*.gql"], section by default

tvvignesh commented 3 years ago

@dotansimha Got it. So, what you mean is that if I have the operations specified in graphqlrc file like this: documents: ./packages/modules/**/src/sdk/**/*.db.gql I need not specify operations in eslintrc right? Thanks.

dotansimha commented 3 years ago

@dotansimha Got it. So, what you mean is that if I have the operations specified in graphqlrc file like this: documents: ./packages/modules/**/src/sdk/**/*.db.gql I need not specify operations in eslintrc right? Thanks.

Yeah, exactly :) Doing it now :)

bennypowers commented 3 years ago

I tried

    {
      "files": ["src/**/*.graphql", "server/**/*.graphql"],
      "parser": "@graphql-eslint/eslint-plugin",
      "plugins": ["@graphql-eslint"],
      "parserOptions": {
        "operations": [
          "src/**/*.query.graphql",
          "src/**/*.mutation.graphql",
          "src/**/*.subscription.graphql"
        ],
        "schema": [
          "server/graphql/**/*.schema.graphql",
          "src/**/*.client.schema.graphql"
        ]
      },

directories having been copied from .graphqlrc

and I got, on each file

/path/to/src/components/app/App.client.schema.graphql 0:0 error Parsing error: Project '/path/to/src/components/app/App.client.schema.graphql' not found

Same for

"schema": "**/*.schema.graphql",
"operations": [
  "src/**/*.query.graphql",
  "src/**/*.mutation.graphql",
  "src/**/*.subscription.graphql"
]
bennypowers commented 3 years ago

With same configs as last post

+ @graphql-eslint/eslint-plugin@0.5.0-alpha-ec33023.0
$ npm run lint  
> neuerenergy-pwa@0.1.0 lint /path/to/app
> run-s lint:*

> neuerenergy-pwa@0.1.0 lint:client /path/to/app
> eslint src

Oops! Something went wrong! :(

ESLint: 7.14.0

Error: You have used a rule which requires GraphQL operations to be loaded. Found "parserServices" generated, but unable to load your GraphQL operations.
Occurred while linting /path/to/app/src/apollo/fragments/Address.fragment.graphql:1
    at requireSiblingsOperations (/path/to/app/node_modules/@graphql-eslint/eslint-plugin/index.cjs.js:21:15)
    at SelectionSet (/path/to/app/node_modules/@graphql-eslint/eslint-plugin/index.cjs.js:665:34)
    at /path/to/app/node_modules/eslint/lib/linter/safe-emitter.js:45:58
    at Array.forEach (<anonymous>)
    at Object.emit (/path/to/app/node_modules/eslint/lib/linter/safe-emitter.js:45:38)
    at NodeEventGenerator.applySelector (/path/to/app/node_modules/eslint/lib/linter/node-event-generator.js:254:26)
    at NodeEventGenerator.applySelectors (/path/to/app/node_modules/eslint/lib/linter/node-event-generator.js:283:22)
    at NodeEventGenerator.enterNode (/path/to/app/node_modules/eslint/lib/linter/node-event-generator.js:297:14)
    at CodePathAnalyzer.enterNode (/path/to/app/node_modules/eslint/lib/linter/code-path-analysis/code-path-analyzer.js:711:23)
    at /path/to/app/node_modules/eslint/lib/linter/linter.js:952:32
tvvignesh commented 3 years ago

I am facing a navigation issue with VSCode graphql using fragments and PNPM workspaces. Reported it here: graphql/graphiql#2344

dotansimha commented 3 years ago

Added another round of fixes in https://github.com/dotansimha/graphql-eslint/commit/08007cbf53731b29fba1cf3c01cb7db7eec7222c , now it should work better with inline fragments. Available in @graphql-eslint/eslint-plugin@0.5.0. Let me know if there are other issues.

bennypowers commented 3 years ago

This looks great @dotansimha , thanks

I got the error message

Error: You have used a rule which requires GraphQL operations to be loaded. Found "parserServices" generated, but unable to load your GraphQL operations.

And, after reading the README, figured I needed to add operations to parserServices, but this wasn't clear from the message. Even from README, it wasn't clear that if i have a .graphqlrc, I have to add my operations to the top-level documents field. Users might get confused, so it should be something more like:

Error: Rule '${ruleName}' requires parserOptions.operations to be set. See https://url.shortener/whatever for more info

dotansimha commented 3 years ago

Sure, you are right, @bennypowers , I'll improve that message :) Thank you!

dotansimha commented 3 years ago

@bennypowers I fixed it in: https://github.com/dotansimha/graphql-eslint/commit/1594288e70837b70b895b180e34b2a4de59cbbdd