Stop delivering "stale" results to ObservableQuery.

[benjamn]

One of the most counterintuitive and confusing behaviors of Apollo Client is to silently re-deliver previous results whenever an incomplete result is read from the cache here:

      // If there is some data missing and the user has told us that they
      // do not tolerate partial data then we want to return the previous
      // result and mark it as stale.
      const stale = !diff.complete && !(
        returnPartialData ||
        partialRefetch ||
        fetchPolicy === 'cache-only'
      );

      const lastResult = observableQuery.getLastResult();
      const resultFromStore: ApolloQueryResult<T> = {
        data: stale ? lastResult && lastResult.data : diff.result,
        loading: isNetworkRequestInFlight(networkStatus),
        networkStatus,
        stale,
      };

      // If the query wants updates on errors, add them to the result.
      if (errorPolicy === 'all' && hasGraphQLErrors) {
        resultFromStore.errors = graphQLErrors;
      }

      observer.next(resultFromStore);

Consider how pointless this is: we obtain observableQuery.getLastResult() and then send it immediately back to the ObservableQuery, just because it doesn't seem safe to send the incomplete diff.result.

Instead, we should never be sending stale or incomplete results to the ObservableQuery. And instead of silently dropping the stale results, we should report appropriate errors to the ObservableQuery via observer.error, so that the application can handle them. That's what this PR does.

I have not finished validating these changes against @ahrnee's reproduction in #5991 (hence the WIP status of the PR), but that's my first priority tomorrow.

[codecov]

Codecov Report

Merging #6058 into master will decrease coverage by 0.01%.
The diff coverage is 88.13%.

@@            Coverage Diff             @@
##           master    #6058      +/-   ##
==========================================
- Coverage   95.37%   95.36%   -0.02%     
==========================================
  Files          88       89       +1     
  Lines        3653     3666      +13     
  Branches      903      872      -31     
==========================================
+ Hits         3484     3496      +12     
  Misses        146      146              
- Partials       23       24       +1     

Impacted Files	Coverage Δ
src/core/ObservableQuery.ts	97.70% <ø> (-0.03%)	⬇️
src/core/types.ts	100.00% <ø> (ø)
src/utilities/testing/observableToPromise.ts	91.17% <0.00%> (ø)
src/react/data/QueryData.ts	90.52% <79.31%> (-0.48%)	⬇️
src/cache/core/types/common.ts	100.00% <100.00%> (ø)
src/cache/index.ts	100.00% <100.00%> (ø)
src/cache/inmemory/readFromStore.ts	100.00% <100.00%> (ø)
src/core/QueryManager.ts	97.69% <100.00%> (+<0.01%)	⬆️
src/errors/ApolloError.ts	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aacba4...70ae548. Read the comment docs.

[benjamn]

My reasoning for this !equal(diff.result, {}) check: results where zero fields were successfully read seem truly useless, and thus should not be reported to the client as errors, but partially successful reads with a few missing fields should be reported as errors.

[hwillson]

This all looks excellent @benjamn - and the new missing field logging is going to be super helpful! 🎉