Fix ObservableQuery result dirty check with last.reportedResult

apollographql · Jul 1, 2022 · b2e4eb0 · b2e4eb0
1 parent 0475025
commit b2e4eb0
Show file tree

Hide file tree

Showing 2 changed files with 63 additions and 6 deletions.
diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
@@ -59,6 +59,7 @@ interface Last<TData, TVariables> {
   result: ApolloQueryResult<TData>;
   variables?: TVariables;
   error?: ApolloError;
+  reportedResult: ApolloQueryResult<TData>
 }
 
 export class ObservableQuery<
@@ -303,8 +304,8 @@ export class ObservableQuery<
 
   // Compares newResult to the snapshot we took of this.lastResult when it was
   // first received.
-  public isDifferentFromLastResult(newResult: ApolloQueryResult<TData>) {
-    return !this.last || !equal(this.last.result, newResult);
+  public isDifferentFromLastReportedResult(newResult: ApolloQueryResult<TData>) {
+    return !this.last || !equal(this.last.reportedResult, newResult);
   }
 
   private getLast<K extends keyof Last<TData, TVariables>>(
@@ -758,11 +759,13 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
     newResult: ApolloQueryResult<TData>,
     variables = this.variables,
   ) {
+    const immutableResult = this.queryManager.assumeImmutableResults
+      ? newResult
+      : cloneDeep(newResult);
     this.last = {
       ...this.last,
-      result: this.queryManager.assumeImmutableResults
-        ? newResult
-        : cloneDeep(newResult),
+      result: immutableResult,
+      reportedResult: immutableResult,
       variables,
     };
     if (!isNonEmptyArray(newResult.errors)) {
@@ -771,6 +774,19 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
     return this.last;
   }
 
+  private updateLastReportedResultOnly(
+    newResult: ApolloQueryResult<TData>
+  ) {
+    if (!this.last) return;
+
+    this.last = {
+      ...this.last,
+      reportedResult: this.queryManager.assumeImmutableResults
+        ? newResult
+        : cloneDeep(newResult)
+    };
+  }
+
   public reobserve(
     newOptions?: Partial<WatchQueryOptions<TVariables, TData>>,
     newNetworkStatus?: NetworkStatus,
@@ -871,9 +887,11 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
     variables: TVariables | undefined,
   ) {
     const lastError = this.getLastError();
-    if (lastError || this.isDifferentFromLastResult(result)) {
+    if (lastError || this.isDifferentFromLastReportedResult(result)) {
       if (lastError || !result.partial || this.options.returnPartialData) {
         this.updateLastResult(result, variables);
+      } else {
+        this.updateLastReportedResultOnly(result);
       }
 
       iterateObserversSafely(this.observers, 'next', result);

diff --git a/src/core/__tests__/ObservableQuery.ts b/src/core/__tests__/ObservableQuery.ts
@@ -2443,4 +2443,43 @@ describe('ObservableQuery', () => {
       error: reject,
     });
   });
+
+  itAsync("ObservableQuery uses last.reportedResult for dirty checks, not last.result", (resolve, reject) => {
+    const manager = mockQueryManager(
+      reject,
+      {
+        request: { query, variables },
+        result: { data: dataOne },
+      },
+      {
+        request: { query, variables: differentVariables },
+        result: { data: dataOne },
+      },
+    );
+
+    const observable = manager.watchQuery({
+      query,
+      variables,
+      notifyOnNetworkStatusChange: true,
+    });
+
+    subscribeAndCount(reject, observable, async (handleCount, result) => {
+      if (handleCount === 1) {
+        // Should be saved as last.reportedResult and last.result
+        expect(result.data).toEqual(dataOne);
+        expect(result.loading).toBe(false);
+        await observable.setVariables(differentVariables);
+      } else if (handleCount === 2) {
+        // Should be saved as last.reportedResult but not as last.result
+        expect(result.loading).toBe(true);
+        expect(result.networkStatus).toBe(NetworkStatus.setVariables);
+      } else if (handleCount === 3) {
+        // Should compare this result against last.reportedResult (loading=true) and send a notification
+        // Otherwise the subscriber would be stuck with loading=true forever
+        expect(result.data).toEqual(dataOne);
+        expect(result.loading).toBe(false);
+        resolve();
+      }
+    });
+  });
 });