Mike-Gibson / mock-apollo-client

Helps unit test components which use the Apollo Client.
MIT License
117 stars 15 forks source link

I always get "Request handler not defined for query:..." #15

Closed justinmchase closed 4 years ago

justinmchase commented 4 years ago

This line is getting the key from the query: https://github.com/Mike-Gibson/mock-apollo-client/blob/e4afa4142ca24c1cb2d060767681e09f41d92a01/src/mockLink.ts#L24

But on the set its getting the keys from identifiers: https://github.com/Mike-Gibson/mock-apollo-client/blob/e4afa4142ca24c1cb2d060767681e09f41d92a01/src/mockLink.ts#L13f

Which, even though I am passing the exact same query (by reference) in both cases they generate different keys.

"{"query":"query Nextweb_PersonCardUser($userId: ID) {\n  user(id: $userId) {\n    _id\n    avatarUrl\n    fullName\n    bannerUrl\n    profile {\n      positionTitle\n      spotlightedFields {\n        _id\n        value\n        format\n        leadIn\n        leadOut\n        icon\n        ... on DateField {\n          dateFormat\n        }\n      }\n    }\n  }\n}\n"}"
"{"query":"query Nextweb_PersonCardUser($userId: ID) {\n  user(id: $userId) {\n    _id\n    avatarUrl\n    fullName\n    bannerUrl\n    profile {\n      positionTitle\n      spotlightedFields {\n        _id\n        value\n        format\n        leadIn\n        leadOut\n        icon\n        ... on DateField {\n          dateFormat\n          __typename\n        }\n        __typename\n      }\n      __typename\n    }\n    __typename\n  }\n}\n"}"

And thus I always get the Request handler not defined for query: ... even though its the same query, I only have 1 query registered right now.

justinmchase commented 4 years ago

Actually I don't actually have the __typename field in my query yet somehow its being added when the query is actually made.

justinmchase commented 4 years ago

It looks like its in the InMemoryCache and I can add addTypename: false to fix it.

const mockClient = createMockClient({
  cache: new InMemoryCache({
    addTypename: false, // Does not work without this.
  }
})

It may be worth adding that tidbit here: https://github.com/Mike-Gibson/mock-apollo-client#specifying-apollo-client-options

I'm not sure if there is a more robust fix which would solve it better but I was going down the route of just using the operationName instead of a serialized version of the entire query object. I don't see why that wouldn't be good enough... but just adding this setting does fix it for me.

If you'd like to recommend a preferred fix (documentation update or other) and would like a PR let me know and I'll send some kind of a patch.

justinmchase commented 4 years ago

Ok more problems, it looks like if you use a fragment then it implicitly adds the __typename but if you have addTypename: false then that gets messed up too.

... on DateField {
  dateFormat
}

So I'm really thinking that just mapping it by operation name is way simpler... provided you don't have multiple operations with the same name.

justinmchase commented 4 years ago

Yeah, confirmed that is happening. If I manually add __typename to every object in my query then it works... if I don't then apollo will add it for fragments and it will break the query matching method.

Mike-Gibson commented 4 years ago

Hi, thanks for raising the detailed issue and investigating the codebase.

The addTypename functionality is a big source of problems when using queries in tests. There's a bit more info in https://github.com/Mike-Gibson/mock-apollo-client/issues/8 - have you come across that yet?

The default cache already sets addTypename to false: https://github.com/Mike-Gibson/mock-apollo-client/blob/master/src/mockClient.ts#L26

So you should not have to specify the cache when creating the mock client.

Fragments are also tricky when using queries in tests, and there is a discussion in https://github.com/Mike-Gibson/mock-apollo-client/pull/5 about it, but this doesn't tackle the addTypename issue.

This issue looks like a combination of both!

A recent change was made to allow the Apollo client constructor options to be passed through the mock client. This was to try and support custom fragment matchers and cache options, which you've found. How is the Apollo client configured in your production code? Any chance you can provide code to repro the issue?

justinmchase commented 4 years ago

I ended up adding a kind of minimalist version of this that worked for me to my repo because I have a lot of queries which omit the __typename but also use fragments. So both cases were giving me errors.

Here is the createMockClient function, which uses the fragmentMatcher but does not have addTypename set (because that breaks fragments).

export const createMockClient = (
  options: MockApolloClientOptions = {},
  handlers: Record<string, RequestHandler> = {},
): MockApolloClient => {
  const mockLink = new MockLink(handlers);
  return new ApolloClient({
    cache: new InMemoryCache({
      fragmentMatcher: new IntrospectionFragmentMatcher({
        introspectionQueryResultData,
      }),
    }),
    ...options,
    link: mockLink,
  });
};

Then my solution to finding the right handler is to not try to serialize the entire query but rather to just key off of the operation.operationName in the link.

export class MockLink extends ApolloLink {
  constructor(private readonly handlers: Record<string, RequestHandler> = {}) {
    super();
  }

  request(operation: Operation) {
    const handler = this.handlers[operation.operationName];
    if (!handler) {
      throw new Error(`Request handler not defined for query: ${operation.operationName}`);
    }

    try {
      const handlerResult = handler(operation.variables);
      return new Observable<FetchResult>((observer) => {
        (async () => {
          try {
            const result: any = await handlerResult;
            observer.next(result);
          } catch (err) {
            observer.error(err);
          } finally {
            observer.complete();
          }
        })();
        return () => ({});
      });
    } catch (error) {
      throw new Error(`Unexpected error whilst calling request handler: ${error.message}`);
    }
  }
}

Then in my test I create a component which accepts an operation name and a data object such as:

const userId = "u0";
const user = { __typename: "User",  _id: userId, ... };
return (
  <MockApollo operation="GetUserInfo" response={{ data: { user } }}>
    <UserInfo id="user-test" userId={userId} />
  </MockApollo>
);

The UserInfo component is doing a gql query for an operation called "GetUserInfo" which returns the mock user object I have created here. This all works for both scenarios, the only "downside" is that I have to add __typename to each object in my mock result data. Which is frankly fine.

Mike-Gibson commented 4 years ago

I'm glad to see you've found a solution for your problem.

I took inspiration for the serialisation from the official Apollo testing approach - I'm not sure what could/would break if it changed to using the operation name instead.

It would be great if you could provide a repro of the problem - I'd like to look at whether there's something I can change in this library to help others that might run into the same issue. Would you be able to do that?

justinmchase commented 4 years ago

Its going to be tough for me to make a stand alone repro quickly but it was a query like this:

const PERSON_CARD_QUERY = gql`
  query PersonCardUser($userId: ID) {
    user(id: $userId) {
      _id
      avatarUrl
      fullName
      bannerUrl
      profile {
        positionTitle
        fields {
          _id
          value
          format
          ... on DateField {
            dateFormat
          }
        }
      }
    }
  }
`;

Then when I was using this library I had a custom cache for fragments:

const mockClient = createMockClient({
  cache: new InMemoryCache({
    fragmentMatcher: new IntrospectionFragmentMatcher({
      introspectionQueryResultData,
    }),
  }
})

This was failing with the original error in this repo because i didn't have addTypename: false set, so it was mismatching queries when trying to resolve them because it was adding typename in the cache and was different. So then I discovered the addTypename and set that and it appeared to work but I had fields: [] in my mock data. Once I set the fields to have mock objects it started to error because there it needs the typename to resolve the fragment. And there lies the problem, the once addTypename: false is set even if you add it to your mock data it will strip it off and fail to resolve the fragments and give you errors. So I had to unset it... but then the queries differed. Thus the reliance on operation name.

So I bet you could solve it by forcing addTypename to true, then before storing the incoming query, add a typename to all ast nodes and make up a type like "Mock". Another way might be to make a MockCache which works like the in memory cache but doesn't add typename but is smart enough to still resolve the fragments. Like maybe just caching the query twice, once raw and once with the added typenames. The raw is used to resolve mocks but the modified is used to actually operate. Sounds hard but its probably possible. If I get some time I'll try to make a demo but it may be more work than I have time for.

Mike-Gibson commented 4 years ago

Unfortunately I didn't have time to investigate the issue any further. I have just pushed a new version which supports Apollo client 3, and version 3 changes the approach to fragment matching, so it might solve this problem (but could well create new ones!).

I'm going to close this issue for now. Please re-open if there are further developments.

justinmchase commented 4 years ago

No problem I have a workaround and you have some ideas how to address it later if you want.