Merge pull request #1054 from apollostack/fix-reducers

Fix reducers

CHANGELOG.md

@@ -3,6 +3,9 @@
 Expect active development and potentially significant breaking changes in the `0.x` track. We'll try to be diligent about releasing a `1.0` version in a timely fashion (ideally within 3 to 6 months), to signal the start of a more stable API.
 
 ### vNEXT
+
+- ...
+- Fix bug that caused updateQuery and reducers to run on stopped queries [PR #1054](https://github.com/apollostack/apollo-client/pull/1054)
 - Ensure transporters are using `isomorphic-fetch` instead of `whatwg-fetch` for universal compatibility [PR #1018](https://github.com/apollostack/apollo-client/pull/1018)
 
 ### 0.5.21

src/core/ObservableQuery.ts

@@ -366,6 +366,11 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
 
     const retQuerySubscription = {
       unsubscribe: () => {
+        if (this.observers.findIndex(el => el === observer) < 0 ) {
+          // XXX can't unsubscribe if you've already unsubscribed...
+          // for some reason unsubscribe gets called multiple times by some of the tests
+          return;
+        }
         this.observers = this.observers.filter((obs) => obs !== observer);
 
         if (this.observers.length === 0) {
@@ -435,6 +440,9 @@ export class ObservableQuery extends Observable<ApolloQueryResult> {
     this.subscriptionHandles = [];
 
     this.queryManager.stopQuery(this.queryId);
+    if (this.shouldSubscribe) {
+      this.queryManager.removeObservableQuery(this.queryId);
+    }
     this.observers = [];
   }
 }

src/core/QueryManager.ts

@@ -150,7 +150,7 @@ export class QueryManager {
   private queryListeners: { [queryId: string]: QueryListener[] };
   private queryDocuments: { [queryId: string]: Document };
 
-  private idCounter = 0;
+  private idCounter = 1; // XXX let's not start at zero to avoid pain with bad checks
 
   // A map going from a requestId to a promise that has not yet been resolved. We use this to keep
   // track of queries that are inflight and reject them in case some
@@ -271,19 +271,6 @@ export class QueryManager {
 
     this.queryDocuments[mutationId] = mutation;
 
-    const extraReducers = Object.keys(this.observableQueries).map( queryId => {
-      const queryOptions = this.observableQueries[queryId].observableQuery.options;
-      if (queryOptions.reducer) {
-        return createStoreReducer(
-          queryOptions.reducer,
-          queryOptions.query,
-          queryOptions.variables,
-          this.reducerConfig,
-          );
-      }
-      return null;
-    }).filter( reducer => reducer !== null );
-
     this.store.dispatch({
       type: 'APOLLO_MUTATION_INIT',
       mutationString,
@@ -293,7 +280,7 @@ export class QueryManager {
       mutationId,
       optimisticResponse,
       resultBehaviors: [...resultBehaviors, ...updateQueriesResultBehaviors],
-      extraReducers,
+      extraReducers: this.getExtraReducers(),
     });
 
     return new Promise((resolve, reject) => {
@@ -315,7 +302,7 @@ export class QueryManager {
                 ...resultBehaviors,
                 ...this.collectResultBehaviorsFromUpdateQueries(updateQueries, result),
             ],
-            extraReducers,
+            extraReducers: this.getExtraReducers(),
           });
 
           refetchQueries.forEach((name) => { this.refetchQueryByName(name); });
@@ -607,7 +594,7 @@ export class QueryManager {
     // Insert the ObservableQuery into this.observableQueriesByName if the query has a name
     const queryDef = getQueryDefinition(observableQuery.options.query);
     if (queryDef.name && queryDef.name.value) {
-      const queryName = getQueryDefinition(observableQuery.options.query).name.value;
+      const queryName = queryDef.name.value;
 
       // XXX we may we want to warn the user about query name conflicts in the future
       this.queryIdsByName[queryName] = this.queryIdsByName[queryName] || [];
@@ -617,11 +604,14 @@ export class QueryManager {
 
   public removeObservableQuery(queryId: string) {
     const observableQuery = this.observableQueries[queryId].observableQuery;
-    const queryName = getQueryDefinition(observableQuery.options.query).name.value;
+    const definition = getQueryDefinition(observableQuery.options.query);
+    const queryName = definition.name ? definition.name.value : null;
     delete this.observableQueries[queryId];
-    this.queryIdsByName[queryName] = this.queryIdsByName[queryName].filter((val) => {
-      return !(observableQuery.queryId === val);
-    });
+    if (queryName) {
+      this.queryIdsByName[queryName] = this.queryIdsByName[queryName].filter((val) => {
+        return !(observableQuery.queryId === val);
+      });
+    }
   }
 
   public resetStore(): void {
@@ -650,9 +640,7 @@ export class QueryManager {
     Object.keys(this.observableQueries).forEach((queryId) => {
       const storeQuery = this.reduxRootSelector(this.store.getState()).queries[queryId];
 
-      if (! this.observableQueries[queryId].observableQuery.options.noFetch &&
-        ! (storeQuery && storeQuery.stopped)
-      ) {
+      if (!this.observableQueries[queryId].observableQuery.options.noFetch) {
         this.observableQueries[queryId].observableQuery.refetch();
       }
     });

src/queries/store.ts

@@ -39,7 +39,6 @@ export type QueryStoreValue = {
   queryString: string;
   variables: Object;
   previousVariables: Object;
-  stopped: boolean;
   loading: boolean;
   networkStatus: NetworkStatus;
   networkError: Error;
@@ -108,7 +107,6 @@ export function queries(
       queryString: action.queryString,
       variables: action.variables,
       previousVariables,
-      stopped: false,
       loading: true,
       networkError: null,
       graphQLErrors: null,
@@ -182,12 +180,7 @@ export function queries(
   } else if (isQueryStopAction(action)) {
     const newState = assign({}, previousState) as QueryStore;
 
-    newState[action.queryId] = assign({}, previousState[action.queryId], {
-      loading: false,
-      stopped: true,
-      networkStatus: NetworkStatus.ready,
-    }) as QueryStoreValue;
-
+    delete newState[action.queryId];
     return newState;
   } else if (isStoreResetAction(action)) {
     return resetQueryState(previousState, action);

test/QueryManager.ts

@@ -535,7 +535,7 @@ describe('QueryManager', () => {
     });
   });
 
-  it('allows you to subscribe twice to the one query', (done) => {
+  it('allows you to subscribe twice to one query', (done) => {
     const request = {
       query: gql`
         query fetchLuke($id: String) {

test/client.ts

@@ -484,17 +484,16 @@ describe('client', () => {
 
     const finalState = { apollo: assign({}, initialState.apollo, {
       queries: {
-        '0': {
+        '1': {
           queryString: print(query),
           variables: undefined,
           loading: false,
           networkStatus: NetworkStatus.ready,
-          stopped: false,
           networkError: null,
           graphQLErrors: null,
           forceFetch: false,
           returnPartialData: false,
-          lastRequestId: 1,
+          lastRequestId: 2,
           previousVariables: null,
           metadata: null,
         },

test/mutationResults.ts

@@ -5,6 +5,8 @@ import { MutationBehaviorReducerArgs, MutationBehavior, cleanArray } from '../sr
 import { NormalizedCache, StoreObject } from '../src/data/storeUtils';
 import { isMutationResultAction, isQueryResultAction } from '../src/actions';
 
+import { Subscription } from '../src/util/Observable';
+
 import assign = require('lodash/assign');
 import clonedeep = require('lodash/cloneDeep');
 
@@ -993,10 +995,20 @@ describe('mutation results', () => {
     };
 
     it('analogous of ARRAY_INSERT', () => {
+      let subscriptionHandle: Subscription;
       return setup({
         request: { query: mutation },
         result: mutationResult,
       })
+      .then(() => {
+        // we have to actually subscribe to the query to be able to update it
+        return new Promise( (resolve, reject) => {
+          const handle = client.watchQuery({ query });
+          subscriptionHandle = handle.subscribe({
+            next(res) { resolve(res); },
+          });
+        });
+      })
       .then(() => {
         return client.mutate({
           mutation,
@@ -1017,6 +1029,8 @@ describe('mutation results', () => {
         return client.query({ query });
       })
       .then((newResult: any) => {
+        subscriptionHandle.unsubscribe();
+
         // There should be one more todo item than before
         assert.equal(newResult.data.todoList.todos.length, 4);
 
@@ -1105,10 +1119,20 @@ describe('mutation results', () => {
         errors.push(msg);
       };
 
+      let subscriptionHandle: Subscription;
       return setup({
         request: { query: mutation },
         result: mutationResult,
       })
+      .then(() => {
+        // we have to actually subscribe to the query to be able to update it
+        return new Promise( (resolve, reject) => {
+          const handle = client.watchQuery({ query });
+          subscriptionHandle = handle.subscribe({
+            next(res) { resolve(res); },
+          });
+        });
+      })
       .then(() => {
         return client.mutate({
           mutation,
@@ -1120,6 +1144,7 @@ describe('mutation results', () => {
         });
       })
       .then(() => {
+        subscriptionHandle.unsubscribe();
         assert.lengthOf(errors, 1);
         assert.equal(errors[0].message, `Hello... It's me.`);
         console.error = oldError;