Merge pull request #8642 from apollographql/avoid-voiding-result.data…

…-in-getCurrentResult

Avoid voiding `result.data` in `ObservableQuery#getCurrentResult` when `!diff.complete`

CHANGELOG.md

@@ -5,6 +5,9 @@
 - Fix error thrown by nested `keyFields: ["a", ["b", "c"], "d"]` type policies when writing results into the cache where any of the key fields (`.a`, `.a.b`, `.a.c`, or `.d`) have been renamed by query field alias syntax. <br/>
   [@benjamn](https://github.com/benjamn) in [#8643](https://github.com/apollographql/apollo-client/pull/8643)
 
+- Fix regression from PR [#8422](https://github.com/apollographql/apollo-client/pull/8422) (first released in `@apollo/client@3.4.0-rc.15`) that caused `result.data` to be set to undefined in some cases after `ObservableQuery#getCurrentResult` reads an incomplete result from the cache. <br/>
+  [@benjamn](https://github.com/benjamn) in [#8642](https://github.com/apollographql/apollo-client/pull/8642)
+
 ## Apollo Client 3.4.7
 
 ### Bug Fixes

package.json

@@ -55,7 +55,7 @@
     {
       "name": "apollo-client",
       "path": "./dist/apollo-core.cjs.min.js",
-      "maxSize": "24.6 kB"
+      "maxSize": "24.7 kB"
     }
   ],
   "engines": {

src/__tests__/client.ts

@@ -2590,15 +2590,16 @@ describe('client', () => {
         subscription.unsubscribe();
 
         const lastError = observable.getLastError();
-        const lastResult = observable.getLastResult();
+        expect(lastError).toBeInstanceOf(ApolloError);
+        expect(lastError!.networkError).toEqual(error);
 
+        const lastResult = observable.getLastResult();
         expect(lastResult).toBeTruthy();
         expect(lastResult!.loading).toBe(false);
         expect(lastResult!.networkStatus).toBe(8);
 
         observable.resetLastResults();
         subscription = observable.subscribe(observerOptions);
-        Object.assign(observable, { lastError, lastResult });
 
         // The error arrived, run a refetch to get the third result
         // which should now contain valid data.

src/core/ObservableQuery.ts

@@ -45,6 +45,12 @@ export interface UpdateQueryOptions<TVariables> {
 
 let warnedAboutUpdateQuery = false;
 
+interface Last<TData, TVariables> {
+  result: ApolloQueryResult<TData>;
+  variables?: TVariables;
+  error?: ApolloError;
+}
+
 export class ObservableQuery<
   TData = any,
   TVariables = OperationVariables
@@ -68,12 +74,15 @@ export class ObservableQuery<
   private observers = new Set<Observer<ApolloQueryResult<TData>>>();
   private subscriptions = new Set<ObservableSubscription>();
 
-  private lastResult: ApolloQueryResult<TData> | undefined;
-  private lastResultSnapshot: ApolloQueryResult<TData> | undefined;
-  private lastError: ApolloError | undefined;
+  private last?: Last<TData, TVariables>;
+
   private queryInfo: QueryInfo;
 
+  // When this.concast is defined, this.observer is the Observer currently
+  // subscribed to that Concast.
   private concast?: Concast<ApolloQueryResult<TData>>;
+  private observer?: Observer<ApolloQueryResult<TData>>;
+
   private pollingInfo?: {
     interval: number;
     timeout: ReturnType<typeof setTimeout>;
@@ -102,10 +111,11 @@ export class ObservableQuery<
       this.observers.add(observer);
 
       // Deliver most recent error or result.
-      if (this.lastError) {
-        observer.error && observer.error(this.lastError);
-      } else if (this.lastResult) {
-        observer.next && observer.next(this.lastResult);
+      const last = this.last;
+      if (last && last.error) {
+        observer.error && observer.error(last.error);
+      } else if (last && last.result) {
+        observer.next && observer.next(last.result);
       }
 
       // Initiate observation of this query if it hasn't been reported to
@@ -177,12 +187,8 @@ export class ObservableQuery<
   }
 
   public getCurrentResult(saveAsLastResult = true): ApolloQueryResult<TData> {
-    const {
-      lastResult,
-      options: {
-        fetchPolicy = "cache-first",
-      },
-    } = this;
+    // Use the last result as long as the variables match this.variables.
+    const lastResult = this.getLastResult(true);
 
     const networkStatus =
       this.queryInfo.networkStatus ||
@@ -202,11 +208,14 @@ export class ObservableQuery<
     if (!this.queryManager.transform(this.options.query).hasForcedResolvers) {
       const diff = this.queryInfo.getDiff();
 
-      result.data = (
-        diff.complete ||
-        this.options.returnPartialData
-      ) ? diff.result : void 0;
+      if (diff.complete || this.options.returnPartialData) {
+        result.data = diff.result;
+      }
+      if (equal(result.data, {})) {
+        result.data = void 0 as any;
+      }
 
+      const { fetchPolicy = "cache-first" } = this.options;
       if (diff.complete) {
         // If the diff is complete, and we're using a FetchPolicy that
         // terminates after a complete cache read, we can assume the next
@@ -247,23 +256,31 @@ export class ObservableQuery<
   // Compares newResult to the snapshot we took of this.lastResult when it was
   // first received.
   public isDifferentFromLastResult(newResult: ApolloQueryResult<TData>) {
-    return !equal(this.lastResultSnapshot, newResult);
+    return !this.last || !equal(this.last.result, newResult);
+  }
+
+  private getLast<K extends keyof Last<TData, TVariables>>(
+    key: K,
+    variablesMustMatch?: boolean,
+  ) {
+    const last = this.last;
+    if (last &&
+        last[key] &&
+        (!variablesMustMatch || equal(last!.variables, this.variables))) {
+      return last[key];
+    }
   }
 
-  // Returns the last result that observer.next was called with. This is not the same as
-  // getCurrentResult! If you're not sure which you need, then you probably need getCurrentResult.
-  public getLastResult(): ApolloQueryResult<TData> | undefined {
-    return this.lastResult;
+  public getLastResult(variablesMustMatch?: boolean): ApolloQueryResult<TData> | undefined {
+    return this.getLast("result", variablesMustMatch);
   }
 
-  public getLastError(): ApolloError | undefined {
-    return this.lastError;
+  public getLastError(variablesMustMatch?: boolean): ApolloError | undefined {
+    return this.getLast("error", variablesMustMatch);
   }
 
   public resetLastResults(): void {
-    delete this.lastResult;
-    delete this.lastResultSnapshot;
-    delete this.lastError;
+    delete this.last;
     this.isTornDown = false;
   }
 
@@ -499,7 +516,6 @@ once, rather than every time you call fetchMore.`);
     const { result } = queryManager.cache.diff<TData>({
       query: this.options.query,
       variables: this.variables,
-      previousResult: this.lastResult?.data,
       returnPartialData: true,
       optimistic: false,
     });
@@ -599,16 +615,21 @@ once, rather than every time you call fetchMore.`);
     poll();
   }
 
-  private updateLastResult(newResult: ApolloQueryResult<TData>) {
-    const previousResult = this.lastResult;
-    this.lastResult = newResult;
-    this.lastResultSnapshot = this.queryManager.assumeImmutableResults
-      ? newResult
-      : cloneDeep(newResult);
+  private updateLastResult(
+    newResult: ApolloQueryResult<TData>,
+    variables = this.variables,
+  ) {
+    this.last = {
+      ...this.last,
+      result: this.queryManager.assumeImmutableResults
+        ? newResult
+        : cloneDeep(newResult),
+      variables,
+    };
     if (!isNonEmptyArray(newResult.errors)) {
-      delete this.lastError;
+      delete this.last.error;
     }
-    return previousResult;
+    return this.last;
   }
 
   public reobserve(
@@ -657,22 +678,32 @@ once, rather than every time you call fetchMore.`);
       }
     }
 
+    const variables = options.variables && { ...options.variables };
     const concast = this.fetch(options, newNetworkStatus);
+    const observer: Observer<ApolloQueryResult<TData>> = {
+      next: result => {
+        this.reportResult(result, variables);
+      },
+      error: error => {
+        this.reportError(error, variables);
+      },
+    };
 
     if (!useDisposableConcast) {
       // We use the {add,remove}Observer methods directly to avoid wrapping
       // observer with an unnecessary SubscriptionObserver object, in part so
       // that we can remove it here without triggering any unsubscriptions,
       // because we just want to ignore the old observable, not prematurely shut
       // it down, since other consumers may be awaiting this.concast.promise.
-      if (this.concast) {
+      if (this.concast && this.observer) {
         this.concast.removeObserver(this.observer, true);
       }
 
       this.concast = concast;
+      this.observer = observer;
     }
 
-    concast.addObserver(this.observer);
+    concast.addObserver(observer);
 
     return concast.promise;
   }
@@ -684,41 +715,51 @@ once, rather than every time you call fetchMore.`);
     // save the fetchMore result as this.lastResult, causing it to be
     // ignored due to the this.isDifferentFromLastResult check in
     // this.observer.next.
-    this.observer.next(this.getCurrentResult(false));
+    this.reportResult(
+      this.getCurrentResult(false),
+      this.variables,
+    );
   }
 
-  private observer = {
-    next: (result: ApolloQueryResult<TData>) => {
-      if (this.lastError || this.isDifferentFromLastResult(result)) {
-        this.updateLastResult(result);
-        iterateObserversSafely(this.observers, 'next', result);
-      }
-    },
+  private reportResult(
+    result: ApolloQueryResult<TData>,
+    variables: TVariables | undefined,
+  ) {
+    if (this.getLastError() || this.isDifferentFromLastResult(result)) {
+      this.updateLastResult(result, variables);
+      iterateObserversSafely(this.observers, 'next', result);
+    }
+  }
 
-    error: (error: ApolloError) => {
-      // Since we don't get the current result on errors, only the error, we
-      // must mirror the updates that occur in QueryStore.markQueryError here
-      this.updateLastResult({
-        ...this.lastResult,
-        error,
-        errors: error.graphQLErrors,
-        networkStatus: NetworkStatus.error,
-        loading: false,
-      } as ApolloQueryResult<TData>);
-
-      iterateObserversSafely(this.observers, 'error', this.lastError = error);
-    },
-  };
+  private reportError(
+    error: ApolloError,
+    variables: TVariables | undefined,
+  ) {
+    // Since we don't get the current result on errors, only the error, we
+    // must mirror the updates that occur in QueryStore.markQueryError here
+    const errorResult = {
+      ...this.getLastResult(),
+      error,
+      errors: error.graphQLErrors,
+      networkStatus: NetworkStatus.error,
+      loading: false,
+    } as ApolloQueryResult<TData>;
+
+    this.updateLastResult(errorResult, variables);
+
+    iterateObserversSafely(this.observers, 'error', this.last!.error = error);
+  }
 
   public hasObservers() {
     return this.observers.size > 0;
   }
 
   private tearDownQuery() {
     if (this.isTornDown) return;
-    if (this.concast) {
+    if (this.concast && this.observer) {
       this.concast.removeObserver(this.observer);
       delete this.concast;
+      delete this.observer;
     }
 
     this.stopPolling();

src/core/__tests__/ObservableQuery.ts

@@ -798,7 +798,7 @@ describe('ObservableQuery', () => {
           expect(result.errors).toEqual([error]);
           expect(observable.getCurrentResult().errors).toEqual([error]);
           observable.setVariables(differentVariables);
-          expect(observable.getCurrentResult().errors).toEqual([error]);
+          expect(observable.getCurrentResult().errors).toBeUndefined();
         } else if (handleCount === 2) {
           expect(result.data).toEqual(dataTwo);
           expect(observable.getCurrentResult().data).toEqual(

src/react/data/QueryData.ts

@@ -323,14 +323,13 @@ export class QueryData<TData, TVariables> extends OperationData<
     // has a chance to stay open).
     const { currentObservable } = this;
     if (currentObservable) {
-      const lastError = currentObservable.getLastError();
-      const lastResult = currentObservable.getLastResult();
-      currentObservable.resetLastResults();
-      this.startQuerySubscription();
-      Object.assign(currentObservable, {
-        lastError,
-        lastResult
-      });
+      const last = currentObservable["last"];
+      try {
+        currentObservable.resetLastResults();
+        this.startQuerySubscription();
+      } finally {
+        currentObservable["last"] = last;
+      }
     }
   }
 

src/react/hooks/__tests__/useQuery.test.tsx

@@ -18,7 +18,6 @@ import { ApolloLink } from '../../../link/core';
 import { itAsync, MockLink, MockedProvider, mockSingleLink } from '../../../testing';
 import { useQuery } from '../useQuery';
 import { useMutation } from '../useMutation';
-import { invariant } from 'ts-invariant';
 
 describe('useQuery Hook', () => {
   describe('General use', () => {
@@ -2438,7 +2437,6 @@ describe('useQuery Hook', () => {
   describe('Missing Fields', () => {
     it('should log debug messages about MissingFieldErrors from the cache', async () => {
       const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
-      const debugSpy = jest.spyOn(invariant, 'debug').mockImplementation(() => {});
 
       const carQuery: DocumentNode = gql`
         query cars($id: Int) {
@@ -2488,15 +2486,8 @@ describe('useQuery Hook', () => {
       expect(result.current.error).toBe(undefined);
       await waitForNextUpdate();
       expect(result.current.loading).toBe(false);
-      expect(result.current.data).toBe(undefined);
-
+      expect(result.current.data).toEqual(carData);
       expect(result.current.error).toBeUndefined();
-      expect(debugSpy).toHaveBeenCalledTimes(1);
-      expect(debugSpy).toHaveBeenLastCalledWith(
-        "Missing cache result fields: cars.0.vin",
-        [new Error("Can't find field 'vin' on Car:1 object")]
-      );
-      debugSpy.mockRestore();
 
       expect(errorSpy).toHaveBeenCalledTimes(1);
       expect(errorSpy).toHaveBeenLastCalledWith(