Fix bugs around unsubscribe and skipping

See #255.

@glasser and I found a few bugs around the (old and new) skip options, and wrote some tests to confirm the fixes and stop regressions

- didn't delete querySubscription in .unsubscribeFromQuery() 
  - this mean if you skipped and then unskipped, it would break

- not checking for querySubscription before .unsubscribe on unmounting
  - if you skip then unmount, you have problem

- check for non-skip -> skip in willReceiveProps didn't return in the remain skipping case
  - mean "non skipping" code ran when remaining skipping.

Changelog.md

@@ -4,6 +4,8 @@ Expect active development and potentially significant breaking changes in the `0
 
 ### vNext
 
+- Bug: Fix possible crash in unsubscribeFromQuery [#260](https://github.com/apollostack/react-apollo/pull/260)
+
 ### v0.5.8
 
 - Feature: Remove nested imports for apollo-client. Making local development eaiser. [#234](https://github.com/apollostack/react-apollo/pull/234)

src/graphql.tsx

@@ -267,10 +267,13 @@ export default function graphql(
       }
 
       componentWillReceiveProps(nextProps) {
-        // if this has changed, remove data and unsubscribeFromQuery
-        if (!mapPropsToSkip(this.props) && mapPropsToSkip(nextProps)) {
-          delete this.data;
-          return this.unsubscribeFromQuery();
+        if (mapPropsToSkip(nextProps)) {
+          if (!mapPropsToSkip(this.props)) {
+            // if this has changed, remove data and unsubscribeFromQuery
+            this.data = {};
+            this.unsubscribeFromQuery();
+          }
+          return;
         }
         if (shallowEqual(this.props, nextProps)) return;
 
@@ -354,7 +357,7 @@ export default function graphql(
           if (this.querySubscription) {
             this.hasOperationDataChanged = true;
             this.data = { loading: false };
-            this.querySubscription.unsubscribe();
+            this.unsubscribeFromQuery();
             this.forceRenderChildren();
           }
           return;
@@ -416,9 +419,10 @@ export default function graphql(
       }
 
       unsubscribeFromQuery() {
-        if ((this.querySubscription as Subscription).unsubscribe) {
+        if (this.querySubscription) {
           (this.querySubscription as Subscription).unsubscribe();
           delete this.queryObservable;
+          delete this.querySubscription;
         }
       }
 

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

@@ -370,6 +370,44 @@ describe('queries', () => {
     }, 25);
   });
 
+  it('continues to not subscribe to a skipped query when props change', (done) => {
+    const query = gql`query people { allPeople(first: 1) { people { name } } }`;
+    const networkInterface = mockNetworkInterface();
+    const oldQuery = networkInterface.query;
+
+    networkInterface.query = (request) => {
+      console.log('failing')
+      fail(new Error('query ran even though skip present'));
+      return oldQuery(request);
+    };
+    const client = new ApolloClient({ networkInterface });
+
+    @graphql(query, { skip: true })
+    class Container extends React.Component<any, any> {8
+      componentWillReceiveProps(props) {
+        done();
+      }
+      render() {
+        return null;
+      }
+    };
+
+    class Parent extends React.Component<any, any> {
+      constructor() {
+        super();
+        this.state = { foo: 42 };
+      }
+      componentDidMount() {
+        this.setState({ foo: 43 });
+      }
+      render() {
+        return <Container foo={this.state.foo} />;
+      }
+    };
+
+    renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
+  });
+
   it('doesn\'t run options or props when skipped', (done) => {
     const query = gql`query people { allPeople(first: 1) { people { name } } }`;
     const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
@@ -426,6 +464,87 @@ describe('queries', () => {
     }, 25);
   });
 
+  // test the case of skip:false -> skip:true -> skip:false to make sure things
+  // are cleaned up properly
+  it('allows you to skip then unskip a query with top-level syntax', (done) => {
+    const query = gql`query people { allPeople(first: 1) { people { name } } }`;
+    const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
+    const networkInterface = mockNetworkInterface({ request: { query }, result: { data } });
+    const client = new ApolloClient({ networkInterface });
+
+    let hasSkipped = false;
+    @graphql(query, { skip: ({ skip }) => skip })
+    class Container extends React.Component<any, any> {8
+      componentWillReceiveProps(newProps) {
+        if (newProps.skip) {
+          hasSkipped = true;
+          this.props.setSkip(false);
+        } else {
+          if (hasSkipped) {
+            done();
+          } else {
+            this.props.setSkip(true);
+          }
+        }
+      }
+      render() {
+        return null;
+      }
+    };
+
+    class Parent extends React.Component<any, any> {
+      constructor() {
+        super();
+        this.state = { skip: false };
+      }
+      render() {
+        return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
+      }
+    };
+
+    renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
+  });
+
+  it('allows you to skip then unskip a query with opts syntax', (done) => {
+    const query = gql`query people { allPeople(first: 1) { people { name } } }`;
+    const data = { allPeople: { people: [ { name: 'Luke Skywalker' } ] } };
+    const networkInterface = mockNetworkInterface({ request: { query }, result: { data } });
+    const client = new ApolloClient({ networkInterface });
+
+    let hasSkipped = false;
+    @graphql(query, { options: ({ skip }) => ({ skip}) })
+    class Container extends React.Component<any, any> {8
+      componentWillReceiveProps(newProps) {
+        if (newProps.skip) {
+          hasSkipped = true;
+          this.props.setSkip(false);
+        } else {
+          if (hasSkipped) {
+            done();
+          } else {
+            this.props.setSkip(true);
+          }
+        }
+      }
+      render() {
+        return null;
+      }
+    };
+
+    class Parent extends React.Component<any, any> {
+      constructor() {
+        super();
+        this.state = { skip: false };
+      }
+      render() {
+        return <Container skip={this.state.skip} setSkip={(skip) => this.setState({ skip })} />;
+      }
+    };
+
+    renderer.create(<ProviderMock client={client}><Parent /></ProviderMock>);
+  });
+
+
   it('removes the injected props if skip becomes true', (done) => {
     let count = 0;
     const query = gql`
@@ -495,6 +614,44 @@ describe('queries', () => {
     renderer.create(<ProviderMock client={client}><ChangingProps /></ProviderMock>);
   });
 
+
+  fit('allows you to unmount a skipped query', (done) => {
+    const query = gql`query people { allPeople(first: 1) { people { name } } }`;
+    const networkInterface = mockNetworkInterface();
+    const client = new ApolloClient({ networkInterface });
+
+    @graphql(query, {
+      skip: true,
+    })
+    class Container extends React.Component<any, any> {
+      componentDidMount() {
+        this.props.hide();
+      }
+      componentWillUnmount() {
+        done();
+      }
+      render() {
+        return null;
+      }
+    };
+
+    class Hider extends React.Component<any, any> {
+      constructor() {
+        super();
+        this.state = { hide: false };
+      }
+      render() {
+        if (this.state.hide) {
+          return null;
+        }
+        return <Container hide={() => this.setState({ hide: true })} />;
+      }
+    }
+
+    renderer.create(<ProviderMock client={client}><Hider /></ProviderMock>);
+  });
+
+
   it('reruns the query if it changes', (done) => {
     let count = 0;
     const query = gql`