Simplify and improve performance

[lydell]

trafficstars

This plugin is great, but unnecessarily complicated, so I'm suggesting a possibly simpler approach, which also should result in better performance and simplify the code base!

.eslintrc.js:

'graphql/queries': ['error', {
  // Mapping from tag names to config.
  'gql': {
    env: 'apollo',
    projectName: 'apollo',
    validators: [
      'default', // Allows extending the default.
      'NamedOperations', // Replaces `graphql/named-operations`.
      'RequiredFields', // Replaces `graphql/required-fields`.
      'CapitalizedTypeNames', // Replaces `graphql/capitalized-type-name`.
    ],
    requiredFields: ['id'],
  },
  'Relay.QL': {
    env: 'relay',
    projectName: 'relay',
  },
  '.graphql': { // Special name for targeting literal files.
    env: 'apollo', // NOT 'literal'. Want to base `validators` on 'apollo'.
    projectName: 'apollo',
  },
],

.graphqlconfig:

{
  "projects": {
    "apollo": {
      "schemaPath": "./schema-apollo.json"
    },
    "relay": {
      "schemaPath": "./schema-relay.graphql"
    }
  }
}

Changes:

• graphql/queries, graphql/named-operations, graphql/required-fields and graphql-capitalized-type-name are merged into graphql/template-literals. This means that you won't have to specify the same options to four rules, and that everything can be linted in one pass instead of parsing every GraphQL query four times. All rules already share the same code anyway, and are implemented as validators.

• graphql/queries takes a single object as config, which is a mapping from tag names that you want to check, to config for that tag name. The special .graphql name maps to .graphql and .gql files (current env: 'literal').

• env is now required. Explicit is better than implicit.

• env: 'literal' is removed. As said, '.graphql': {...} is used instead. This also allows specifying env: 'apollo' for .graphql files to get the set of validators for Apollo.

• schemaJson, schemaJsonFilepath and schemaString are all removed. Just use .graphqlconfig and get it all for free.

• requiredFields is only required if validators contains 'RequiredFields', and it must contain at least one required field.

What do you think?

[jnwng]

@lydell thank you for thinking through all this, and apologies for taking so long to get back to you. overall there's a lot of good stuff here, and i'd like to start a discussion about some of the specific things. addressing in order:

Merging all rules into graphql/template-literals

my only concern here is that the concept of validators is not an eslint concept, but rather a graphql concept. however, you raise a really good point here — maybe we should be sending these upstream to graphql-js as validator additions? /cc @stubailo in case you have thoughts on this (i would suspect that graphql/named-operations and graphql/capitalized-type-name are good candidates)

additionally, this means that you cannot change the granularity of warning for these individual rules. i.e., if you only want to "warn" on unnamed types, you cannot specify that the way things are laid out.

i dont know if the performance overhead of parsing queries multiple times is actually something we should be concerned with, but please let me know otherwise

tagNames as key names in config

this seems pretty reasonable to me. i can't think off the top of my head a case where you'd want to have different options / rules for the same tagName, so enforcing exclusivity seems fine.

however, it does add a little more overhead to configuring this rule since gql is normally the default, and i also dont know how often people are using multiple tagNames in the first place. it also doesn't allow us to default to a tagName for a particular environment to make setup simpler

env is required

if our goal is to be a linter for all graphql projects, then i agree that removing apollo as the default makes sense

remove literal for .graphql

being able to separate the env from where the query is retrieved makes a ton of sense to me. using .graphql as "magic" name seems a little prescriptive since its possible to have files with different extensions. can we decouple these two suggestions?

use .graphqlconfig

let's do it. dealing with schema locations is pretty frustrating but .graphqlconfig has resolved the issues around specifying where they are.

---

as should be fairly obvious, this is a major overhaul of eslint-plugin-graphql, but it coincides with my own thoughts on general housekeeping tasks like separating out each of the rules into their own files so we dont have this huge rules file. since we're basically writing our own graphql validators, too, it seems to make sense to separate each into their own validation step (and we can combine them as necessary depending on which ones are specified). i hope the comments i left are useful in starting a list of discrete tasks we can address for a new major version!

thank you again!

[lydell]

apologies for taking so long to get back to you.

Hey, don't worry: 3 days is nothing! :)

Merging all rules into graphql/template-literals: my only concern here is that the concept of validators is not an eslint concept, but rather a graphql concept.

That's a totally valid point. We already have the concept of validators though (since it is possible to customize which validators from the graphql package to run), and since the other rules are just validators in disguise I think it is less confusing if they are exposed as validators as well. Actually, I don't think the users care if they need to enable some more rules or some more validators. I think they'd just appreciate the ease of configuration :)

additionally, this means that you cannot change the granularity of warning for these individual rules. i.e., if you only want to "warn" on unnamed types, you cannot specify that the way things are laid out.

Personally I use "error" for every single ESLint rule I enable, so I hadn't thought of that. Good to keep in mind while discussing :)

i dont know if the performance overhead of parsing queries multiple times is actually something we should be concerned with, but please let me know otherwise tagNames as key names in config

I don't know either. It might be a pre-mature optimization. Either way, the most important thing I wanted to bring up with this issue was to make this plugin easier to configure and understand.

i can't think off the top of my head a case where you'd want to have different options / rules for the same tagName, so enforcing exclusivity seems fine.

Actually, it is already invalid to specify different rules for the same tagName: https://github.com/apollographql/eslint-plugin-graphql/blob/45b3f95dcdaa5154233c10fb90a21da20c8ad1f5/src/index.js#L80-L82

however, it does add a little more overhead to configuring this rule since gql is normally the default, and i also dont know how often people are using multiple tagNames in the first place. it also doesn't allow us to default to a tagName for a particular environment to make setup simpler

This is true, but we already have copy-pastable configs for every env in the readme; I don't think this way would be any more difficult.

---

Thanks for the positive comments! I'm happy to help out here when we've decided more on how to proceed :)