setVariables() ignores modified variables

[PowerKiKi]

Intended outcome:

Calling ObservableQuery.setVariables() with modified variables should rerun the query.

Actual outcome:

If the variables object is the same as the original, but some of its values are modified, then calling ObservableQuery.setVariables() will have no effect at all, because internally the original variables and new variables will incorrectly be considered equals.

How to reproduce the issue:

Here is a patch to cover this case with unit tests:

diff --git a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
index fca78f3..a5caece 100644
--- a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
+++ b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
@@ -670,6 +670,20 @@ describe('ObservableQuery', () => {
 
   describe('setVariables', () => {
     it('reruns query if the variables change', done => {
+
+      // Creating an entirely different object like that would work
+      // const otherVariables = {id:  3};
+
+      // Modifying existing variables object will not work
+      const modifiedVariables = differentVariables;
+      modifiedVariables.id = 3;
+
+      const modifiedData = {
+          people_one: {
+            name: 'Kylo Ren',
+        },
+      };
+
       const observable: ObservableQuery<any> = mockWatchQuery(
         {
           request: { query, variables },
@@ -679,6 +693,10 @@ describe('ObservableQuery', () => {
           request: { query, variables: differentVariables },
           result: { data: dataTwo },
         },
+        {
+          request: { query, variables: modifiedVariables },
+          result: { data: modifiedData },
+        },
       );
 
       subscribeAndCount(done, observable, (handleCount, result) => {
@@ -691,6 +709,13 @@ describe('ObservableQuery', () => {
         } else if (handleCount === 3) {
           expect(result.loading).toBe(false);
           expect(result.data).toEqual(dataTwo);
+          observable.setVariables(modifiedVariables);
+        } else if (handleCount === 4) {
+          expect(result.loading).toBe(true);
+          expect(result.data).toEqual(dataTwo);
+        } else if (handleCount === 5) {
+          expect(result.loading).toBe(false);
+          expect(result.data).toEqual(modifiedData);
           done();
         }
       });

Version

• apollo-client, master branch, c266eba

Suggested solutions:

Before submitting a PR for that, I would like to discuss if the actual behavior is intended or not. If it is, then it should be better documented, at the very least in the function itself, maybe in the general doc as well.

If the actual behavior is not intended, then I could submit a PR to deepClone variables at the beginning of ObservableQuery.setVariables(). That way we would avoid keeping a reference on the original variables internally, and even if the consumer modifies their variables, it would not affect our internal variables. Then later on when the consumer call setVariables(), the original variables will correctly not be equals to the new variables. That seems to be a much more intuitive behavior. The only downside I can see is that it might have a small performance impact because it means we would always need to deep compare variables (via the recursive isEqual()), instead of relying on fast identity checks (===).

[PowerKiKi]

@stubailo, you previously said "mutating the variables is not the right approach long term". I would tend to disagree with you and say that mutating variables is a very natural, and convenient, way to work, independently of what that may means for apollo-client's internals.

Would you have any thoughts about this new issue ? Should it be solved via documentation or via code ?

[PowerKiKi]

Coming back to this issue I now realized I made a mistake. The test was incorrectly written and actually reproduced the "cache hit not being notified" as described in #2712.

A proper test, showing that it actually works as intended, would be the following:

diff --git a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
index d8e5533..be9eb94 100644
--- a/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
+++ b/packages/apollo-client/src/core/__tests__/ObservableQuery.ts
@@ -670,6 +670,12 @@ describe('ObservableQuery', () => {
 
   describe('setVariables', () => {
     it('reruns query if the variables change', done => {
+      const modifiedData = {
+        people_one: {
+          name: 'Kylo Ren',
+        },
+      };
+
       const observable: ObservableQuery<any> = mockWatchQuery(
         {
           request: { query, variables },
@@ -679,6 +685,10 @@ describe('ObservableQuery', () => {
           request: { query, variables: differentVariables },
           result: { data: dataTwo },
         },
+        {
+          request: { query, variables: { id: 3 } },
+          result: { data: modifiedData },
+        },
       );
 
       subscribeAndCount(done, observable, (handleCount, result) => {
@@ -691,6 +701,16 @@ describe('ObservableQuery', () => {
         } else if (handleCount === 3) {
           expect(result.loading).toBe(false);
           expect(result.data).toEqual(dataTwo);
+
+          // Modifying existing variables object will indeed work
+          differentVariables.id = 3;
+          observable.setVariables(differentVariables);
+        } else if (handleCount === 4) {
+          expect(result.loading).toBe(true);
+          expect(result.data).toEqual(dataTwo);
+        } else if (handleCount === 5) {
+          expect(result.loading).toBe(false);
+          expect(result.data).toEqual(modifiedData);
           done();
         }
       });

So this can be closed as invalid.