recycled query teardown (#740)

* increase setTimeout for intermittent test failre

* remove query before creating new observable

* add note about order until I can think how to test it

* don't create new observable during recyle

* Update package.json

Changelog.md

@@ -6,7 +6,7 @@
 #### BREAKING FOR TYPESCRIPT USERS
 - Feature: Enhanced typescript definitions to allow for more valid type checking of graphql HOC [PR #695](https://github.com/apollographql/react-apollo/pull/695)
 - Feature: Flow types: [PR #695](https://github.com/apollographql/react-apollo/pull/695)
-
+- Fix: Fix bug with sync re-renders and recyled queries [PR #740](https://github.com/apollographql/react-apollo/pull/740)
 
 ### 1.3.0
 - Feature: Support tree shaking and smaller (marginally) bundles via rollup [PR #691](https://github.com/apollographql/react-apollo/pull/691)

package.json

@@ -135,7 +135,7 @@
     "uglify-js": "^3.0.13"
   },
   "dependencies": {
-    "apollo-client": "^1.2.2",
+    "apollo-client": "^1.4.0",
     "graphql-anywhere": "^3.0.0",
     "graphql-tag": "^2.0.0",
     "hoist-non-react-statics": "^1.2.0",

src/graphql.tsx

@@ -268,8 +268,13 @@ export default function graphql<TResult = {}, TProps = {}, TChildProps = Default
             delete this.queryObservable;
           }
 
+          // It is critical that this happens prior to recyling the query
+          // if not it breaks the loading state / network status because
+          // an orphan observer is created in AC (intended) which is cleaned up
+          // when the browser has time via a setTimeout(0)
           // Unsubscribe from our query subscription.
           this.unsubscribeFromQuery();
+
         }
 
         if (this.type === DocumentType.Subscription) this.unsubscribeFromQuery();
@@ -609,6 +614,7 @@ class ObservableQueryRecycler {
     observableQuery.setOptions({
       fetchPolicy: 'standby',
       pollInterval: 0,
+      fetchResults: false, // ensure we don't create another observer in AC
     });
 
     this.observableQueries.push({

test/react-web/client/graphql/queries/observableQuery.test.tsx

@@ -34,7 +34,7 @@ function wait(ms) {
 describe('[queries] observableQuery', () => {
 
   // observableQuery
-  it('will recycle `ObservableQuery`s when re-rendering the entire tree', () => {
+  it('will recycle `ObservableQuery`s when re-rendering the entire tree', (done) => {
     const query = gql`query people { allPeople(first: 1) { people { name } } }`;
     const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
     const networkInterface = mockNetworkInterface(
@@ -43,48 +43,115 @@ describe('[queries] observableQuery', () => {
     );
     const client = new ApolloClient({ networkInterface, addTypename: false });
 
-    @graphql(query)
+    // storage
+    let queryObservable1: ObservableQuery<any>;
+    let queryObservable2: ObservableQuery<any>;
+    let originalOptions;
+    let wrapper1;
+    let wrapper2;
+    let count = 0;
+    let recycledOptions;
+
+    const assert1 = () => {
+      expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
+      queryObservable1 = (client as any).queryManager.observableQueries['1'].observableQuery;
+      originalOptions = Object.assign({}, queryObservable1.options);
+    }
+
+    const assert2 = () => {
+      expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
+    }
+
+    @graphql(query, { options: { fetchPolicy: 'cache-and-network' }})
     class Container extends React.Component<any, any> {
+      componentWillMount() {
+
+        // during the first mount, the loading prop should be true;
+        if (count === 0) {
+          expect(this.props.data.loading).toBe(true);
+        }
+
+        // during the second mount, the loading prop should be false, and data should
+        // be present;
+        if (count === 3) {
+          expect(this.props.data.loading).toBe(false);
+          expect(this.props.data.allPeople).toEqual(data.allPeople);
+        }
+      }
+
+      componentDidMount(){
+        if (count === 4) {
+          wrapper1.unmount();
+          done();
+        }
+      }
+
+      componentDidUpdate(prevProps) {
+        if (count === 3) {
+          expect(prevProps.data.loading).toBe(true);
+          expect(this.props.data.loading).toBe(false);
+          expect(this.props.data.allPeople).toEqual(data.allPeople);
+
+          // ensure first assertion and umount tree
+          assert1();
+          wrapper1.find("#break").simulate("click");
+
+          // ensure cleanup
+          assert2();
+       }
+      }
+
       render () {
+        // side effect to keep track of render counts
+        count++;
         return null;
       }
     }
 
-    const wrapper1 = renderer.create(
-      <ApolloProvider client={client}>
-        <Container/>
-      </ApolloProvider>
-    );
+    class RedirectOnMount extends React.Component<any, any> {
+      componentWillMount() {
+        this.props.onMount();
+      }
 
-    expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
-    const queryObservable1: ObservableQuery<any> = (client as any).queryManager.observableQueries['1'].observableQuery;
+      render() {
+        return null;
+      }
+    }
 
-    const originalOptions = Object.assign({}, queryObservable1.options);
+    class AppWrapper extends React.Component<any, any> {
+      state = {
+        renderRedirect: false,
+      };
 
-    wrapper1.unmount();
+      goToRedirect = () => {
+        this.setState({ renderRedirect: true });
+      };
 
-    expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
+      handleRedirectMount = () => {
+        this.setState({ renderRedirect: false });
+      };
 
-    const wrapper2 = renderer.create(
+      render() {
+        if (this.state.renderRedirect) {
+          return <RedirectOnMount onMount={this.handleRedirectMount} />;
+        } else {
+          return (
+            <div>
+              <Container />
+              <button id="break" onClick={this.goToRedirect}>Break things</button>
+            </div>
+          );
+        }
+      }
+    }
+
+    wrapper1 = mount(
       <ApolloProvider client={client}>
-        <Container/>
+        <AppWrapper/>
       </ApolloProvider>
     );
 
-    expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
-    const queryObservable2: ObservableQuery<any> = (client as any).queryManager.observableQueries['1'].observableQuery;
-
-    const recycledOptions = queryObservable2.options;
-
-    expect(queryObservable1).toBe(queryObservable2);
-    expect(recycledOptions.query).toEqual(originalOptions.query);
-    expect(recycledOptions.metadata).toEqual(originalOptions.metadata);
-    expect(recycledOptions.notifyOnNetworkStatusChange).toEqual(originalOptions.notifyOnNetworkStatusChange);
-
-    wrapper2.unmount();
-
-    expect(Object.keys((client as any).queryManager.observableQueries)).toEqual(['1']);
-  });
+ });
 
   it('will not try to refetch recycled `ObservableQuery`s when resetting the client store', (done) => {
     const query = gql`query people { allPeople(first: 1) { people { name } } }`;