Restore variables replacement behavior of `ObservableQuery#reobserv…

…e` method (#9741) Should fix issues #9671 and #9736.
apollographql · May 19, 2022 · f6ab338 · f6ab338
1 parent 964ca12
commit f6ab338
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 5 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -2,6 +2,9 @@
 
 ### Bug Fixes
 
+- Restore pre-v3.6 `variables` replacement behavior of `ObservableQuery#reobserve` method, fixing a regression that prevented removal of variables. <br/>
+  [@benjamn](https://github.com/benjamn) in [#9741](https://github.com/apollographql/apollo-client/pull/9741)
+
 - Preserve `previousData` even when different query or client provided to `useQuery`, instead of resetting `previousData` to undefined in those cases, matching behavior prior to v3.6.0. <br/>
   [@benjamn](https://github.com/benjamn) in [#9734](https://github.com/apollographql/apollo-client/pull/9734)
 

diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts
@@ -6,6 +6,7 @@ import { NetworkStatus, isNetworkRequestInFlight } from './networkStatus';
 import {
   Concast,
   cloneDeep,
+  compact,
   getOperationDefinition,
   Observable,
   Observer,
@@ -14,7 +15,6 @@ import {
   isNonEmptyArray,
   fixObservableSubclass,
   getQueryDefinition,
-  mergeOptions,
 } from '../utilities';
 import { ApolloError } from '../errors';
 import { QueryManager } from './QueryManager';
@@ -792,7 +792,7 @@ Did you mean to call refetch(variables) instead of refetch({ variables })?`);
     const oldVariables = this.options.variables;
     const oldFetchPolicy = this.options.fetchPolicy;
 
-    const mergedOptions = mergeOptions(this.options, newOptions || {});
+    const mergedOptions = compact(this.options, newOptions || {});
     const options = useDisposableConcast
       // Disposable Concast fetches receive a shallow copy of this.options
       // (merged with newOptions), leaving this.options unmodified.

diff --git a/src/react/hooks/__tests__/useQuery.test.tsx b/src/react/hooks/__tests__/useQuery.test.tsx
@@ -935,7 +935,7 @@ describe('useQuery Hook', () => {
               variables: {
                 sourceOfVar: "local",
                 isGlobal: false,
-              },
+              } as OperationVariables,
             },
             variables: {
               mandatory: true,
@@ -991,10 +991,14 @@ describe('useQuery Hook', () => {
         "cache-and-network",
       ]);
 
-      const reobservePromise = result.current.observable.reobserve({
+      const reobservePromise = act(() => result.current.observable.reobserve({
         fetchPolicy: "network-only",
         nextFetchPolicy: "cache-first",
         variables: {
+          // Since reobserve replaces the variables object rather than merging
+          // the individual variables together, we need to include the current
+          // variables manually if we want them to show up in the output below.
+          ...result.current.observable.variables,
           sourceOfVar: "reobserve",
         },
       }).then(finalResult => {
@@ -1006,7 +1010,7 @@ describe('useQuery Hook', () => {
             mandatory: true,
           },
         });
-      });
+      }));
 
       expect(
         result.current.observable.options.fetchPolicy
@@ -1043,6 +1047,60 @@ describe('useQuery Hook', () => {
         "cache-and-network",
         "cache-first",
       ]);
+
+      const reobserveNoVarMergePromise = act(() => result.current.observable.reobserve({
+        fetchPolicy: "network-only",
+        nextFetchPolicy: "cache-first",
+        variables: {
+          // This reobservation is like the one above, with no variable merging.
+          // ...result.current.observable.variables,
+          sourceOfVar: "reobserve without variable merge",
+        },
+      }).then(finalResult => {
+        expect(finalResult.loading).toBe(false);
+        expect(finalResult.data).toEqual({
+          vars: {
+            sourceOfVar: "reobserve without variable merge",
+            // Since we didn't merge in result.current.observable.variables, we
+            // don't see these variables anymore:
+            // isGlobal: false,
+            // mandatory: true,
+          },
+        });
+      }));
+
+      expect(
+        result.current.observable.options.fetchPolicy
+      ).toBe("network-only");
+
+      expect(result.current.observable.variables).toEqual({
+        sourceOfVar: "reobserve without variable merge",
+      });
+
+      await reobserveNoVarMergePromise;
+
+      expect(
+        result.current.observable.options.fetchPolicy
+      ).toBe("cache-first");
+
+      expect(result.current.loading).toBe(false);
+      expect(result.current.data).toEqual({
+        vars: {
+          sourceOfVar: "reobserve without variable merge",
+        },
+      });
+      expect(
+        result.current.observable.variables
+      ).toEqual(
+        result.current.data!.vars
+      );
+
+      expect(fetchPolicyLog).toEqual([
+        "cache-and-network",
+        "cache-and-network",
+        "cache-first",
+        "cache-first",
+      ]);
     });
 
     it("defaultOptions do not confuse useQuery when unskipping a query (issue #9635)", async () => {