From 1b9d64efeeb7a8839fbe4dfab1d1cddc65daa2b8 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Wed, 21 Sep 2016 14:32:06 -0700 Subject: [PATCH 1/6] Remove all diffing features and some dead code --- src/QueryManager.ts | 100 +++-------------- src/actions.ts | 2 - src/batching.ts | 4 +- src/data/diffAgainstStore.ts | 176 +++-------------------------- src/data/store.ts | 4 +- src/queries/directives.ts | 6 +- src/queries/store.ts | 4 - src/queryPrinting.ts | 107 ------------------ test/diffAgainstStore.ts | 208 +---------------------------------- 9 files changed, 40 insertions(+), 571 deletions(-) delete mode 100644 src/queryPrinting.ts diff --git a/src/QueryManager.ts b/src/QueryManager.ts index fad0dbf233f..0541350e609 100644 --- a/src/QueryManager.ts +++ b/src/QueryManager.ts @@ -58,7 +58,6 @@ import { import { diffSelectionSetAgainstStore, - removeUnusedVariablesFromQuery, } from './data/diffAgainstStore'; import { @@ -66,10 +65,6 @@ import { MutationQueryReducersMap, } from './data/mutationResults'; -import { - queryDocument, -} from './queryPrinting'; - import { QueryFetchRequest, QueryBatcher, @@ -179,7 +174,7 @@ export class QueryManager { queryTransformer?: QueryTransformer, resultTransformer?: ResultTransformer, resultComparator?: ResultComparator, - shouldBatch?: Boolean, + shouldBatch?: boolean, batchInterval?: number, }) { // XXX this might be the place to do introspection for inserting the `id` into the query? or @@ -805,53 +800,6 @@ export class QueryManager { }; } - // Takes a selection set for a query and diffs it against the store. - // Returns a query document of selection sets - // that must be fetched from the server and as well as the data returned from the store. - private handleDiffQuery({ - queryDef, - rootId, - variables, - fragmentMap, - noFetch, - }: { - queryDef: OperationDefinition, - rootId: string, - variables: Object, - fragmentMap: FragmentMap, - noFetch: boolean, - }): { - diffedQuery: Document, - initialResult: Object, - } { - const { missingSelectionSets, result } = diffSelectionSetAgainstStore({ - selectionSet: queryDef.selectionSet, - store: this.reduxRootSelector(this.store.getState()).data, - throwOnMissingField: false, - rootId, - variables, - fragmentMap, - }); - - const initialResult = result; - let diffedQuery: Document; - if (missingSelectionSets && missingSelectionSets.length && !noFetch) { - diffedQuery = queryDocument({ - missingSelectionSets, - variableDefinitions: queryDef.variableDefinitions, - name: queryDef.name, - fragmentMap, - }); - - removeUnusedVariablesFromQuery(diffedQuery); - } - - return { - diffedQuery, - initialResult, - }; - } - // Takes a request id, query id, a query document and information associated with the query // (e.g. variables, fragment map, etc.) and send it to the network interface. Returns // a promise for the result associated with that request. @@ -954,6 +902,7 @@ export class QueryManager { queryDoc, fragmentMap, } = this.transformQueryDocument(options); + const queryDef = getQueryDefinition(queryDoc); const queryString = print(queryDoc); const querySS = { @@ -962,52 +911,35 @@ export class QueryManager { selectionSet: queryDef.selectionSet, } as SelectionSetWithRoot; - // If we don't use diffing, then these will be the same as the original query, other than - // the queryTransformer that could have been applied. - let minimizedQueryString = queryString; - let minimizedQuery = querySS; - let minimizedQueryDoc = queryDoc; let storeResult: any; + let needToFetch: boolean = forceFetch; // If this is not a force fetch, we want to diff the query against the // store before we fetch it from the network interface. if (!forceFetch) { - const { - diffedQuery, - initialResult, - } = this.handleDiffQuery({ - queryDef, + const { isMissing, result } = diffSelectionSetAgainstStore({ + selectionSet: queryDef.selectionSet, + store: this.reduxRootSelector(this.store.getState()).data, + throwOnMissingField: false, rootId: querySS.id, variables, fragmentMap, - noFetch, }); - storeResult = initialResult; - if (diffedQuery) { - minimizedQueryDoc = diffedQuery; - minimizedQueryString = print(minimizedQueryDoc); - minimizedQuery = { - id: querySS.id, - typeName: 'Query', - selectionSet: getQueryDefinition(diffedQuery).selectionSet, - } as SelectionSetWithRoot; - } else { - minimizedQueryDoc = null; - minimizedQueryString = null; - minimizedQuery = null; - } + + // If we're in here, only fetch if we have missing fields + needToFetch = isMissing; + + storeResult = result; } const requestId = this.generateRequestId(); - const shouldFetch = minimizedQuery && !noFetch; + const shouldFetch = needToFetch && !noFetch; // Initialize query in store with unique requestId this.store.dispatch({ type: 'APOLLO_QUERY_INIT', queryString, query: querySS, - minimizedQueryString, - minimizedQuery, variables, forceFetch, returnPartialData: returnPartialData || noFetch, @@ -1027,7 +959,7 @@ export class QueryManager { result: { data: storeResult }, variables, query: querySS, - complete: !! minimizedQuery, + complete: !needToFetch, queryId, }); } @@ -1036,8 +968,8 @@ export class QueryManager { return this.fetchRequest({ requestId, queryId, - query: minimizedQueryDoc, - querySS: minimizedQuery, + query: queryDoc, + querySS, options, fragmentMap, }); diff --git a/src/actions.ts b/src/actions.ts index f9fc3c68806..965b428dce7 100644 --- a/src/actions.ts +++ b/src/actions.ts @@ -40,8 +40,6 @@ export interface QueryInitAction { type: 'APOLLO_QUERY_INIT'; queryString: string; query: SelectionSetWithRoot; - minimizedQueryString: string; - minimizedQuery: SelectionSetWithRoot; variables: Object; forceFetch: boolean; returnPartialData: boolean; diff --git a/src/batching.ts b/src/batching.ts index 6ad1b7ded18..8bcc676c681 100644 --- a/src/batching.ts +++ b/src/batching.ts @@ -34,7 +34,7 @@ export class QueryBatcher { // Queue on which the QueryBatcher will operate on a per-tick basis. public queuedRequests: QueryFetchRequest[] = []; - private shouldBatch: Boolean; + private shouldBatch: boolean; private pollInterval: Number; private pollTimer: NodeJS.Timer | any; //oddity in Typescript @@ -46,7 +46,7 @@ export class QueryBatcher { shouldBatch, networkInterface, }: { - shouldBatch: Boolean, + shouldBatch: boolean, networkInterface: NetworkInterface, }) { this.shouldBatch = shouldBatch; diff --git a/src/data/diffAgainstStore.ts b/src/data/diffAgainstStore.ts index 2fdba7548dc..74afb14b31d 100644 --- a/src/data/diffAgainstStore.ts +++ b/src/data/diffAgainstStore.ts @@ -17,19 +17,10 @@ import { isIdValue, } from './store'; -import { - SelectionSetWithRoot, -} from '../queries/store'; - import { SelectionSet, Field, - Directive, Document, - Selection, - FragmentDefinition, - OperationDefinition, - Variable, } from 'graphql'; import { @@ -46,12 +37,9 @@ import { ApolloError, } from '../errors'; -import flatten = require('lodash.flatten'); - export interface DiffResult { result: any; - isMissing?: 'true'; - missingSelectionSets?: SelectionSetWithRoot[]; + isMissing?: boolean; } export function diffQueryAgainstStore({ @@ -123,7 +111,7 @@ export function handleFragmentErrors(fragmentErrors: { [typename: string]: Error * @param {SelectionSet} selectionSet A GraphQL selection set * @param {Store} store The Apollo Client store object * @param {String} rootId The ID of the root object that the selection set applies to - * @param {Boolean} [throwOnMissingField] Throw an error rather than returning any selection sets + * @param {boolean} [throwOnMissingField] Throw an error rather than returning any selection sets * when a field isn't found in the store. * @return {result: Object, missingSelectionSets: [SelectionSet]} */ @@ -151,7 +139,7 @@ export function diffSelectionSetAgainstStore({ } const result = {}; - const missingFields: Selection[] = []; + let hasMissingFields = false; // A map going from a typename to missing field errors thrown on that // typename. This data structure is needed to support union types. For example, if we have @@ -164,16 +152,7 @@ export function diffSelectionSetAgainstStore({ selectionSet.selections.forEach((selection) => { // Don't push more than one missing field per field in the query - let missingFieldPushed = false; let fieldResult: any; - let fieldIsMissing: string; - - function pushMissingField(missingField: Selection) { - if (!missingFieldPushed) { - missingFields.push(missingField); - missingFieldPushed = true; - } - } const included = shouldInclude(selection, variables); @@ -187,16 +166,10 @@ export function diffSelectionSetAgainstStore({ fragmentMap, included, }); - fieldIsMissing = diffResult.isMissing; + hasMissingFields = hasMissingFields || diffResult.isMissing; fieldResult = diffResult.result; const resultFieldKey = resultKeyNameFromField(selection); - if (fieldIsMissing) { - // even if the field is not included, we want to keep it in the - // query that is sent to the server. So, we push it into the set of - // fields that is missing. - pushMissingField(selection); - } if (included && fieldResult !== undefined) { (result as any)[resultFieldKey] = fieldResult; } @@ -213,16 +186,14 @@ export function diffSelectionSetAgainstStore({ store, fragmentMap, }); - fieldIsMissing = diffResult.isMissing; - fieldResult = diffResult.result; - if (fieldIsMissing) { - pushMissingField(selection); - } + hasMissingFields = hasMissingFields || diffResult.isMissing; + fieldResult = diffResult.result; if (isObject(fieldResult)) { merge(result, fieldResult); } + if (!fragmentErrors[typename]) { fragmentErrors[typename] = null; } @@ -253,12 +224,9 @@ export function diffSelectionSetAgainstStore({ store, fragmentMap, }); - fieldIsMissing = diffResult.isMissing; + hasMissingFields = hasMissingFields || diffResult.isMissing; fieldResult = diffResult.result; - if (fieldIsMissing) { - pushMissingField(selection); - } if (isObject(fieldResult)) { merge(result, fieldResult); } @@ -281,36 +249,9 @@ export function diffSelectionSetAgainstStore({ handleFragmentErrors(fragmentErrors); } - // Set this to true if we don't have enough information at this level to generate a refetch - // query, so we need to merge the selection set with the parent, rather than appending - let isMissing: any; - let missingSelectionSets: any; - - // If we weren't able to resolve some selections from the store, construct them into - // a query we can fetch from the server - if (missingFields.length) { - if (rootId === 'ROOT_QUERY') { - const typeName = 'Query'; - - missingSelectionSets = [ - { - id: rootId, - typeName, - selectionSet: { - kind: 'SelectionSet', - selections: missingFields, - }, - }, - ]; - } else { - isMissing = 'true'; - } - } - return { result, - isMissing, - missingSelectionSets, + isMissing: hasMissingFields, }; } @@ -329,8 +270,8 @@ function diffFieldAgainstStore({ rootId: string, store: NormalizedCache, fragmentMap?: FragmentMap, - included?: Boolean, -}): FieldDiffResult { + included?: boolean, +}): DiffResult { const storeObj = store[rootId] || {}; const storeFieldKey = storeKeyNameFromField(field, variables); @@ -346,7 +287,8 @@ Perhaps you want to use the \`returnPartialData\` option?`, } return { - isMissing: 'true', + result: null, + isMissing: true, }; } @@ -424,95 +366,3 @@ Perhaps you want to use the \`returnPartialData\` option?`, throw new Error('Unexpected value in the store where the query had a subselection.'); } - -interface FieldDiffResult { - result?: any; - isMissing?: 'true'; -} - -function collectUsedVariablesFromSelectionSet(selectionSet: SelectionSet): any[] { - return uniq(flatten(selectionSet.selections.map((selection) => { - if (isField(selection)) { - return collectUsedVariablesFromField(selection); - } else if (isInlineFragment(selection)) { - return collectUsedVariablesFromSelectionSet(selection.selectionSet); - } else { - // Some named fragment. Don't handle it here, rely on the caller - // to process fragments separately. - return []; - } - }))); -} - -function collectUsedVariablesFromDirectives(directives: Directive[]) { - return flatten(directives.map(directive => { - if (directive.arguments) { - return flatten(directive.arguments.map(arg => { - if (arg.kind === 'Argument' && arg.value.kind === 'Variable') { - return [(arg.value as Variable).name.value]; - } - - return []; - })); - } - - return []; - })); -} - -function collectUsedVariablesFromField(field: Field) { - let variables: any[] = []; - - if (field.arguments) { - variables = flatten(field.arguments.map((arg) => { - if (arg.value.kind === 'Variable') { - return [(arg.value as Variable).name.value]; - } - - return []; - })); - } - - if (field.selectionSet) { - variables = [ - ...variables, - ...collectUsedVariablesFromSelectionSet(field.selectionSet), - ]; - } - - if (field.directives) { - variables = [ - ...variables, - ...collectUsedVariablesFromDirectives(field.directives), - ]; - } - - return uniq(variables); -} - -export function removeUnusedVariablesFromQuery ( - query: Document -): void { - const queryDef = getQueryDefinition(query); - const usedVariables = flatten( - query.definitions.map((def) => collectUsedVariablesFromSelectionSet( - (def as FragmentDefinition | OperationDefinition).selectionSet))); - - if (!queryDef.variableDefinitions) { - return; - } - - const diffedVariableDefinitions = - queryDef.variableDefinitions.filter((variableDefinition) => { - return usedVariables.indexOf( - variableDefinition.variable.name.value) !== -1; - }); - - queryDef.variableDefinitions = diffedVariableDefinitions; -} - -function uniq(array: any[]): any[] { - return array.filter( - (item, index, arr) => - arr.indexOf(item) === index); -} diff --git a/src/data/store.ts b/src/data/store.ts index 39fdcbaacb7..c59f5b07b82 100644 --- a/src/data/store.ts +++ b/src/data/store.ts @@ -104,8 +104,8 @@ export function data( const newState = writeSelectionSetToStore({ result: action.result.data, - dataId: queryStoreValue.minimizedQuery.id, - selectionSet: queryStoreValue.minimizedQuery.selectionSet, + dataId: queryStoreValue.query.id, + selectionSet: queryStoreValue.query.selectionSet, variables: queryStoreValue.variables, store: clonedState, dataIdFromObject: config.dataIdFromObject, diff --git a/src/queries/directives.ts b/src/queries/directives.ts index 6e93650eee7..f064a5e5d6c 100644 --- a/src/queries/directives.ts +++ b/src/queries/directives.ts @@ -7,7 +7,7 @@ import { } from 'graphql'; -export function shouldInclude(selection: Selection, variables?: { [name: string]: any }): Boolean { +export function shouldInclude(selection: Selection, variables?: { [name: string]: any }): boolean { if (!variables) { variables = {}; } @@ -16,7 +16,7 @@ export function shouldInclude(selection: Selection, variables?: { [name: string] return true; } - let res: Boolean = true; + let res: boolean = true; selection.directives.forEach((directive) => { // TODO should move this validation to GraphQL validation once that's implemented. if (directive.name.value !== 'skip' && directive.name.value !== 'include') { @@ -38,7 +38,7 @@ export function shouldInclude(selection: Selection, variables?: { [name: string] } const ifValue = directive.arguments[0].value; - let evaledValue: Boolean = false; + let evaledValue: boolean = false; if (!ifValue || ifValue.kind !== 'BooleanValue') { // means it has to be a variable value if this is a valid @skip or @include directive if (ifValue.kind !== 'Variable') { diff --git a/src/queries/store.ts b/src/queries/store.ts index 07a382389de..53faffac109 100644 --- a/src/queries/store.ts +++ b/src/queries/store.ts @@ -32,8 +32,6 @@ export interface QueryStore { export interface QueryStoreValue { queryString: string; query: SelectionSetWithRoot; - minimizedQueryString: string; - minimizedQuery: SelectionSetWithRoot; variables: Object; previousVariables: Object; loading: boolean; @@ -73,8 +71,6 @@ export function queries( newState[action.queryId] = { queryString: action.queryString, query: action.query, - minimizedQueryString: action.minimizedQueryString, - minimizedQuery: action.minimizedQuery, variables: action.variables, previousVariables, loading: true, diff --git a/src/queryPrinting.ts b/src/queryPrinting.ts deleted file mode 100644 index e1a4a43c24f..00000000000 --- a/src/queryPrinting.ts +++ /dev/null @@ -1,107 +0,0 @@ -import { - OperationDefinition, - VariableDefinition, - Name, - Document, -} from 'graphql'; - -import { print } from 'graphql-tag/printer'; - -import { - SelectionSetWithRoot, -} from './queries/store'; - -import { FragmentMap } from './queries/getFromAST'; - -export function printQueryForMissingData(options: QueryDefinitionOptions) { - return printQueryFromDefinition(queryDefinition(options)); -} - -export function printQueryFromDefinition(queryDef: OperationDefinition) { - const queryDocumentAst = { - kind: 'Document', - definitions: [ - queryDef, - ], - }; - - return print(queryDocumentAst); -} - -// Creates a query document out of the missing selection sets, named fragments, etc. -// in order to print. -export function queryDocument({ - missingSelectionSets, - variableDefinitions = null, - name = null, - fragmentMap, -}: QueryDocumentOptions): Document { - - const doc: Document = { - kind: 'Document', - definitions: [], - }; - - const opDefinition = queryDefinition({ - missingSelectionSets, - variableDefinitions, - name, - }); - - // add fragments to the query document - doc.definitions = [opDefinition]; - Object.keys(fragmentMap).forEach((key) => { - doc.definitions.push(fragmentMap[key]); - }); - - return doc; -} - -export function queryDefinition({ - missingSelectionSets, - variableDefinitions = null, - name = null, -}: QueryDefinitionOptions): OperationDefinition { - const selections: any[] = []; - - missingSelectionSets.forEach((missingSelectionSet: SelectionSetWithRoot, ii: any) => { - if (missingSelectionSet.id === 'CANNOT_REFETCH') { - throw new Error('diffAgainstStore did not merge selection sets correctly'); - } - - if (missingSelectionSet.id !== 'ROOT_QUERY') { - // At some point, put back support for the node interface. Look in the git history for - // the code that printed node queries here. - throw new Error('Only root query selections supported.'); - } - - missingSelectionSet.selectionSet.selections.forEach((selection) => { - selections.push(selection); - }); - }); - - return { - kind: 'OperationDefinition', - operation: 'query', - name, - variableDefinitions, - directives: [], - selectionSet: { - kind: 'SelectionSet', - selections, - }, - }; -} - -export type QueryDocumentOptions = { - missingSelectionSets: SelectionSetWithRoot[]; - variableDefinitions?: VariableDefinition[]; - name?: Name; - fragmentMap: FragmentMap; -} - -export type QueryDefinitionOptions = { - missingSelectionSets: SelectionSetWithRoot[]; - variableDefinitions?: VariableDefinition[]; - name?: Name; -} diff --git a/test/diffAgainstStore.ts b/test/diffAgainstStore.ts index 734b3e36741..1617ed03de0 100644 --- a/test/diffAgainstStore.ts +++ b/test/diffAgainstStore.ts @@ -5,7 +5,6 @@ import { diffSelectionSetAgainstStore, } from '../src/data/diffAgainstStore'; import { writeQueryToStore } from '../src/data/writeToStore'; -import { printQueryForMissingData } from '../src/queryPrinting'; import { getQueryDefinition, getFragmentDefinitions, @@ -17,10 +16,6 @@ import { getIdField, } from '../src/data/extensions'; -import { - NormalizedCache, -} from '../src/data/store'; - import gql from 'graphql-tag'; describe('diffing queries against the store', () => { @@ -44,81 +39,10 @@ describe('diffing queries against the store', () => { query, }); - assert.isUndefined(diffQueryAgainstStore({ + assert.isFalse(diffQueryAgainstStore({ store, query, - }).missingSelectionSets); - }); - - it('works with multiple root queries', () => { - const query = gql` - { - people_one(id: "1") { - name - } - otherQuery - } - `; - - - const store = {} as NormalizedCache; - - // Should return two root query selections - assert.equal(diffQueryAgainstStore({ - store, - query, - }).missingSelectionSets[0].selectionSet.selections.length, 2); - }); - - it('when the store is missing one field and doesn\'t know IDs', () => { - const firstQuery = gql` - { - people_one(id: "1") { - __typename - id - name - } - } - `; - - const result = { - people_one: { - __typename: 'Person', - id: 'lukeId', - name: 'Luke Skywalker', - }, - }; - - const store = writeQueryToStore({ - result, - query: firstQuery, - }); - - const secondQuery = gql` - { - people_one(id: "1") { - name - age - } - } - `; - - const { missingSelectionSets } = diffQueryAgainstStore({ - store, - query: secondQuery, - }); - - // XXX a more efficient diffing algorithm would actually only fetch `age` here. Something to - // implement next - assert.equal(printQueryForMissingData({ - missingSelectionSets, - }), `{ - people_one(id: "1") { - name - age - } -} -`); + }).isMissing); }); it('caches root queries both under the ID of the node and the query name', () => { @@ -156,136 +80,12 @@ describe('diffing queries against the store', () => { } `; - const { missingSelectionSets } = diffQueryAgainstStore({ - store, - query: secondQuery, - }); - - assert.isUndefined(missingSelectionSets); - assert.deepEqual(store['1'], result.people_one); - }); - - it('diffs root queries even when IDs are turned off', () => { - const firstQuery = gql` - { - people_one(id: "1") { - __typename, - id, - name - } - } - `; - - const result = { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }; - - const store = writeQueryToStore({ - result, - query: firstQuery, - dataIdFromObject: getIdField, - }); - - const secondQuery = gql` - { - people_one(id: "1") { - __typename - id - name - } - people_one(id: "2") { - __typename - id - name - } - } - `; - - const { missingSelectionSets } = diffQueryAgainstStore({ - store, - query: secondQuery, - }); - - assert.equal(printQueryForMissingData({ - missingSelectionSets, - }), `{ - people_one(id: "2") { - __typename - id - name - } -} -`); - assert.deepEqual(store['1'], result.people_one); - }); - - it('works with inline fragments', () => { - const firstQuery = gql` - { - people_one(id: "1") { - __typename, - ... on Person { - id, - name - } - } - } - `; - - const result = { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }; - - const store = writeQueryToStore({ - result, - query: firstQuery, - dataIdFromObject: getIdField, - }); - - const secondQuery = gql` - { - people_one(id: "1") { - __typename - ... on Person { - id - name - } - } - people_one(id: "2") { - __typename - ... on Person { - id - name - } - } - } - `; - - const { missingSelectionSets } = diffQueryAgainstStore({ + const { isMissing } = diffQueryAgainstStore({ store, query: secondQuery, }); - assert.equal(printQueryForMissingData({ - missingSelectionSets, - }), `{ - people_one(id: "2") { - __typename - ... on Person { - id - name - } - } -} -`); + assert.isFalse(isMissing); assert.deepEqual(store['1'], result.people_one); }); From 4125b1cca7f187f4f54cb81aac8de2f5d5d48809 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Wed, 21 Sep 2016 14:38:26 -0700 Subject: [PATCH 2/6] Fix tests --- src/QueryManager.ts | 1 - src/data/diffAgainstStore.ts | 3 +- test/QueryManager.ts | 355 ----------------------------------- test/diffAgainstStore.ts | 4 +- 4 files changed, 3 insertions(+), 360 deletions(-) diff --git a/src/QueryManager.ts b/src/QueryManager.ts index 0541350e609..8f99739b52e 100644 --- a/src/QueryManager.ts +++ b/src/QueryManager.ts @@ -40,7 +40,6 @@ import { import { GraphQLResult, Document, - OperationDefinition, FragmentDefinition, // We need to import this here to allow TypeScript to include it in the definition file even // though we don't use it. https://github.com/Microsoft/TypeScript/issues/5711 diff --git a/src/data/diffAgainstStore.ts b/src/data/diffAgainstStore.ts index 74afb14b31d..8f662b92c43 100644 --- a/src/data/diffAgainstStore.ts +++ b/src/data/diffAgainstStore.ts @@ -38,7 +38,7 @@ import { } from '../errors'; export interface DiffResult { - result: any; + result?: any; isMissing?: boolean; } @@ -287,7 +287,6 @@ Perhaps you want to use the \`returnPartialData\` option?`, } return { - result: null, isMissing: true, }; } diff --git a/test/QueryManager.ts b/test/QueryManager.ts index 85054de91cb..5a33e81d6ec 100644 --- a/test/QueryManager.ts +++ b/test/QueryManager.ts @@ -16,7 +16,6 @@ import { } from '../src/store'; import { - IdGetter, getIdField, } from '../src/data/extensions'; @@ -31,10 +30,6 @@ import { assert, } from 'chai'; -import { - series, -} from 'async'; - import { Document, GraphQLResult, @@ -843,101 +838,6 @@ describe('QueryManager', () => { }); }); - it('doesn\'t explode if you refetch before first fetch is done with query diffing', (done) => { - const primeQuery = gql` - { - people_one(id: 1) { - name - } - } - `; - - const complexQuery = gql` - { - luke: people_one(id: 1) { - name - } - vader: people_one(id: 4) { - name - } - } - `; - - const diffedQuery = gql` - { - vader: people_one(id: 4) { - name - } - } - `; - - const data1 = { - people_one: { - name: 'Luke Skywalker', - }, - }; - - const data2 = { - vader: { - name: 'Darth Vader', - }, - }; - - const dataRefetch = { - luke: { - name: 'Luke has a new name', - }, - vader: { - name: 'Vader has a new name', - }, - }; - - const queryManager = mockQueryManager( - { - request: { query: primeQuery }, - result: { data: data1 }, - }, - { - request: { query: diffedQuery }, - result: { data: data2 }, - delay: 5, - }, - { - request: { query: complexQuery }, - result: { data: dataRefetch }, - delay: 10, - } - ); - - // First, prime the store so that query diffing removes the query - queryManager.query({ query: primeQuery }).then(() => { - let handleCount = 0; - - const handle = queryManager.watchQuery({ - query: complexQuery, - }); - - const subscription = handle.subscribe({ - next(result) { - handleCount++; - if (handleCount === 1) { - // We never get the first fetch in the observable, because we called refetch first, - // which means we just don't get the outdated result - assert.deepEqual(result.data, dataRefetch); - subscription.unsubscribe(); - done(); - } - }, - error(error) { - done(error); - }, - }); - - // Refetch before we get any data - maybe the network is slow, and the user clicked refresh? - handle.refetch(); - }); - }); - it('supports returnPartialData #193', () => { const primeQuery = gql` query primeQuery { @@ -1206,202 +1106,6 @@ describe('QueryManager', () => { }); }); - it('diffs queries, preserving variable declarations', (done) => { - testDiffing([ - { - query: gql` - { - people_one(id: "1") { - __typename, - id, - name - } - } - `, - diffedQuery: gql` - { - people_one(id: "1") { - __typename, - id, - name - } - } - `, - diffedQueryResponse: { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }, - fullResponse: { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }, - variables: {}, - }, - { - query: gql` - query getSeveralPeople($lukeId: String!, $vaderId: String!) { - luke: people_one(id: $lukeId) { - __typename - id - name - } - vader: people_one(id: $vaderId) { - __typename - id - name - } - } - `, - diffedQuery: gql` - query getSeveralPeople($vaderId: String!) { - vader: people_one(id: $vaderId) { - __typename - id - name - } - } - `, - diffedQueryResponse: { - vader: { - __typename: 'Person', - id: '4', - name: 'Darth Vader', - }, - }, - fullResponse: { - luke: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - vader: { - __typename: 'Person', - id: '4', - name: 'Darth Vader', - }, - }, - variables: { - lukeId: '1', - vaderId: '4', - }, - }, - ], {}, done); - }); - - it('diffs queries with fragments, removing unused variables', (done) => { - testDiffing([ - { - query: gql` - query one ($fullName: String!, $includeNick: Boolean!) { - people_one(id: "1") { - ...personInfo - nick @include(if: $includeNick) - } - } - - fragment personInfo on Person { - __typename, - id, - name(fullName: $fullName) - } - `, - diffedQuery: gql` - query one ($fullName: String!, $includeNick: Boolean!) { - people_one(id: "1") { - ...personInfo - nick @include(if: $includeNick) - } - } - - fragment personInfo on Person { - __typename, - id, - name(fullName: $fullName) - } - `, - diffedQueryResponse: { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }, - fullResponse: { - people_one: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - }, - variables: { - fullName: 'Edmonds Karp', - includeNick: false, - }, - }, - { - query: gql` - query getSeveralPeople($lukeId: String!, $vaderId: String!, $fullName: String!) { - luke: people_one(id: $lukeId) { - ...personInfo - } - vader: people_one(id: $vaderId) { - ...personInfo - } - } - - fragment personInfo on Person { - __typename, - id, - name(fullName: $fullName) - } - `, - diffedQuery: gql` - query getSeveralPeople($vaderId: String!, $fullName: String!) { - vader: people_one(id: $vaderId) { - ...personInfo - } - } - - fragment personInfo on Person { - __typename, - id, - name(fullName: $fullName) - } - `, - diffedQueryResponse: { - vader: { - __typename: 'Person', - id: '4', - name: 'Darth Vader', - }, - }, - fullResponse: { - luke: { - __typename: 'Person', - id: '1', - name: 'Luke Skywalker', - }, - vader: { - __typename: 'Person', - id: '4', - name: 'Darth Vader', - }, - }, - variables: { - lukeId: '1', - vaderId: '4', - fullName: 'Edmonds Karp', - }, - }, - ], {}, done); - }); - it('does not broadcast queries when non-apollo actions are dispatched', (done) => { const query = gql` query fetchLuke($id: String) { @@ -3541,62 +3245,3 @@ describe('QueryManager', () => { }); }); - -function testDiffing( - queryArray: { - // The query the UI asks for - query: Document, - - // The query that we expect to be sent to the server - diffedQuery: Document, - - // The response the server would return for the diffedQuery - diffedQueryResponse: any, - - // The result the actual UI receives, after all data is fetched - fullResponse: any, - - // Variables to use in all queries - variables?: Object, - }[], - config: { - dataIdFromObject?: IdGetter, - }, - done: (err?: Error) => void -) { - const mockedResponses = queryArray.map(({ - diffedQuery, - diffedQueryResponse, - variables = {}, - }) => { - return { - request: { query: diffedQuery, variables }, - result: { data: diffedQueryResponse }, - }; - }); - const networkInterface = mockNetworkInterface(...mockedResponses); - - const queryManager = new QueryManager({ - networkInterface, - store: createApolloStore({ - config: { dataIdFromObject: getIdField }, - }), - reduxRootSelector: (state) => state.apollo, - }); - - const steps = queryArray.map(({ query, fullResponse, variables }) => { - return (cb: Function) => { - queryManager.query({ - query, - variables, - }).then((result) => { - assert.deepEqual(result.data, fullResponse); - cb(); - }); - }; - }); - - series(steps, (err, res) => { - done(err); - }); -} diff --git a/test/diffAgainstStore.ts b/test/diffAgainstStore.ts index 1617ed03de0..e61280e202c 100644 --- a/test/diffAgainstStore.ts +++ b/test/diffAgainstStore.ts @@ -39,7 +39,7 @@ describe('diffing queries against the store', () => { query, }); - assert.isFalse(diffQueryAgainstStore({ + assert.notOk(diffQueryAgainstStore({ store, query, }).isMissing); @@ -85,7 +85,7 @@ describe('diffing queries against the store', () => { query: secondQuery, }); - assert.isFalse(isMissing); + assert.notOk(isMissing); assert.deepEqual(store['1'], result.people_one); }); From 93f6d97b34c44cac61dd86aa9c2a91f37ca4a4c1 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Wed, 21 Sep 2016 14:39:33 -0700 Subject: [PATCH 3/6] Remove unused deps while we're at it --- package.json | 7 ------- 1 file changed, 7 deletions(-) diff --git a/package.json b/package.json index ed8fdc6f83e..1a7b8471e5f 100644 --- a/package.json +++ b/package.json @@ -73,13 +73,10 @@ "@types/promises-a-plus": "0.0.26", "@types/redux": "^3.5.29", "@types/sinon": "^1.16.29", - "async": "^2.0.0", "browserify": "^13.0.0", "chai": "^3.5.0", "chai-as-promised": "^6.0.0", "colors": "^1.1.2", - "concurrently": "^3.0.0", - "dataloader": "^1.1.0", "es6-promise": "^3.1.2", "typed-graphql": "1.0.2", "grunt": "1.0.1", @@ -88,14 +85,10 @@ "isomorphic-fetch": "^2.2.1", "istanbul": "^0.4.5", "lodash": "^4.6.1", - "lodash.mapvalues": "^4.3.0", - "lodash.omit": "^4.2.1", "minimist": "^1.2.0", "mocha": "^3.0.0", - "nodemon": "^1.9.2", "pretty-bytes": "^4.0.0", "remap-istanbul": "^0.5.1", - "request-promise": "^4.0.1", "rxjs": "^5.0.0-beta.11", "sinon": "^1.17.4", "source-map-support": "^0.4.0", From 614a9a243a07353b1614ae6970332e73a1f499d1 Mon Sep 17 00:00:00 2001 From: Tom Coleman Date: Fri, 30 Sep 2016 11:21:01 +1000 Subject: [PATCH 4/6] Fixed issue with double boolean logic error --- src/QueryManager.ts | 2 +- src/queries/store.ts | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/QueryManager.ts b/src/QueryManager.ts index 8f99739b52e..e9ba6e4a91e 100644 --- a/src/QueryManager.ts +++ b/src/QueryManager.ts @@ -958,7 +958,7 @@ export class QueryManager { result: { data: storeResult }, variables, query: querySS, - complete: !needToFetch, + complete: !shouldFetch, queryId, }); } diff --git a/src/queries/store.ts b/src/queries/store.ts index 53faffac109..13b31116421 100644 --- a/src/queries/store.ts +++ b/src/queries/store.ts @@ -131,7 +131,7 @@ export function queries( const newState = assign({}, previousState) as QueryStore; newState[action.queryId] = assign({}, previousState[action.queryId], { - loading: action.complete, + loading: !action.complete, networkError: null, previousVariables: null, }) as QueryStoreValue; From c1e93e86ede9da65ebe0f10d675689d3c22d0775 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 30 Sep 2016 10:24:20 -0700 Subject: [PATCH 5/6] Fix two last tests --- test/QueryManager.ts | 12 ++---------- test/client.ts | 2 -- 2 files changed, 2 insertions(+), 12 deletions(-) diff --git a/test/QueryManager.ts b/test/QueryManager.ts index 5a33e81d6ec..29aea229bd8 100644 --- a/test/QueryManager.ts +++ b/test/QueryManager.ts @@ -2800,18 +2800,10 @@ describe('QueryManager', () => { }`; const fullData = { fortuneCookie, author }; - const diffQuery = gql` - query { - author { - name - } - }`; - const diffData = { author }; - const queryManager = mockQueryManager( { - request: { query: diffQuery }, - result: { data: diffData }, + request: { query }, + result: { data: fullData }, delay: 5, }, { diff --git a/test/client.ts b/test/client.ts index 79af4a41062..b8df0cf82a8 100644 --- a/test/client.ts +++ b/test/client.ts @@ -425,8 +425,6 @@ describe('client', () => { typeName: 'Query', selectionSet: (query.definitions[0] as OperationDefinition).selectionSet, }, - minimizedQueryString: null, - minimizedQuery: null, variables: undefined, loading: false, stopped: false, From 07c0ee81b8c354d550a390a13a53e7124196e973 Mon Sep 17 00:00:00 2001 From: Sashko Stubailo Date: Fri, 30 Sep 2016 10:26:54 -0700 Subject: [PATCH 6/6] Add to changelog --- CHANGELOG.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 342a210db9a..a9e712d0825 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,10 @@ 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 + +- **Feature removal**: Remove query diffing functionality to make client more predictable and simplify implementation. Queries will still read from the store, and if the store does not have all of the necessary data the entire query will fetch from the server. Read justification and discussion in [Issue #615](https://github.com/apollostack/apollo-client/issues/615) [PR #693](https://github.com/apollostack/apollo-client/pull/693) + ### v0.4.20 - Fix: Warn but do not fail when refetchQueries includes an unknown query name [PR #700](https://github.com/apollostack/apollo-client/pull/700) - Fix: avoid field error on mutations after a query cancellation or a query failure by enforcing returnPartialData during previous data retrieval before applying a mutation update. [PR #696](https://github.com/apollostack/apollo-client/pull/696) and [Issue #647](https://github.com/apollostack/apollo-client/issues/647).