Invalid "Variable is never used" warning

[tgriesser]

trafficstars

First off, great tool! I haven't dug too far into it, but I just loaded it up with this gql query / fragment combo and I'm seeing the error:

Variable $first is never used in operation "getUserTracks".

If i'm not mistaken, it's not taking into account any fragments which might eventually make use of the variable (which seems to be allowed in the spec)?

const myItemsFragment = createFragment(gql`
  fragment myItems on AuthenticatedUser {
    tracks(first: $first, after: $offset) {
      edges {
        cursor
        item: node {
          ...itemFragment
        }
      }
      pageInfo {
        endCursor
      }
    }
  }
`)

const ConnectedRows = graphql(gql`
  query getUserItems($first: Int!, $offset: String) {
    me {
      displayName,
      ...myItems
    }
  }
`, {
  options: {
    fragments: [myItemsFragment, itemFragment],
  },
  props({data: { loading, me }}) {
    if (loading) {
      return {
        loading
      }
    }
    const { items, ...userInfo } = me
    return {
      loading,
      userInfo,
      items: me.items.edges,
      after: me.items.pageInfo.endCursor
    }
  }
})(Rows);

[stubailo]

That's a good call. I think this means we should remove the "unused variable" validation rule.

I guess once the query is split across multiple strings a lot of the rules become less useful, and so it might be worthwhile to have a more holistic tool that looks at all queries together.

Mind sending a PR to remove these? https://github.com/apollostack/eslint-plugin-graphql/blob/d3784f5b21649818f3bbced0e89bd06df49a58cb/src/index.js#L25-L26

We're currently looking at building a CLI tool for persisted queries that will aggregate all queries from the whole project, so maybe that tool can be smarter at actually validating variables across fragments (which would actually be a super useful feature).

[tgriesser]

As I understand it, the issue here is that the gql query has no knowledge of the fragments it uses until it is constructed and executed with graphql(query, options), where the options object contains the fragments referenced in the query.

Without knowing much about the overall architecture of apollo, I am curious whether you've considered using the gql tag to be able to interpolate fragments in the tagged template directly, rather than needing to provide them as options:

const itemFragment = gql`
fragment item on Item {
   title
   content
}
`

const myItemsFragment = gql`
fragment myItems on AuthenticatedUser {
  items(first: $first, after: $offset) {
    edges {
      cursor
      item: node {
        ${itemFragment}
      }
    }
    pageInfo {
      endCursor
    }
  }
}
`

const getUserItemsQuery = gql`
query getUserItems($first: Int!, $offset: String) {
  me {
    displayName,
    ${myItemsFragment}
  }
}
`

I presume if the entire body of a query can be derived from just the tagged template output, it'd be possible to properly validate the query with eslint.

[stubailo]

Interesting - the way we have designed it today you can simply use the fragment name from the other string, and they are enforced to be unique. This is so that queries can be easily validated and persisted without needing to follow variable references.

Does string interpolation make it easier to access the other fragment's contents with eslint?

[tgriesser]

Interesting - the way we have designed it today you can simply use the fragment name from the other string, and they are enforced to be unique. This is so that queries can be easily validated and persisted without needing to follow variable references.

I think this might actually make things harder to validate, since the full definition of the query wouldn't be known until execution time.

Does string interpolation make it easier to access the other fragment's contents with eslint?

Yeah, you'd be interpolating the fragment document objects returned from other gql tagged templates (where the fragments are created). Then you would concat those onto the query.

This would make it simpler to not necessitate that the fragment names be unique, since that pattern might not always scale well to larger codebases.

I took an initial pass at what I'm talking about as a PR over here.

[tgriesser]

Does string interpolation make it easier to access the other fragment's contents with eslint?

Actually I might be wrong on my answer to this above... I'm not sure if it makes it easier, will need to look further into how the eslint plugin is working.

[stubailo]

Here's the reason we have it the way it is now:

Given this code:

const myItemsFragment = createFragment(gql`
  fragment myItems on AuthenticatedUser {
    tracks(first: $first, after: $offset) {
      edges {
        cursor
        item: node {
          ...itemFragment
        }
      }
      pageInfo {
        endCursor
      }
    }
  }
`)

const ConnectedRows = graphql(gql`
  query getUserItems($first: Int!, $offset: String) {
    me {
      displayName,
      ...myItems
    }
  }
`,

You can easily extract the following document:

  fragment myItems on AuthenticatedUser {
    tracks(first: $first, after: $offset) {
      edges {
        cursor
        item: node {
          ...itemFragment
        }
      }
      pageInfo {
        endCursor
      }
    }
  }

  query getUserItems($first: Int!, $offset: String) {
    me {
      displayName,
      ...myItems
    }
  }

Simply by extracting the stuff inside gql. Then you can really easily just run graphql-js validation on it.

This will no longer be possible if using string interpolation - you will have to do complicated JS AST transforms, follow imports, etc.

This is why I claim that the syntax today is easier to statically analyze. Perhaps ESLint is the wrong approach, but it should be trivial to build a tool that uses regex to just get all the GraphQL strings, concatenates them, and runs validation on the entire thing at once.

What do you think?

[cprass]

Is there a way to remove the validators in the configuration until this is fixed (or what's the status of the issue anyway)?

I am using the standard apollo config and I only saw a way to add validators with the validators property.

I guess the alternative would be to use a custom config and add all from the apollo config except the two validators that are broken, right?

[MartinDawson]

Indeed, having no way of black listing validation rules is annoying.

[leethree]

Any update on this?

[cprass]

I was finally able to find a workaround. It works by importing all validators that eslint would use and then filtering out the rules that are not working correctly. Then just override the list of validators used in the rules-section.

// .eslintrc.js

const { specifiedRules: allGraphqlRules } = require('graphql');

const validators = allGraphqlRules
  .map(rule => rule.name)
  .filter(ruleName => [
    'NoUnusedFragments',
    'KnownFragmentNames',
    'NoUnusedVariables'
  ].findIndex(x => x === ruleName) === -1);

module.exports = {
  parser: "babel-eslint",
  rules: {
    "graphql/template-strings": ['error', {
      // .graphql files
      env: 'literal',
      schemaJson: require('./schema.json'),
      validators
    }, {
      env: 'apollo',
      schemaJson: require('./schema.json'),
    }]
  },
  plugins: [
    'graphql'
  ]
}

[jorgeiglopez]

I'm having a similar error message, but I'm not using fragments. The way I avoid the error was getting rid of "first" in the query. But I need to be able to count more than the default 10 results.

[hutber]

I would thoroughly love to be able suppress the unused variable rule. For example I have a query that has filters, meaning it would be very flexible for me to just pump in an empty query variable and have it return the filters if they are there or not.