Skip @client queries related to apollo-link-state

[stipsan]

This will not fix #99, but it will at least stop queries that use @client from making the linter fail (causing grief for those who want to use both apollo-link-state and eslint-plugin-graphql).

So queries like this will no longer fail:

  query {
    networkStatus @client {
      isConnected
    }
  }

However the downside is that if any field in the query uses the @client directive the linter will be ok with it, even if there's non-client fields that should fail.
Like here:

  query {
    networkStatus @client {
      isConnected
    }
    # The typo here will not be detected
    articess {
      id
      title
    }
  }

I tried re-implementing the logic used in apollo-link-state itself to strip out fields from the query before sending it over the network: https://github.com/apollographql/apollo-link-state/blob/b81edfdd875c1e3f106ac3ecd1a3e5b0c2e7f8e5/packages/apollo-link-state/src/utils.ts#L14-L31
But I hit a Maximum call stack size exceeded error. I will look into this, but it could be worth looking into it in another PR after this one. Why?
Any query with @client will fail today, so it blocks anyone from combining this linter plugin with apollo-link-state, so this PR will not cause a regression for existing users.

TODO:

• Make sure all of the significant new logic is covered by tests
• Rebase your changes on master so that they can be merged easily
• Make sure all tests pass
• Update CHANGELOG.md with your change
• If this was a change that affects the external API, update the README

Pull Request Labels

• has-reproduction
• feature
• blocking
• good first review

[apollo-cla]

@stipsan: Thank you for submitting a pull request! Before we can merge it, you'll need to sign the Meteor Contributor Agreement here: https://contribute.meteor.com/

[jnwng]

@stipsan thanks for the PR and taking a look at this — i agree that its important that fit well with the apollo-link-state paradigm of using client directives.

unfortunately i think its a little heavy-handed to fail linting for the entire graphql document if even one part of it has a client directive. while its technically "backwards-compatible" i dont think its a great experience because we're silently ignoring the queries. i would be much more comfortable if stripped fields from the query before linting, lets figure out how to get that to work.

does that sound reasonable?

[stipsan]

I agree the best way forward is to make it strip the client parts of the query.
I'm sure the Maximum call stack is solvable, it's just a matter of going deeper in the rabbit hole.

I'll look into it and hopefully it's easier to get it working than I expected 😄

[tizmagik]

Hey @stipsan any update on this? Do you have a branch somewhere where the Maximum call stack error is happening? Would be interested in taking a look to see if I could help out 😁

[stipsan]

Hey, I’ll make a repro for you later today :)

I have been so swamped with other things, didn’t mean to drop the ball on this! 😅

[stipsan]

@tizmagik I have updated the PR so it no longer has merge conflicts, and pushed changes so that the Maximum call stack size exceeded is reproduced.

You should see something like this if you run npm test now:

[loicplaire]

This test will currently always pass. The KnownDirectives validator needs to be added to the baseConfig.rules object:

baseConfig: {
  rules: {
    'graphql/required-fields': [
      'error',
      {
        schemaJson,
        env: 'literal',
        requiredFields: ['id'],
        validators: ['KnownDirectives'],
      },
    ],
  },
},

[stipsan]

Oki, I've updated the test with that addition. I had to clone the execute function otherwise unrelated tests where failing.

[loicplaire]

@stipsan have look at this: apollographql/apollo-client#1717 (comment).

If I add { noLocation: true } here: https://github.com/stipsan/eslint-plugin-graphql/blob/2bd3721a2b68c0444d8efa219e070ec1ef526399/src/index.js#L394, then the test passes. But adding this option make other tests fails...

We could potentially do ast = parse(text, { noLocation: text.includes('@client') }); but I don't think it make sense to use that noLocation option since I am guessing that the linter needs to know the location of the error.

It seems to me that it's an issue with apollo-utilities so I wonder if you could potentially use another lib that let's you remove a directive from an AST.

I don't have a solution for this but I hope this sheds some lights on the issue you are having.

[stipsan]

Hey thanks @loicplaire! That's really helpful information that you're sharing!

I hope I get time to look at this again soon, I can't commit to a timeframe but I'll report back to this thread when I do 🙂

[stipsan]

I've cloned the removeDirectivesFromDocument utility, removing the cloneDeep part and it looks like it is passing now @loicplaire. I failed to find another package on npm that was able to remove parts of the graphql AST.

@jnwng what do you say?

[jnwng]

i took a peek at this and the cloneDeep call chokes on the “loc” property of the document. calling cloneDeep without the “loc” does what we want (i think) but requires us to either re-parse the document (this is probably okay) passing the noLoc flag or to remove the loc from our already parsed document. in either case, as long as the clone is doing what we want it should be okay to move forward!

…

On Sun, Aug 5, 2018 at 10:25 AM Cody Olsen ***@***.***> wrote: I've cloned the removeDirectivesFromDocument utility, removing the cloneDeep part and it looks like it is passing now @loicplaire <https://github.com/loicplaire>. I failed to find another package on npm that was able to remove parts of the graphql AST. @jnwng <https://github.com/jnwng> what do you say? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#117 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABERRrHnAMxiGnB4OW5sbF1m6Ung-eQyks5uNyqPgaJpZM4SoDR4> .

[stipsan]

I tried something like:

if(hasDirectives(['client'], ast)) {
  const {loc, ...astWithoutLoc} = ast
  ast = removeDirectivesFromDocument( [{ name: 'client' }], astWithoutLoc);
}

But it still hit the overflow, I can only assume because of the other .loc properties that are all over the tree.

@jnwng what do you think about making a PR on the apollo-utilities project to add a flag to skip the deep clone on removeDirectivesFromDocument so we avoid maintaining all this duplicated code here? I recon we don't really need the clone here since the ast is not used elsewhere so the mutations should be safe in this context 🤔

[MrTibbles]

Any movement on this? Feels like plenty of time for thought to be furthered.

[MrTibbles]

Resolved, see comment