From 1cb496cc6f3cf92d5187b6548508eca1455efb99 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 09:38:42 +0800 Subject: [PATCH 01/11] simplify reducer calls on mutations --- src/core/QueryManager.ts | 17 ++--------------- 1 file changed, 2 insertions(+), 15 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 4dc2abe9607..fcc09b9b476 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -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); }); From 1626bdfff2c84d7a7652c308f10d0749796afc0d Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:06:15 +0800 Subject: [PATCH 02/11] actually remove observable queries when stopped --- src/core/ObservableQuery.ts | 8 ++++++++ src/core/QueryManager.ts | 15 +++++++++------ 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/src/core/ObservableQuery.ts b/src/core/ObservableQuery.ts index f54a8cb5dd6..159abfcda01 100644 --- a/src/core/ObservableQuery.ts +++ b/src/core/ObservableQuery.ts @@ -366,6 +366,11 @@ export class ObservableQuery extends Observable { 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 { this.subscriptionHandles = []; this.queryManager.stopQuery(this.queryId); + if (this.shouldSubscribe) { + this.queryManager.removeObservableQuery(this.queryId); + } this.observers = []; } } diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index fcc09b9b476..3c862f31356 100644 --- a/src/core/QueryManager.ts +++ b/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 @@ -594,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] || []; @@ -604,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 { From 56f846d2be901824b66c268865ee96dd6ccfba4c Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:07:14 +0800 Subject: [PATCH 03/11] note to remove queries from store as well --- src/queries/store.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/src/queries/store.ts b/src/queries/store.ts index c3d03946afd..c7042516bd2 100644 --- a/src/queries/store.ts +++ b/src/queries/store.ts @@ -182,6 +182,7 @@ export function queries( } else if (isQueryStopAction(action)) { const newState = assign({}, previousState) as QueryStore; + // TODO (NOW): actually remove stopped queries from store. Fix for real this time. newState[action.queryId] = assign({}, previousState[action.queryId], { loading: false, stopped: true, From 504188c12b2861b15589482024196933d048a374 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:07:48 +0800 Subject: [PATCH 04/11] fix typo in test name --- test/QueryManager.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/QueryManager.ts b/test/QueryManager.ts index 9e3eee591d7..bbfa45b4466 100644 --- a/test/QueryManager.ts +++ b/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) { From 1b5b0fd56625f4e1919d20d0357a630da6a1dfe0 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:08:20 +0800 Subject: [PATCH 05/11] fix test broken by renumbering --- test/client.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/client.ts b/test/client.ts index dcc87854ec6..4210957f4e3 100644 --- a/test/client.ts +++ b/test/client.ts @@ -484,7 +484,7 @@ describe('client', () => { const finalState = { apollo: assign({}, initialState.apollo, { queries: { - '0': { + '1': { queryString: print(query), variables: undefined, loading: false, @@ -494,7 +494,7 @@ describe('client', () => { graphQLErrors: null, forceFetch: false, returnPartialData: false, - lastRequestId: 1, + lastRequestId: 2, previousVariables: null, metadata: null, }, From a68b3cbcc5f1c014a54c8795b4335482fcb8c7b1 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:10:03 +0800 Subject: [PATCH 06/11] fix mutation result tests --- test/mutationResults.ts | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/test/mutationResults.ts b/test/mutationResults.ts index 9b2bffb9667..353f37228c1 100644 --- a/test/mutationResults.ts +++ b/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; From b07655a51a261a22075e8ee0ae9612efd7895751 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:10:12 +0800 Subject: [PATCH 07/11] fix optimistic result tests --- test/optimistic.ts | 76 ++++++++++++++++++++++++++++++++++++++++++---- 1 file changed, 70 insertions(+), 6 deletions(-) diff --git a/test/optimistic.ts b/test/optimistic.ts index 14210c7f1b3..4f5b87b13d1 100644 --- a/test/optimistic.ts +++ b/test/optimistic.ts @@ -10,6 +10,8 @@ import { addFragmentsToDocument } from '../src/queries/getFromAST'; import assign = require('lodash/assign'); import clonedeep = require('lodash/cloneDeep'); +import { Subscription } from '../src/util/Observable'; + import gql from 'graphql-tag'; import { @@ -608,8 +610,8 @@ describe('optimistic mutation results', () => { }).then((res) => { checkBothMutationsAreApplied('This one was created with a mutation.', 'Optimistically generated 2'); const mutationsState = client.store.getState().apollo.mutations; - assert.equal(mutationsState[2].loading, false); - assert.equal(mutationsState[3].loading, true); + assert.equal(mutationsState['3'].loading, false); + assert.equal(mutationsState['4'].loading, true); return res; }); @@ -621,15 +623,15 @@ describe('optimistic mutation results', () => { }).then((res) => { checkBothMutationsAreApplied('This one was created with a mutation.', 'Second mutation.'); const mutationsState = client.store.getState().apollo.mutations; - assert.equal(mutationsState[2].loading, false); assert.equal(mutationsState[3].loading, false); + assert.equal(mutationsState[4].loading, false); return res; }); const mutationsState = client.store.getState().apollo.mutations; - assert.equal(mutationsState[2].loading, true); assert.equal(mutationsState[3].loading, true); + assert.equal(mutationsState[4].loading, true); checkBothMutationsAreApplied('Optimistically generated', 'Optimistically generated 2'); @@ -697,10 +699,20 @@ describe('optimistic 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(() => { const promise = client.mutate({ mutation, @@ -727,6 +739,7 @@ describe('optimistic 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); @@ -736,6 +749,7 @@ describe('optimistic mutation results', () => { }); it('two ARRAY_INSERT like mutations', () => { + let subscriptionHandle: Subscription; return setup({ request: { query: mutation }, result: mutationResult, @@ -744,6 +758,15 @@ describe('optimistic mutation results', () => { result: mutationResult2, delay: 50, }) + .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(() => { const updateQueries = { todoList: (prev, options) => { @@ -783,6 +806,7 @@ describe('optimistic 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, 5); @@ -793,13 +817,24 @@ describe('optimistic mutation results', () => { }); it('two mutations, one fails', () => { + let subscriptionHandle: Subscription; return setup({ request: { query: mutation }, error: new Error('forbidden (test error)'), + delay: 20, }, { request: { query: mutation }, result: mutationResult2, }) + .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(() => { const updateQueries = { todoList: (prev, options) => { @@ -835,6 +870,7 @@ describe('optimistic mutation results', () => { return Promise.all([promise, promise2]); }) .then(() => { + subscriptionHandle.unsubscribe(); const dataInStore = client.queryManager.getDataWithOptimisticResults(); assert.equal((dataInStore['TodoList5'] as any).todos.length, 4); assert.notProperty(dataInStore, 'Todo99'); @@ -1123,13 +1159,24 @@ describe('optimistic mutation - githunt comments', () => { commentContent: 'New Comment', }; + let subscriptionHandle: Subscription; return setup({ request: { query: addTypenameToDocument(mutation), variables: mutationVariables, }, result: mutationResult, - }).then(() => { + }) + .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, variables }); + subscriptionHandle = handle.subscribe({ + next(res) { resolve(res); }, + }); + }); + }) + .then(() => { return client.mutate({ mutation, optimisticResponse, @@ -1139,6 +1186,7 @@ describe('optimistic mutation - githunt comments', () => { }).then(() => { return client.query({ query, variables }); }).then((newResult: any) => { + subscriptionHandle.unsubscribe(); assert.equal(newResult.data.entry.comments.length, 2); }); }); @@ -1149,13 +1197,28 @@ describe('optimistic mutation - githunt comments', () => { commentContent: 'New Comment', }; + let subscriptionHandle: Subscription; return setup({ request: { query: addFragmentsToDocument(addTypenameToDocument(mutationWithFragment), fragmentWithTypenames), variables: mutationVariables, }, result: mutationResult, - }).then(() => { + }) + .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: queryWithFragment, + variables, + fragments: fragment, + }); + subscriptionHandle = handle.subscribe({ + next(res) { resolve(res); }, + }); + }); + }) + .then(() => { return client.mutate({ mutation: mutationWithFragment, optimisticResponse, @@ -1166,6 +1229,7 @@ describe('optimistic mutation - githunt comments', () => { }).then(() => { return client.query({ query: queryWithFragment, variables, fragments: fragment }); }).then((newResult: any) => { + subscriptionHandle.unsubscribe(); assert.equal(newResult.data.entry.comments.length, 2); }); }); From 40fb08d767fabdcba7ff8d028e7cf83ec1db2ef1 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:10:52 +0800 Subject: [PATCH 08/11] make note about nasty race condition --- test/optimistic.ts | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/optimistic.ts b/test/optimistic.ts index 4f5b87b13d1..f953fe2fbb4 100644 --- a/test/optimistic.ts +++ b/test/optimistic.ts @@ -825,6 +825,12 @@ describe('optimistic mutation results', () => { }, { request: { query: mutation }, result: mutationResult2, + // XXX this test will uncover a flaw in the design of optimistic responses combined with + // updateQueries or result reducers if you un-comment the line below. The issue is that + // optimistic updates are not commutative but are treated as such. When undoing an + // optimistic update, other optimistic updates should be rolled back and re-applied in the + // same order as before, otherwise the store can end up in an inconsistent state. + // delay: 50, }) .then(() => { // we have to actually subscribe to the query to be able to update it From 0c902d05b627dec99e7e2fbdb791ed6217e10830 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:15:10 +0800 Subject: [PATCH 09/11] actually remove stopped queries from the store --- src/queries/store.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/queries/store.ts b/src/queries/store.ts index c7042516bd2..c0055017212 100644 --- a/src/queries/store.ts +++ b/src/queries/store.ts @@ -182,13 +182,7 @@ export function queries( } else if (isQueryStopAction(action)) { const newState = assign({}, previousState) as QueryStore; - // TODO (NOW): actually remove stopped queries from store. Fix for real this time. - 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); From 3d60017090dfd56355bf5b9077700d1e8794f4b1 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:20:50 +0800 Subject: [PATCH 10/11] remove stopped attribute from query store values --- src/core/QueryManager.ts | 4 +--- src/queries/store.ts | 2 -- test/client.ts | 1 - test/store.ts | 1 - 4 files changed, 1 insertion(+), 7 deletions(-) diff --git a/src/core/QueryManager.ts b/src/core/QueryManager.ts index 3c862f31356..6ee8e3ab8ee 100644 --- a/src/core/QueryManager.ts +++ b/src/core/QueryManager.ts @@ -640,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(); } }); diff --git a/src/queries/store.ts b/src/queries/store.ts index c0055017212..ff258639ea0 100644 --- a/src/queries/store.ts +++ b/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, diff --git a/test/client.ts b/test/client.ts index 4210957f4e3..3e8fbe13e7a 100644 --- a/test/client.ts +++ b/test/client.ts @@ -489,7 +489,6 @@ describe('client', () => { variables: undefined, loading: false, networkStatus: NetworkStatus.ready, - stopped: false, networkError: null, graphQLErrors: null, forceFetch: false, diff --git a/test/store.ts b/test/store.ts index a2ffb8b8e4b..ff287761548 100644 --- a/test/store.ts +++ b/test/store.ts @@ -157,7 +157,6 @@ describe('createApolloStore', () => { 'previousVariables': undefined as any, 'queryString': '', 'returnPartialData': false, - 'stopped': false, 'variables': {}, 'metadata': null, }, From 0da4037126718fcfe4c9e3a6aed5179497105ba2 Mon Sep 17 00:00:00 2001 From: Jonas Helfer Date: Sat, 17 Dec 2016 22:33:33 +0800 Subject: [PATCH 11/11] update CHANGELOG.md --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index ca7561599bb..6526cc1eb48 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,7 @@ Expect active development and potentially significant breaking changes in the `0 ### vNEXT - ... +- Fix bug that caused updateQuery and reducers to run on stopped queries [PR #1054](https://github.com/apollostack/apollo-client/pull/1054) ### 0.5.21