Ensure last results are reset when working with partial data

[hwillson]

Issue #6334 exposed a problem where the lastResult mechanism we use to prevent duplicate subscription notifications (when data hasn't changed) can unintentionally block certain results from propagating through Apollo Client. This leads to issues like loading states not being updated properly, due to new partial results looking similar to last results.

This commit ensures that last results are properly cleared out when new partial results come in.

Fixes #6334

[benjamn]

Looks good in terms of fixing the bug, just one comment about our React testing approach (below).

[benjamn]

If I comment out this line, the test fails as it should, but the test failures appear to be associated with a different test, unless I change the itAsync to itAsync.only.

I'm not sure if this is a limitation in Jest or the React component testing approach we're using for these tests, but it seems like something we should address eventually, to prevent confusion when tests fail.

[hwillson]

Thanks @benjamn, I agree. I just ran a few quick tests with the latest version of @testing-library/react (which is 1 major version greater than the version we're using), and it looks like this issue disappeared. There are a few other failures after upgrading though, so I'll get this PR in place, then look into opening a separate PR to address the @testing-library/react upgrade.

[michaelcbrook]

I can confirm this is absolutely still an issue in Apollo Client 3.6.9 @benjamn @hwillson. I've been wracking my brain with this problem for a few days now, and finally narrowed it down to the fact that when two subsequent queries (with different variables in the request) return the same result, "loading" switches to true and never returns back to false, even after the query returns successfully.

During my investigation, I discovered this has been a long-standing issue and should have been fixed with this PR a couple of years ago, but I am still experiencing this issue with the latest version of Apollo. Of note, I am not using React. I am interacting directly with an Apollo Client observable.

The way I was able to confirm that is indeed being caused by Apollo Client is that I created a test scenario on the server where the GraphQL server would return a fresh timestamp with each response. Lo and behold, that worked, presumably because now the response is never being served from the cache.

I tried many other approaches without success: changing networkPolicy to network-only, no-cache, and cache-and-network. None of them worked. I even tried setting pollInterval to 0, which some people in the GitHub comments had suggested. But that did not work either. It's interesting that instructing networkPolicy to not use the cache at all still resulted in it comparing against the cache.

It's just a hunch, but I wonder if this part of the code could explain the problem:

  private reportResult(
    result: ApolloQueryResult<TData>,
    variables: TVariables | undefined,
  ) {
    const lastError = this.getLastError();
    if (lastError || this.isDifferentFromLastResult(result)) {
      if (lastError || !result.partial || this.options.returnPartialData) {
        this.updateLastResult(result, variables);
      }

      iterateObserversSafely(this.observers, 'next', result);
    }
  }

Maybe it was supposed to be:

  private reportResult(
    result: ApolloQueryResult<TData>,
    variables: TVariables | undefined,
  ) {
    const lastError = this.getLastError();
    if (lastError || !result.partial || this.options.returnPartialData || this.isDifferentFromLastResult(result)) {
      this.updateLastResult(result, variables);
      iterateObserversSafely(this.observers, 'next', result);
    }
  }

You guys would know better than I would, but it's easy to see how this.updateLastResult(result, variables) would never get executed if there are no errors and the new result is the same as the last result.

Just for completion, I'm also running the graphql package at 16.6.0, in addition to @apollo/client at 3.6.9.

Until this is fixed, unfortunately, I'm left with no workarounds other than to change my GraphQL API, which I would really like to avoid because it's a public-facing API.

Thank you!

[hwillson]

👋 @michaelcbrook - this PR was addressing a very specific issue, which was validated by the included test. Sounds like there are more things to look at. Let's continue the discussion in #10105. Thanks!