Possible bugs surrounding invariants.

[jimmyhmiller]

As I was diving into the code to see if I could help capture some invariants to improve error messages, I noticed some strange things in the code. I wanted to discuss it to make sure I wasn't misunderstanding before just making an unsolicited PR

  if (queries.length && mutations.length && mutations.length) {
    invariant(((queries.length + mutations.length + mutations.length) > 1),
      // tslint:disable-line
      `react-apollo only supports a query, subscription, or a mutation per HOC. ${document} had ${queries.length} queries, ${subscriptions.length} subscriptions and ${mutations.length} muations. You can use 'compose' to join multiple operation types to a component`
    );
}

https://github.com/apollographql/react-apollo/blob/master/src/parser.ts#L59-L64

Here the invariant is wrapped in an additional if statement, wasn't quite sure why that would be. It doesn't seem necessary and would duplicate the check. It also seems to have a bug where uses mutations.length twice, when based on the error message it should be using subscriptions.

And then there is this.

  if (fragments.length && (!queries.length || !mutations.length || !subscriptions.length)) {
    invariant(true,
      `Passing only a fragment to 'graphql' is not yet supported. You must include a query, subscription or mutation as well`
    );
}

https://github.com/apollographql/react-apollo/blob/master/src/parser.ts#L53-L57

invariant will never throw if the first variable is truthy. So this will never be called.

  invariant((!!document || !!document.kind),
    // tslint:disable-line
    `Argument of ${document} passed to parser was not a valid GraphQL DocumentNode. You may need to use 'graphql-tag' or another method to convert your operation into a document`
);

https://github.com/apollographql/react-apollo/blob/master/src/parser.ts#L32-L35

This invariant does not have that issue of being surrounded in ifs or always being truthy, but I don't think it is doing what is intended. If document is undefined then !!document is false. That means we will go to the second case, where we will throw an error for trying to access the kind property of undefined. I think what was meant was !!document && !!document.kind. Basically we are trying to make sure document.kind exists.

And then, as I was looking to fix these problems, I noticed the tests aren't setup properly.

  it('should error if both a query and a mutation is present', () => {
    const query = gql`
      query { user { name } }
      mutation ($t: String) { addT(t: $t) { user { name } } }
    `;

    try { parser(query); } catch (e) {
      expect(e).toMatch(/Invariant Violation/);
    }
});

https://github.com/apollographql/react-apollo/blob/master/test/parser.test.ts#L15-L24

If parser(query) doesn't throw, this test will still pass. It should be using expect(...).toThrowError() if the expectation is that it should throw an error.

There may well be a very good reason for some of these things. I just wanted to see if there was or wasn't before opening a PR. If these are actual bugs I'm happy to dive in an fix them.

[helfer]

@jimmyhmiller You're absolutely right on all of these! I'd be very glad to merge your PR fixing this 🙂