Add test for Query bug which is caused by changing props (#1749) (#2718)

* Add test for Query bug which is caused by changing props (#1749)

* Skip the failing test (#1749)

* Enable failing test

* Leverage lastResult to limit unnecessary subscription re-creation

When the Observable subscription created in `startQuerySubcription`
receives an error, we only want to remove the old subscription,
and start a new subscription, when we don't have a
previous result stored. So if no previous result was received due to
problems fetching data, or the previous result has been forcefully
cleared out for some reason, we shoule re-create a new
subscription. Otherwise we might be unnecessarily starting a new
subscription the `Query` component isn't expecting, and isn't tying
into its update cycle properly.

* Changelog update

Author: MerzDaniel

Changelog.md

@@ -7,6 +7,10 @@
 - Fix an infinite loop caused by using `setState` in the
   `onError` / `onCompleted` callbacks of the `Query` component.  <br/>
   [@chenesan](https://github.com/chenesan) in [#2751](https://github.com/apollographql/react-apollo/pull/2751)
+- Fixed an issue that prevented good results from showing up in a `Query`
+  component, after an error was received, variables were adjusted, and then
+  the good data was fetched.  <br/>
+  [@MerzDaniel](https://github.com/MerzDaniel) in [#2718](https://github.com/apollographql/react-apollo/pull/2718)
 
 ### Improvements
 

src/Query.tsx

@@ -345,10 +345,15 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
         this.updateCurrentData();
       },
       error: error => {
-        this.resubscribeToQuery();
-        // Quick fix for https://github.com/apollostack/react-apollo/issues/378
+        if (!this.lastResult) {
+          // We only want to remove the old subscription, and start a new
+          // subscription, when an error was received and we don't have a
+          // previous result stored. This means either no previous result was
+          // received due to problems fetching data, or the previous result
+          // has been forcefully cleared out.
+          this.resubscribeToQuery();
+        }
         if (!error.hasOwnProperty('graphQLErrors')) throw error;
-
         this.updateCurrentData();
       },
     });
@@ -365,13 +370,15 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
   private resubscribeToQuery() {
     this.removeQuerySubscription();
 
+    // Unfortunately, if `lastError` is set in the current
+    // `queryObservable` when the subscription is re-created,
+    // the subscription will immediately receive the error, which will
+    // cause it to terminate again. To avoid this, we first clear
+    // the last error/result from the `queryObservable` before re-starting
+    // the subscription, and restore it afterwards (so the subscription
+    // has a chance to stay open).
     const lastError = this.queryObservable!.getLastError();
-    const lastResult = this.lastResult;
-
-    // If lastError is set, the observable will immediately
-    // send it, causing the stream to terminate on initialization.
-    // We clear everything here and restore it afterward to
-    // make sure the new subscription sticks.
+    const lastResult = this.queryObservable!.getLastResult();
     this.queryObservable!.resetLastResults();
     this.startQuerySubscription();
     Object.assign(this.queryObservable!, { lastError, lastResult });

test/client/Query.test.tsx

@@ -1387,6 +1387,107 @@ describe('Query component', () => {
     );
   });
 
+  it(
+    'should not persist previous result errors when a subsequent valid ' +
+    'result is received',
+    done => {
+      const query: DocumentNode = gql`
+        query somethingelse ($variable: Boolean) {
+          allPeople(first: 1, yetisArePeople: $variable) {
+            people {
+            name
+          }
+        }
+      }`;
+
+      const data = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+      const variableGood = { variable: true }
+      const variableBad = { variable: false }
+
+      const link = mockSingleLink(
+        {
+          request: {
+            query,
+            variables: variableGood,
+          },
+          result: {
+            data,
+          },
+        },
+        {
+          request: {
+            query,
+            variables: variableBad,
+          },
+          result: {
+            errors: [new Error('This is an error!')],
+          },
+        },
+        {
+          request: {
+            query,
+            variables: variableGood,
+          },
+          result: {
+            data,
+          },
+        },
+      );
+
+      const client = new ApolloClient({
+        link,
+        cache: new Cache({ addTypename: false }),
+      });
+
+      let count = 0;
+      const DummyComp = (props: any) => {
+        if (!props.loading) {
+          try {
+            switch (count++) {
+              case 0:
+                expect(props.data.allPeople).toBeTruthy();
+                expect(props.error).toBeFalsy();
+                // Change query variables to trigger bad result.
+                setTimeout(() => {
+                  wrapper!.setProps({ variables: variableBad });
+                }, 0);
+                break;
+              case 1:
+                // Error should be received, but last known good value
+                // should still be accessible (in-case the UI needs it).
+                expect(props.error).toBeTruthy();
+                expect(props.data.allPeople).toBeTruthy();
+                // Change query variables to trigger a good result.
+                setTimeout(() => {
+                  wrapper!.setProps({ variables: variableGood });
+                }, 0);
+                break
+              case 2:
+                // Good result should be received without any errors.
+                expect(props.error).toBeFalsy();
+                expect(props.data.allPeople).toBeTruthy();
+                done();
+                break;
+              default:
+                done.fail('Unknown count');
+            }
+          } catch (error) {
+            done.fail(error);
+          }
+        }
+        return null;
+      }
+
+      wrapper = mount(
+        <Query client={client} query={query} variables={variableGood}>
+          {(result: any) => {
+            return <DummyComp id='dummyId' {...result} />;
+          }}
+        </Query>
+      );
+    }
+  );
+
   it('should not repeatedly call onCompleted if setState in it', done => {
     const query = gql`
       query people($first: Int) {