Merge branch 'master' into hwillson/full-dep-updates

Changelog.md

@@ -7,6 +7,14 @@
 - 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)
+- Fixed an issue that prevented `Query` component updates from firing (under
+  certain circumstances) due to the internal `lastResult` value (that's used
+  to help prevent unnecessary re-renders) not being updated.  <br/>
+  [@Glennrs](https://github.com/Glennrs) in [#2840](https://github.com/apollographql/react-apollo/pull/2840)
 
 ### Improvements
 

examples/rollup/package.json

@@ -15,7 +15,7 @@
     "rollup-plugin-node-resolve": "4.0.1",
     "rollup-plugin-replace": "2.1.0",
     "rollup-plugin-terser": "4.0.4",
-    "source-map-explorer": "1.7.0"
+    "source-map-explorer": "1.8.0"
   },
   "dependencies": {
     "apollo-cache-inmemory": "^1.5.1",

examples/typescript/package.json

@@ -13,8 +13,8 @@
   },
   "devDependencies": {
     "@types/graphql": "14.0.7",
-    "@types/jest": "24.0.9",
-    "@types/node": "11.10.5",
+    "@types/jest": "24.0.11",
+    "@types/node": "11.11.3",
     "@types/prop-types": "15.7.0",
     "@types/react": "16.4.18",
     "@types/react-dom": "16.8.0",

src/Query.tsx

@@ -342,13 +342,21 @@ export default class Query<TData = any, TVariables = OperationVariables> extends
         }
 
         initial = undefined;
+        if (this.lastResult) {
+          this.lastResult = this.queryObservable!.getLastResult();
+        }
         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 +373,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

@@ -1232,6 +1232,115 @@ describe('Query component', () => {
         </MockedProvider>,
       );
     });
+
+    it(
+      'should update if a manual `refetch` is triggered after a state change',
+      done => {
+        const query: DocumentNode = gql`
+          query {
+            allPeople {
+              people {
+                name
+              }
+            }
+          }
+        `;
+
+        const data1 = { allPeople: { people: [{ name: 'Luke Skywalker' }] } };
+
+        const link = mockSingleLink(
+          {
+            request: { query },
+            result: { data: data1 },
+          },
+          {
+            request: { query },
+            result: { data: data1 },
+          },
+          {
+            request: { query },
+            result: { data: data1 },
+          },
+        );
+
+        const client = new ApolloClient({
+          link,
+          cache: new Cache({ addTypename: false }),
+        });
+
+        let count = 0;
+
+        class SomeComponent extends React.Component {
+          constructor(props: any) {
+            super(props);
+            this.state = {
+              open: false,
+            };
+            this.toggle = this.toggle.bind(this);
+          }
+
+          toggle() {
+            this.setState((prevState: any) => ({
+              open: !prevState.open,
+            }));
+          }
+
+          render() {
+            const { open } = this.state as any;
+            return (
+              <Query client={client} query={query} notifyOnNetworkStatusChange>
+                {(props: any) => {
+                  try {
+                    switch (count) {
+                      case 0:
+                        // Loading first response
+                        expect(props.loading).toBe(true);
+                        expect(open).toBe(false);
+                        break;
+                      case 1:
+                        // First response loaded, change state value
+                        expect(stripSymbols(props.data)).toEqual(data1);
+                        expect(open).toBe(false);
+                        setTimeout(() => {
+                          this.toggle();
+                        }, 0);
+                        break;
+                      case 2:
+                        // State value changed, fire a refetch
+                        expect(open).toBe(true);
+                        setTimeout(() => {
+                          props.refetch();
+                        }, 0);
+                        break;
+                      case 3:
+                        // Second response received, fire another refetch
+                        expect(stripSymbols(props.data)).toEqual(data1);
+                        setTimeout(() => {
+                          props.refetch();
+                        }, 0);
+                        break;
+                      case 4:
+                        // Third response received
+                        expect(stripSymbols(props.data)).toEqual(data1);
+                        done();
+                        break;
+                      default:
+                        done.fail('Unknown count');
+                    }
+                    count += 1;
+                  } catch (error) {
+                    done.fail(error);
+                  }
+                  return null;
+                }}
+              </Query>
+            );
+          }
+        }
+
+        wrapper = mount(<SomeComponent />);
+      }
+    );
   });
 
   it('should error if the query changes type to a subscription', done => {
@@ -1387,6 +1496,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) {