QueryInfo needlessly retriggering queries in @apollo/client 3.x

[stephenh]

Intended outcome:

I'm upgrading from apollo-client 2.x -> 3.x, and fixing a now-broken test that tests a React component that makes two useQuery calls:

  // 1st query
  const tasksQuery = useTasksQuery({ variables: { filter, order } });
  // 2nd query
  const { data: currentInternalUserQueryData } = useCurrentInternalUserQuery();

And I'm setting up MockedProvider with a valid response for the 2nd query and an error response for the 1st query:

// this is the error response for 1st query
export const errorMyTasksResponse = newTasksResponse(
  { filter: defaultTasksFilter, order: { id: Order.Asc } },
  new Error("An error occurred"),
);
// ...leaving out valid response for 2nd query...

...

// rendering the component with valid 2nd query response and error 1st query response
    const { baseElement } = await render(
      <MyTasksPage />,
      withApollo(internalUserResponse, errorMyTasksResponse, errorMyTasksResponse),
      atUrl("/my-tasks"),
    );

Note that I have to pass the errorMyTasksResponse twice (this varargs array to withApollo ends up getting passed to MockedProvider's mocks attribute).

Actual outcome:

If I pass only a single errorMyTasksResponse (which is what we did on apollo-client 2.x), I get an "No more mocked responses" for the useTasksQuery on the 2nd time the query evaluates.

However, none of the input variables are changing, so the query AFAICT should not be re-triggered a 2nd time.

Setting breakpoints, the 2nd query is triggered by QueryInfos notifyTimeout and ends up here, thinking that the query result has changed (see the expanded diff and oldDiff results in the bottom variable pane):

However, what's weird here is that oldDiff.result and diff.result are actually both {}, i.e. it seems to me that this shouldn't be considered "not different". (Tangentially, I'm a little surprised in general that diff vs. oldDiff is an === instead of a structural equal, but I assume somewhere in the guts of the caching/etc. stuff, it's taking care of immutable/identity/something.)

That said, it seems broken for this {} vs. {} case. cc @benjamn apologies for the @ but you're all over the git blame here and I think would be best to at least guess if {} === {} should actually be true for this boundary case.

More guessing, but I wonder if result = {} because I'm setting up the mock with an error? I.e. I've hit this debug point with other queries and usually result isn't {}.

How to reproduce the issue:

I can work on the react-apollo-error-template thing but wanted to file this w/o at while it's in my head.

Note:

1. Having two queries is important to trigger this behavior, i.e. if I comment out the 2nd useCurrentInternalUserQuery query, then the error response query works just fine and isn't needless re-triggered
2. I'm technically using jest.useFakeTimers and it's one of the runPendingTimers that is triggering the notifyTimeout. I'm pretty sure that isn't causing the bug, but just in terms or reproducing.

Versions

$ npx envinfo@latest --preset apollo --clipboard
npx: installed 1 in 1.484s

  System:
    OS: Linux 5.4 Ubuntu 20.04.1 LTS (Focal Fossa)
  Binaries:
    Node: 13.11.0 - ~/.nvm/versions/node/v13.11.0/bin/node
    Yarn: 1.22.4 - /usr/bin/yarn
    npm: 6.13.7 - ~/.nvm/versions/node/v13.11.0/bin/npm
  Browsers:
    Chrome: 84.0.4147.135
    Firefox: 79.0
  npmPackages:
    @apollo/client: ^3.1.3 => 3.1.3

[benjamn]

@stephenh Give @apollo/client@3.2.0-beta.5 (which includes #6891) a try?

[evaneaston]

@benjamn, @stephenh FYI, I was seeing this same problem in 3.1.3 and, after updating to 3.2.0-beta.5, the unneeded requeries have stopped.

[benjamn]

@stephenh @evaneaston So, unfortunately, while #6891 made some problems go away, it also created other problems (regressions in https://studio.apollographql.com/). I've reverted that change in @apollo/client@3.2.0-beta.9, but we will keep looking for a better solution here.

Since @apollo/client@3.2.0-beta.9 reverted the commit that presumably solved your problems, I'm guessing you'll start seeing the problems again if you update to -beta.9, but I would still ask you to give that a try, just to confirm that there's nothing else between 3.1.3 and 3.2.0-beta.9 that could have solved the problem.

As always, some sort of reproduction would be ideal.

[stephenh]

@benjamn cool, thanks for the update. I tried to make a repro this morning and thought it was going to be really simple, i.e. just have multiple queries in a component where one of them has a mocked error, but famous last words I suppose, because I couldn't trigger it. :-/ :-)

I don't have our production codebase in the same easy-to-repro spot anymore, but will watch for it, and keep my WIP repro around to see if I can come back to it.

[evaneaston]

@benhjames @stephenh I can confirm that, after moving to @apollo/client@3.2.0-beta.9, the problem returns.

I will see about making a minimal reproduction today.

[evaneaston]

When using MockedProvider it works fine, not requerying unnecessarily. I can reproduce it running a simple create-react-app against a real graphql server and watching the network tab in the dev tools. I'll have to find an open/public graphql server that I reproduce this against.

[jcdelmas]

@benjamn You can find a reproduction of this issue there: https://github.com/upflowhq/react-apollo-error-template

The bug happens when a component containing a query is unmounted before receiving the response. Then, any query using the same field but with different variables re-trigger the first query.

[berickson1]

I'm seeing this issue using MockedProvider as well. Trying to update from 3.0.2 -> 3.1.4 results in extra query requests hitting MockedProvider and triggering a "No more mocked responses for the query" error.

From my investigation it looks like a call to QueryInfo.setDiff(), sets a timer, which then fires after the component has been cleaned up in the test. This pattern matches to what the original issue indicates.

[berickson1]

This is still occurring on the latest (3.2.1) release

[mvorisek7]

Can confirm that this is still an issue in 3.2.5. For some reason, the dirty check in setDiff seems to be what is failing
(diff && diff.result) !== (oldDiff && oldDiff.result)
When checking the values and comparing manually, they are exactly the same, but they're evaluating to be different.

[renatoargh]

Just to reinforce that @ v3.2.5 I have the same problem

[roberttod]

@benjamn I noticed a lot of unnecessary calls refetching queries, and traced it down to this logic. I've applied your reverted PR, #6891 which solves the issue nicely.

I suspect this is causing a decent amount of extra load for Apollo servers. What regressions did you see in https://studio.apollographql.com/ ? It looks like a fairly innocuous change.

I'm trying to repro using https://github.com/upflowhq/react-apollo-error-template , but not quite there yet. Curious if anyone is seeing this in their apps like I did, or whether it's only been isolated to testing.

[brainkim]

I’m working on #7436, which appears to have the same cause. It is both humbling and motivating to see a bunch of competent devs each come to the same conclusion I did (QueryInfo.setDiff marking oldDiff.result as referentially unequal with diff.result). The reproduction for this issue is tricky. You need to have a query which returns more than 32767 (2^15-1) items, and the query needs to be made in a child component whose parent also makes a query. If y’all can confirm that the bugs you have in this issue are of a similar nature, that would be incredibly helpful.

The nice thing is, if all of the team’s hypotheses are correct, this should be fixed in 3.4, which is currently in beta. Might be a good time to try it out! If everyone here can confirm that their bugs look like #7436, you might want to follow that one because I’ll be posting updates there. Otherwise, feel free to ping me and let me know and I’m happy to investigate.

[hwillson]

This should be resolved in @apollo/client@latest - let us know if not.