From 5eddacaed1b5623aa5a3e740b2d3a674380d2cec Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 18:39:07 +0100 Subject: [PATCH 01/18] Add isListNullable to SchemaPredicates This is to check whether a given type for a list may have nullable entries. --- src/ast/schemaPredicates.ts | 37 +++++++++++++++++++++++++++++++++++++ 1 file changed, 37 insertions(+) diff --git a/src/ast/schemaPredicates.ts b/src/ast/schemaPredicates.ts index add9f9a..7d81c2c 100644 --- a/src/ast/schemaPredicates.ts +++ b/src/ast/schemaPredicates.ts @@ -4,6 +4,8 @@ import warning from 'warning'; import { buildClientSchema, isNullableType, + isListType, + isNonNullType, GraphQLSchema, GraphQLAbstractType, GraphQLObjectType, @@ -52,6 +54,41 @@ export class SchemaPredicates { return isNullableType(field.type); } + isListNullable(typename: string, fieldName: string): boolean { + const type = this.schema.getType(typename); + expectObjectType(type, typename); + + const object = type as GraphQLObjectType; + if (object === undefined) { + warning( + false, + 'Invalid type: The type `%s` is not a type in the defined schema, ' + + 'but the GraphQL document expects it to exist.\n' + + 'Traversal will continue, however this may lead to undefined behavior!', + typename + ); + + return false; + } + + const field = object.getFields()[fieldName]; + if (field === undefined) { + warning( + false, + 'Invalid field: The field `%s` does not exist on `%s`, ' + + 'but the GraphQL document expects it to exist.\n' + + 'Traversal will continue, however this may lead to undefined behavior!', + fieldName, + typename + ); + + return false; + } + + const ofType = isNonNullType(field.type) ? field.type.ofType : field.type; + return isListType(ofType) && isNullableType(ofType.ofType); + } + isInterfaceOfType( typeCondition: null | string, typename: string | void From 8a512c9f3a52a45861af7975e95b58d0af0037ee Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 18:39:46 +0100 Subject: [PATCH 02/18] Reimplement query operations using cascading nullables First of all, this gets rid of `readOperation`, because it's almost identical to `query`, which should save us some bytes. `readSelection` has been reimplemented. Instead of writing fields onto data directly, it now first stores them in `dataFieldValue`. At any point when we encounter an uncached field that is NOT nullable, we return undefined. This is done by checking `dataFieldValue` against the schema. This is now done in one place to save some space and the hassle of reading repetitive code. This works because we now represent uncached values as `undefined`, which has been done in the past, but has been removed. Hence: - When `dataFieldValue` is `undefined` and a field is nullable then we write `null` and continue - When `dataFieldValue` is `undefined` and a field is NOT nullable then we return `undefined` instead of `data` immediately - Otherwise we write the field as usual This is nice, because that means `undefined` cascades upwards and this same logic also applies to resolved links! --- src/exchange.ts | 4 +- src/operations/index.ts | 2 +- src/operations/query.ts | 205 +++++++++++++++------------------------- 3 files changed, 81 insertions(+), 130 deletions(-) diff --git a/src/exchange.ts b/src/exchange.ts index bc03902..8fba4b2 100644 --- a/src/exchange.ts +++ b/src/exchange.ts @@ -8,7 +8,7 @@ import { } from 'urql'; import { filter, map, merge, pipe, share, tap } from 'wonka'; -import { query, write, writeOptimistic, readOperation } from './operations'; +import { query, write, writeOptimistic } from './operations'; import { Store } from './store'; import { @@ -217,7 +217,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({ data = queryResult.data; queryDependencies = queryResult.dependencies; } else { - data = readOperation(store, operation, data).data; + data = query(store, operation, data).data; } } diff --git a/src/operations/index.ts b/src/operations/index.ts index 6964da2..e42218f 100644 --- a/src/operations/index.ts +++ b/src/operations/index.ts @@ -1,2 +1,2 @@ -export { query, readOperation } from './query'; +export { query } from './query'; export { write, writeOptimistic, writeFragment } from './write'; diff --git a/src/operations/query.ts b/src/operations/query.ts index dcbd570..d516903 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -17,7 +17,6 @@ import { DataField, Link, SelectionSet, - Completeness, OperationRequest, NullArray, } from '../types'; @@ -35,90 +34,60 @@ import { joinKeys, keyOfField } from '../helpers'; import { SchemaPredicates } from '../ast/schemaPredicates'; export interface QueryResult { - completeness: Completeness; dependencies: Set; + partial: boolean; data: null | Data; } interface Context { - result: QueryResult; + partial: boolean; store: Store; variables: Variables; fragments: Fragments; schemaPredicates?: SchemaPredicates; } -/** Reads a request entirely from the store */ -export const query = (store: Store, request: OperationRequest): QueryResult => { +export const query = ( + store: Store, + request: OperationRequest, + data?: Data +): QueryResult => { initStoreState(0); - - const result = startQuery(store, request); + const result = read(store, request, data); clearStoreState(); return result; }; -export const startQuery = (store: Store, request: OperationRequest) => { - const operation = getMainOperation(request.query); - const root: Data = Object.create(null); - const result: QueryResult = { - completeness: 'FULL', - dependencies: getCurrentDependencies(), - data: root, - }; - - const ctx: Context = { - variables: normalizeVariables(operation, request.variables), - fragments: getFragments(request.query), - result, - store, - schemaPredicates: store.schemaPredicates, - }; - - result.data = readSelection( - ctx, - ctx.store.getRootKey('query'), - getSelectionSet(operation), - root - ); - - return result; -}; - -export const readOperation = ( +const read = ( store: Store, request: OperationRequest, - data: Data -) => { - initStoreState(0); - + input?: Data +): QueryResult => { const operation = getMainOperation(request.query); - - const result: QueryResult = { - completeness: 'FULL', - dependencies: getCurrentDependencies(), - data: null, - }; + const rootKey = store.getRootKey(operation.operation); + const rootSelect = getSelectionSet(operation); const ctx: Context = { variables: normalizeVariables(operation, request.variables), fragments: getFragments(request.query), - result, + partial: false, store, schemaPredicates: store.schemaPredicates, }; - result.data = readRoot( - ctx, - ctx.store.getRootKey(operation.operation), - getSelectionSet(operation), - data - ); + const data = + input !== undefined + ? readRoot(ctx, rootKey, rootSelect, input) + : readSelection(ctx, rootKey, rootSelect, Object.create(null)); - clearStoreState(); - return result; + return { + dependencies: getCurrentDependencies(), + partial: data === undefined ? false : ctx.partial, + data: data === undefined ? null : data, + }; }; -export const readRoot = ( +const readRoot = ( ctx: Context, entityKey: string, select: SelectionSet, @@ -169,8 +138,11 @@ const readRootField = ( // Write entity to key that falls back to the given parentFieldKey const entityKey = ctx.store.keyOfEntity(originalData); if (entityKey !== null) { - const data: Data = Object.create(null); - return readSelection(ctx, entityKey, select, data); + // We assume that since this is used for result data this can never be undefined, + // since the result data has already been written to the cache + const newData = Object.create(null); + const fieldValue = readSelection(ctx, entityKey, select, newData); + return fieldValue === undefined ? null : fieldValue; } else { const typename = originalData.__typename; return readRoot(ctx, typename, select, originalData); @@ -182,7 +154,7 @@ const readSelection = ( entityKey: string, select: SelectionSet, data: Data -): Data | null => { +): Data | undefined => { const { store, variables, schemaPredicates } = ctx; const isQuery = entityKey === store.getRootKey('query'); if (!isQuery) addDependency(entityKey); @@ -192,15 +164,13 @@ const readSelection = ( ? store.getRootKey('query') : store.getField(entityKey, '__typename'); if (typeof typename !== 'string') { - ctx.result.completeness = 'EMPTY'; - return null; + return undefined; } data.__typename = typename; const iter = new SelectionIterator(typename, entityKey, select, ctx); let node; - let hasFields = false; while ((node = iter.next()) !== undefined) { // Derive the needed data from our node. const fieldName = getName(node); @@ -211,10 +181,13 @@ const readSelection = ( if (isQuery) addDependency(fieldKey); + // We temporarily store the data field in here, but undefined + // means that the value is missing from the cache + let dataFieldValue: void | DataField; + const resolvers = store.resolvers[typename]; - if (resolvers !== undefined && resolvers.hasOwnProperty(fieldName)) { + if (resolvers !== undefined && typeof resolvers[fieldName] === 'function') { // We have a resolver for this field. - hasFields = true; // Prepare the actual fieldValue, so that the resolver can use it if (fieldValue !== undefined) { data[fieldAlias] = fieldValue; @@ -227,74 +200,59 @@ const readSelection = ( ctx ); - if (node.selectionSet === undefined) { - // If it doesn't have a selection set we have resolved a property. - // We assume that a resolver for scalar values implies that this - // field is always present, so completeness won't be set to EMPTY here - data[fieldAlias] = resolverValue !== undefined ? resolverValue : null; + const isNull = resolverValue === undefined || resolverValue === null; + // When we have a schema we check for a user's resolver whether the field is nullable + // Otherwise we trust the resolver and assume that it is + if (node.selectionSet === undefined || isNull) { + dataFieldValue = isNull ? undefined : resolverValue; } else { // When it has a selection set we are resolving an entity with a // subselection. This can either be a list or an object. - const fieldSelect = getSelectionSet(node); - - data[fieldAlias] = resolveResolverResult( + dataFieldValue = resolveResolverResult( ctx, resolverValue, fieldKey, - fieldSelect, + getSelectionSet(node), data[fieldAlias] as Data | Data[] ); } } else if (node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly - if ( - fieldValue === undefined && - schemaPredicates !== undefined && - schemaPredicates.isFieldNullable(typename, fieldName) - ) { - // Cache Incomplete: A missing field means it wasn't cached - ctx.result.completeness = 'PARTIAL'; - data[fieldAlias] = null; - } else if (fieldValue === undefined) { - ctx.result.completeness = 'EMPTY'; - data[fieldAlias] = null; - } else { - // Not dealing with undefined means it's a cached field - data[fieldAlias] = fieldValue; - hasFields = true; - } + dataFieldValue = fieldValue; } else { - // null values mean that a field might be linked to other entities + // We have a selection set which means that we'll be checking for links const fieldSelect = getSelectionSet(node); const link = store.getLink(fieldKey); - // Cache Incomplete: A missing link for a field means it's not cached - if (link === undefined) { - if (typeof fieldValue === 'object' && fieldValue !== null) { - // The entity on the field was invalid and can still be recovered - data[fieldAlias] = fieldValue; - hasFields = true; - } else if ( - schemaPredicates !== undefined && - schemaPredicates.isFieldNullable(typename, fieldName) - ) { - ctx.result.completeness = 'PARTIAL'; - data[fieldAlias] = null; - } else { - ctx.result.completeness = 'EMPTY'; - data[fieldAlias] = null; - } - } else { + if (link !== undefined) { const prevData = data[fieldAlias] as Data; - data[fieldAlias] = resolveLink(ctx, link, fieldSelect, prevData); - hasFields = true; + dataFieldValue = resolveLink(ctx, link, fieldSelect, prevData); + } else if (typeof fieldValue === 'object' && fieldValue !== null) { + // The entity on the field was invalid but can still be recovered + dataFieldValue = fieldValue; } } - } - if (isQuery && ctx.result.completeness === 'PARTIAL' && !hasFields) { - ctx.result.completeness = 'EMPTY'; - return null; + // When dataFieldValue is undefined that means that we have to check whether we can continue, + // or whether this Data is now invalid, in which case we return undefined + if (schemaPredicates !== undefined) { + // If we can check against the schema an uncached field may be acceptable for partial results + if ( + dataFieldValue === undefined && + schemaPredicates.isFieldNullable(typename, fieldName) + ) { + data[fieldAlias] = null; + ctx.partial = true; + } else { + return undefined; + } + } else if (dataFieldValue === undefined) { + // Otherwise an uncached field means that the Data is invalid, so we return undefined + return undefined; + } else { + // Otherwise we can set the field on data and continue + data[fieldAlias] = dataFieldValue; + } } return data; @@ -306,13 +264,15 @@ const resolveResolverResult = ( key: string, select: SelectionSet, prevData: void | Data | Data[] -) => { +): DataField | undefined => { // When we are dealing with a list we have to call this method again. if (Array.isArray(result)) { + // TODO: Convert to for-loop // @ts-ignore: Link cannot be expressed as a recursive type return result.map((childResult, index) => { const data = prevData !== undefined ? prevData[index] : undefined; const indexKey = joinKeys(key, `${index}`); + // TODO: If SchemaPredicates.isListNullable is false we may need to return undefined for the entire list return resolveResolverResult(ctx, childResult, indexKey, select, data); }); } else if (result === null) { @@ -324,17 +284,8 @@ const resolveResolverResult = ( const childKey = (typeof result === 'string' ? result : ctx.store.keyOfEntity(result)) || key; - const selectionResult = readSelection(ctx, childKey, select, data); - - if (selectionResult !== null && typeof result === 'object') { - for (key in result) { - if (key !== '__typename' && result.hasOwnProperty(key)) { - selectionResult[key] = result[key]; - } - } - } - - return selectionResult; + // TODO: Copy over fields from result but check against schema whether that's safe + return readSelection(ctx, childKey, select, data); } warning( @@ -345,8 +296,7 @@ const resolveResolverResult = ( key ); - ctx.result.completeness = 'EMPTY'; - return null; + return undefined; }; const resolveLink = ( @@ -354,7 +304,7 @@ const resolveLink = ( link: Link | Link[], select: SelectionSet, prevData: void | Data | Data[] -): null | Data | Data[] => { +): DataField | undefined => { if (Array.isArray(link)) { const newLink = new Array(link.length); for (let i = 0, l = link.length; i < l; i++) { @@ -362,6 +312,7 @@ const resolveLink = ( newLink[i] = resolveLink(ctx, link[i], select, data); } + // TODO: If SchemaPredicates.isListNullable is false we may need to return undefined for the entire list return newLink; } else if (link === null) { return null; From a6427069496dfbfb3850528c95991031fb45ca80 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 18:47:34 +0100 Subject: [PATCH 03/18] Restore pruning root Query when no field was set When no field has been set on Query and the result would've been partially complete, then return `undefined` for the entire Query. --- src/operations/query.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/src/operations/query.ts b/src/operations/query.ts index d516903..792bd85 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -171,6 +171,7 @@ const readSelection = ( const iter = new SelectionIterator(typename, entityKey, select, ctx); let node; + let hasFields = false; while ((node = iter.next()) !== undefined) { // Derive the needed data from our node. const fieldName = getName(node); @@ -241,6 +242,7 @@ const readSelection = ( dataFieldValue === undefined && schemaPredicates.isFieldNullable(typename, fieldName) ) { + hasFields = true; data[fieldAlias] = null; ctx.partial = true; } else { @@ -251,11 +253,12 @@ const readSelection = ( return undefined; } else { // Otherwise we can set the field on data and continue + hasFields = true; data[fieldAlias] = dataFieldValue; } } - return data; + return isQuery && ctx.partial && !hasFields ? undefined : data; }; const resolveResolverResult = ( From 023ebef25d36c1a7e38889844fa19c9969a1298b Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 18:53:16 +0100 Subject: [PATCH 04/18] Update cachExchange to use partial instead of completeness --- src/exchange.ts | 24 ++++++++---------------- src/types.ts | 3 --- 2 files changed, 8 insertions(+), 19 deletions(-) diff --git a/src/exchange.ts b/src/exchange.ts index 8fba4b2..1022e30 100644 --- a/src/exchange.ts +++ b/src/exchange.ts @@ -9,6 +9,7 @@ import { import { filter, map, merge, pipe, share, tap } from 'wonka'; import { query, write, writeOptimistic } from './operations'; +import { SchemaPredicates } from './ast/schemaPredicates'; import { Store } from './store'; import { @@ -17,7 +18,6 @@ import { OptimisticMutationConfig, KeyingConfig, } from './types'; -import { SchemaPredicates } from './ast/schemaPredicates'; type OperationResultWithMeta = OperationResult & { outcome: CacheOutcome; @@ -96,13 +96,8 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({ }) => { if (!opts) opts = {}; - let schemaPredicates; - if (opts.schema) { - schemaPredicates = new SchemaPredicates(opts.schema); - } - const store = new Store( - schemaPredicates, + opts.schema ? new SchemaPredicates(opts.schema) : undefined, opts.resolvers, opts.updates, opts.optimistic, @@ -175,22 +170,19 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({ operation: Operation ): OperationResultWithMeta => { const policy = getRequestPolicy(operation); - const { data, dependencies, completeness } = query(store, operation); + const { data, dependencies, partial } = query(store, operation); let cacheOutcome: CacheOutcome; - if (completeness === 'FULL' || policy === 'cache-only') { - updateDependencies(operation, dependencies); - cacheOutcome = 'hit'; - } else if (completeness === 'PARTIAL') { - updateDependencies(operation, dependencies); - cacheOutcome = 'partial'; - } else { + if (data === null) { cacheOutcome = 'miss'; + } else { + updateDependencies(operation, dependencies); + cacheOutcome = partial || policy === 'cache-only' ? 'hit' : 'partial'; } return { - operation, outcome: cacheOutcome, + operation, data, }; }; diff --git a/src/types.ts b/src/types.ts index b868f8c..8443767 100644 --- a/src/types.ts +++ b/src/types.ts @@ -97,6 +97,3 @@ export interface OptimisticMutationConfig { export interface KeyingConfig { [typename: string]: KeyGenerator; } - -// Completeness of the query result -export type Completeness = 'EMPTY' | 'PARTIAL' | 'FULL'; From 7ac5833d1a3181bffde9ee8f1bc6727faf751e0d Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 18:57:18 +0100 Subject: [PATCH 05/18] Update Store to use read instead of startQuery --- src/operations/index.ts | 2 +- src/operations/query.ts | 2 +- src/store.ts | 6 ++---- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/src/operations/index.ts b/src/operations/index.ts index e42218f..71f31e8 100644 --- a/src/operations/index.ts +++ b/src/operations/index.ts @@ -1,2 +1,2 @@ -export { query } from './query'; +export { query, read } from './query'; export { write, writeOptimistic, writeFragment } from './write'; diff --git a/src/operations/query.ts b/src/operations/query.ts index 792bd85..3d38ebb 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -58,7 +58,7 @@ export const query = ( return result; }; -const read = ( +export const read = ( store: Store, request: OperationRequest, input?: Data diff --git a/src/store.ts b/src/store.ts index 6727e1e..74bbb48 100644 --- a/src/store.ts +++ b/src/store.ts @@ -15,7 +15,7 @@ import { } from './types'; import { keyOfEntity, joinKeys, keyOfField } from './helpers'; -import { startQuery } from './operations/query'; +import { read } from './operations/query'; import { writeFragment, startWrite } from './operations/write'; import { invalidate } from './operations/invalidate'; import { SchemaPredicates } from './ast/schemaPredicates'; @@ -211,9 +211,7 @@ export class Store { ctx: { query: DocumentNode; variables?: Variables }, updater: (data: Data | null) => null | Data ): void { - const { data, completeness } = startQuery(this, ctx); - const input = completeness === 'EMPTY' ? null : data; - const output = updater(input); + const output = updater(read(this, ctx).data); if (output !== null) { startWrite(this, ctx, output); } From 596ea64f8f302ce841df2dba722c50485c1e1dbe Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:00:31 +0100 Subject: [PATCH 06/18] Fix up tests using completeness to use partial --- src/operations/query.test.ts | 2 +- src/test-utils/examples-1.test.ts | 12 ++++++------ src/test-utils/suite.test.ts | 3 ++- 3 files changed, 9 insertions(+), 8 deletions(-) diff --git a/src/operations/query.test.ts b/src/operations/query.test.ts index 30cd48d..97feb5c 100644 --- a/src/operations/query.test.ts +++ b/src/operations/query.test.ts @@ -45,7 +45,7 @@ describe('Query', () => { it('test partial results', () => { const result = query(store, { query: TODO_QUERY }); - expect(result.completeness).toEqual('PARTIAL'); + expect(result.partial).toBe(true); expect(result.data).toEqual({ __typename: 'Query', todos: [ diff --git a/src/test-utils/examples-1.test.ts b/src/test-utils/examples-1.test.ts index 2863361..2c92fda 100644 --- a/src/test-utils/examples-1.test.ts +++ b/src/test-utils/examples-1.test.ts @@ -62,7 +62,7 @@ it('passes the "getting-started" example', () => { expect(queryRes.data).toEqual(todosData); expect(queryRes.dependencies).toEqual(writeRes.dependencies); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); const mutatedTodo = { ...todosData.todos[2], @@ -82,7 +82,7 @@ it('passes the "getting-started" example', () => { queryRes = query(store, { query: Todos }); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); expect(queryRes.data).toEqual({ ...todosData, todos: [...todosData.todos.slice(0, 2), mutatedTodo], @@ -109,7 +109,7 @@ it('passes the "getting-started" example', () => { queryRes = query(store, { query: Todos }); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); expect(queryRes.data).toEqual({ ...todosData, todos: [...todosData.todos.slice(0, 2), newMutatedTodo], @@ -145,7 +145,7 @@ it('respects property-level resolvers when given', () => { ], }); expect(queryRes.dependencies).toEqual(writeRes.dependencies); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); const mutatedTodo = { ...todosData.todos[2], @@ -165,7 +165,7 @@ it('respects property-level resolvers when given', () => { queryRes = query(store, { query: Todos }); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); expect(queryRes.data).toEqual({ ...todosData, todos: [ @@ -248,7 +248,7 @@ it('Respects property-level resolvers when given', () => { const queryRes = query(store, { query: Todos }); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); expect(queryRes.data).toEqual({ ...todosData, todos: [ diff --git a/src/test-utils/suite.test.ts b/src/test-utils/suite.test.ts index 9b62002..bde4845 100644 --- a/src/test-utils/suite.test.ts +++ b/src/test-utils/suite.test.ts @@ -14,8 +14,9 @@ const expectCacheIntegrity = (testcase: TestCase) => { const request = { query: testcase.query, variables: testcase.variables }; const writeRes = write(store, request, testcase.data); const queryRes = query(store, request); + expect(queryRes.data).not.toBe(null); expect(queryRes.data).toEqual(testcase.data); - expect(queryRes.completeness).toBe('FULL'); + expect(queryRes.partial).toBe(false); expect(queryRes.dependencies).toEqual(writeRes.dependencies); }; From 70654f70957d96f98c7ec5e32f75924651f774f4 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:04:20 +0100 Subject: [PATCH 07/18] Fix cacheOutcome to correctly be partial instead of full --- src/exchange.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/exchange.ts b/src/exchange.ts index 1022e30..e757d82 100644 --- a/src/exchange.ts +++ b/src/exchange.ts @@ -177,7 +177,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({ cacheOutcome = 'miss'; } else { updateDependencies(operation, dependencies); - cacheOutcome = partial || policy === 'cache-only' ? 'hit' : 'partial'; + cacheOutcome = !partial || policy === 'cache-only' ? 'hit' : 'partial'; } return { From a777d7d0849a2a1c0e3dd32e988114c7e7b02e34 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:17:32 +0100 Subject: [PATCH 08/18] Fix readSelection value setting not respecting schema+cached --- src/operations/query.test.ts | 2 +- src/operations/query.ts | 31 +++++++++++++++---------------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/src/operations/query.test.ts b/src/operations/query.test.ts index 97feb5c..1527276 100644 --- a/src/operations/query.test.ts +++ b/src/operations/query.test.ts @@ -43,7 +43,7 @@ describe('Query', () => { ); }); - it('test partial results', () => { + it.only('test partial results', () => { const result = query(store, { query: TODO_QUERY }); expect(result.partial).toBe(true); expect(result.data).toEqual({ diff --git a/src/operations/query.ts b/src/operations/query.ts index 3d38ebb..fdbc68e 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -234,25 +234,24 @@ const readSelection = ( } } - // When dataFieldValue is undefined that means that we have to check whether we can continue, - // or whether this Data is now invalid, in which case we return undefined - if (schemaPredicates !== undefined) { - // If we can check against the schema an uncached field may be acceptable for partial results - if ( - dataFieldValue === undefined && - schemaPredicates.isFieldNullable(typename, fieldName) - ) { - hasFields = true; - data[fieldAlias] = null; - ctx.partial = true; - } else { - return undefined; - } + // Now that dataFieldValue has been retrieved it'll be set on data + // If it's uncached (undefined) but nullable we can continue assembling + // a partial query result + if ( + dataFieldValue === undefined && + schemaPredicates !== undefined && + schemaPredicates.isFieldNullable(typename, fieldName) + ) { + // The field is uncached but we have a schema that says it's nullable + // Set the field to null and continue + hasFields = true; + data[fieldAlias] = null; + ctx.partial = true; } else if (dataFieldValue === undefined) { - // Otherwise an uncached field means that the Data is invalid, so we return undefined + // The field is uncached and not nullable; return undefined return undefined; } else { - // Otherwise we can set the field on data and continue + // Otherwise continue as usual hasFields = true; data[fieldAlias] = dataFieldValue; } From e380478a97a05b2f81d524a23dcf0c40c90759b5 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:36:14 +0100 Subject: [PATCH 09/18] Fix hasFields being accidentally set for nullable, uncached fields --- src/exchange.test.ts | 27 ++++++++++++++++++++------- src/operations/query.ts | 1 - 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/exchange.test.ts b/src/exchange.test.ts index 19d13f5..269dd7a 100644 --- a/src/exchange.test.ts +++ b/src/exchange.test.ts @@ -623,7 +623,7 @@ it('follows nested resolvers for mutations', () => { ]); }); -it.only('reexecutes query and returns data on partial result', () => { +it('reexecutes query and returns data on partial result', () => { jest.useFakeTimers(); const client = createClient({ url: '' }); const [ops$, next] = makeSubject(); @@ -633,6 +633,16 @@ it.only('reexecutes query and returns data on partial result', () => { // partial data. .mockImplementation(() => {}); + const initialQuery = gql` + query { + todos { + id + text + __typename + } + } + `; + const query = gql` query { todos { @@ -649,8 +659,13 @@ it.only('reexecutes query and returns data on partial result', () => { } `; - const queryOperation = client.createRequestOperation('query', { + const initialQueryOperation = client.createRequestOperation('query', { key: 1, + query: initialQuery, + }); + + const queryOperation = client.createRequestOperation('query', { + key: 2, query, }); @@ -673,6 +688,8 @@ it.only('reexecutes query and returns data on partial result', () => { const response = jest.fn( (forwardOp: Operation): OperationResult => { if (forwardOp.key === 1) { + return { operation: initialQueryOperation, data: queryData }; + } else if (forwardOp.key === 2) { return { operation: queryOperation, data: queryData }; } @@ -697,7 +714,7 @@ it.only('reexecutes query and returns data on partial result', () => { publish ); - next(queryOperation); + next(initialQueryOperation); jest.runAllTimers(); expect(response).toHaveBeenCalledTimes(1); expect(reexec).toHaveBeenCalledTimes(0); @@ -706,15 +723,11 @@ it.only('reexecutes query and returns data on partial result', () => { todos: [ { __typename: 'Todo', - author: null, - complete: null, id: '123', text: 'Learn', }, { __typename: 'Todo', - author: null, - complete: null, id: '456', text: 'Teach', }, diff --git a/src/operations/query.ts b/src/operations/query.ts index fdbc68e..705d841 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -244,7 +244,6 @@ const readSelection = ( ) { // The field is uncached but we have a schema that says it's nullable // Set the field to null and continue - hasFields = true; data[fieldAlias] = null; ctx.partial = true; } else if (dataFieldValue === undefined) { From 4573390d287dc84155798b5022d31e0f1ba2aadb Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:41:08 +0100 Subject: [PATCH 10/18] Fix assumed null for cache resolvers --- src/operations/query.ts | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/src/operations/query.ts b/src/operations/query.ts index 705d841..4f38141 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -201,11 +201,15 @@ const readSelection = ( ctx ); - const isNull = resolverValue === undefined || resolverValue === null; - // When we have a schema we check for a user's resolver whether the field is nullable - // Otherwise we trust the resolver and assume that it is - if (node.selectionSet === undefined || isNull) { - dataFieldValue = isNull ? undefined : resolverValue; + if (node.selectionSet === undefined) { + // When we have a schema we check for a user's resolver whether the field is nullable + // Otherwise we trust the resolver and assume that it is + const isNull = resolverValue === undefined || resolverValue === null; + if (isNull && schemaPredicates !== undefined) { + dataFieldValue = undefined; + } else { + dataFieldValue = isNull ? null : resolverValue; + } } else { // When it has a selection set we are resolving an entity with a // subselection. This can either be a list or an object. From bba155dfa52193d2bbe340d0290fc558bfa4c2fb Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:53:28 +0100 Subject: [PATCH 11/18] Correct invalidateQuery having an edge case where an empty query is returned --- src/store.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/store.test.ts b/src/store.test.ts index 79fd48f..0c6fe09 100644 --- a/src/store.test.ts +++ b/src/store.test.ts @@ -139,7 +139,7 @@ describe('Store with OptimisticMutationConfig', () => { store.invalidateQuery(Todos); clearStoreState(); ({ data } = query(store, { query: Todos })); - expect((data as any).todos).toEqual(null); + expect(data).toBe(null); expect(store.getRecord('Todo:0.text')).toBe(undefined); }); @@ -175,7 +175,7 @@ describe('Store with OptimisticMutationConfig', () => { query: Appointment, variables: { id: '1' }, })); - expect((data as any).appointment).toEqual(null); + expect(data).toBe(null); expect(store.getRecord('Appointment:1.info')).toBe(undefined); }); From 30306c4ddb3847840eda9abf88a5fca55142bacb Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 19:54:19 +0100 Subject: [PATCH 12/18] Defer setting ctx.partial in readSelection If we encounter an uncached, nullable field we'd set ctx.partial, but if we then later return early with undefined, that'd still be set! Instead we need to set it after iterating, so that early `return undefined` statements skip setting ctx.partial --- src/operations/query.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/operations/query.ts b/src/operations/query.ts index 4f38141..8679b12 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -172,6 +172,7 @@ const readSelection = ( let node; let hasFields = false; + let hasPartials = false; while ((node = iter.next()) !== undefined) { // Derive the needed data from our node. const fieldName = getName(node); @@ -248,8 +249,8 @@ const readSelection = ( ) { // The field is uncached but we have a schema that says it's nullable // Set the field to null and continue + hasPartials = true; data[fieldAlias] = null; - ctx.partial = true; } else if (dataFieldValue === undefined) { // The field is uncached and not nullable; return undefined return undefined; @@ -260,7 +261,8 @@ const readSelection = ( } } - return isQuery && ctx.partial && !hasFields ? undefined : data; + if (hasPartials) ctx.partial = true; + return isQuery && hasPartials && !hasFields ? undefined : data; }; const resolveResolverResult = ( From e3e75bed5a076733f3c739ab1705f8f802050d6b Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 20:07:17 +0100 Subject: [PATCH 13/18] Fix condition for using readRoot instead of readSelection --- src/operations/query.ts | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/src/operations/query.ts b/src/operations/query.ts index 8679b12..6a05d88 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -75,10 +75,11 @@ export const read = ( schemaPredicates: store.schemaPredicates, }; - const data = - input !== undefined - ? readRoot(ctx, rootKey, rootSelect, input) - : readSelection(ctx, rootKey, rootSelect, Object.create(null)); + let data = input || Object.create(null); + data = + rootKey !== 'Query' + ? readRoot(ctx, rootKey, rootSelect, data) + : readSelection(ctx, rootKey, rootSelect, data); return { dependencies: getCurrentDependencies(), From c22b2d16419d324d1ebdc5479efd75e8fa3057ff Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 20:07:30 +0100 Subject: [PATCH 14/18] Fix missing null field in mutation mock in Exchange tests --- src/exchange.test.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/exchange.test.ts b/src/exchange.test.ts index 269dd7a..a31f92b 100644 --- a/src/exchange.test.ts +++ b/src/exchange.test.ts @@ -497,6 +497,7 @@ it('follows nested resolvers for mutations', () => { { __typename: 'Author', id: '123', + book: null, name: '[REDACTED ONLINE]', }, { @@ -537,8 +538,7 @@ it('follows nested resolvers for mutations', () => { (forwardOp: Operation): OperationResult => { if (forwardOp.key === 1) { return { operation: queryOperation, data: queryData }; - } - if (forwardOp.key === 2) { + } else if (forwardOp.key === 2) { return { operation: mutationOperation, data: mutationData }; } From caf622ca8fad624da8bda8404f5706c1ae75fcd7 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 20:22:07 +0100 Subject: [PATCH 15/18] Fix resolver links not being trusted when undefined --- src/operations/query.ts | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/operations/query.ts b/src/operations/query.ts index 6a05d88..5b84cce 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -196,26 +196,17 @@ const readSelection = ( data[fieldAlias] = fieldValue; } - const resolverValue = resolvers[fieldName]( + let resolverValue: DataField | undefined = resolvers[fieldName]( data, fieldArgs || {}, store, ctx ); - if (node.selectionSet === undefined) { - // When we have a schema we check for a user's resolver whether the field is nullable - // Otherwise we trust the resolver and assume that it is - const isNull = resolverValue === undefined || resolverValue === null; - if (isNull && schemaPredicates !== undefined) { - dataFieldValue = undefined; - } else { - dataFieldValue = isNull ? null : resolverValue; - } - } else { + if (node.selectionSet !== undefined) { // When it has a selection set we are resolving an entity with a // subselection. This can either be a list or an object. - dataFieldValue = resolveResolverResult( + resolverValue = resolveResolverResult( ctx, resolverValue, fieldKey, @@ -223,6 +214,15 @@ const readSelection = ( data[fieldAlias] as Data | Data[] ); } + + // When we have a schema we check for a user's resolver whether the field is nullable + // Otherwise we trust the resolver and assume that it is + const isNull = resolverValue === undefined || resolverValue === null; + if (isNull && schemaPredicates !== undefined) { + dataFieldValue = undefined; + } else { + dataFieldValue = isNull ? null : resolverValue; + } } else if (node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly dataFieldValue = fieldValue; @@ -316,8 +316,8 @@ const resolveLink = ( if (Array.isArray(link)) { const newLink = new Array(link.length); for (let i = 0, l = link.length; i < l; i++) { - const data = prevData !== undefined ? prevData[i] : undefined; - newLink[i] = resolveLink(ctx, link[i], select, data); + const innerPrevData = prevData !== undefined ? prevData[i] : undefined; + newLink[i] = resolveLink(ctx, link[i], select, innerPrevData); } // TODO: If SchemaPredicates.isListNullable is false we may need to return undefined for the entire list From cebd12484b9dcb7263694c550a7150db3846415f Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 20:37:27 +0100 Subject: [PATCH 16/18] Check for nullable entities in lists --- src/exchange.test.ts | 1 + src/operations/query.ts | 67 ++++++++++++++++++++++++++++++++++------- 2 files changed, 57 insertions(+), 11 deletions(-) diff --git a/src/exchange.test.ts b/src/exchange.test.ts index a31f92b..e938430 100644 --- a/src/exchange.test.ts +++ b/src/exchange.test.ts @@ -520,6 +520,7 @@ it('follows nested resolvers for mutations', () => { __typename: 'Author', id: '123', name: '[REDACTED ONLINE]', + book: null, }, { __typename: 'Author', diff --git a/src/operations/query.ts b/src/operations/query.ts index 5b84cce..77b39d3 100644 --- a/src/operations/query.ts +++ b/src/operations/query.ts @@ -209,6 +209,8 @@ const readSelection = ( resolverValue = resolveResolverResult( ctx, resolverValue, + typename, + fieldName, fieldKey, getSelectionSet(node), data[fieldAlias] as Data | Data[] @@ -233,7 +235,14 @@ const readSelection = ( if (link !== undefined) { const prevData = data[fieldAlias] as Data; - dataFieldValue = resolveLink(ctx, link, fieldSelect, prevData); + dataFieldValue = resolveLink( + ctx, + link, + typename, + fieldName, + fieldSelect, + prevData + ); } else if (typeof fieldValue === 'object' && fieldValue !== null) { // The entity on the field was invalid but can still be recovered dataFieldValue = fieldValue; @@ -269,20 +278,39 @@ const readSelection = ( const resolveResolverResult = ( ctx: Context, result: DataField, + typename: string, + fieldName: string, key: string, select: SelectionSet, prevData: void | Data | Data[] ): DataField | undefined => { // When we are dealing with a list we have to call this method again. if (Array.isArray(result)) { - // TODO: Convert to for-loop - // @ts-ignore: Link cannot be expressed as a recursive type - return result.map((childResult, index) => { - const data = prevData !== undefined ? prevData[index] : undefined; - const indexKey = joinKeys(key, `${index}`); - // TODO: If SchemaPredicates.isListNullable is false we may need to return undefined for the entire list - return resolveResolverResult(ctx, childResult, indexKey, select, data); - }); + const { schemaPredicates } = ctx; + const isListNullable = + schemaPredicates !== undefined && + schemaPredicates.isListNullable(typename, fieldName); + const newResult = new Array(result.length); + for (let i = 0, l = result.length; i < l; i++) { + const data = prevData !== undefined ? prevData[i] : undefined; + const childKey = joinKeys(key, `${i}`); + const childResult = resolveResolverResult( + ctx, + result[i], + typename, + fieldName, + childKey, + select, + data + ); + if (childResult === undefined && !isListNullable) { + return undefined; + } else { + result[i] = childResult !== undefined ? childResult : null; + } + } + + return newResult; } else if (result === null) { return null; } else if (isDataOrKey(result)) { @@ -310,17 +338,34 @@ const resolveResolverResult = ( const resolveLink = ( ctx: Context, link: Link | Link[], + typename: string, + fieldName: string, select: SelectionSet, prevData: void | Data | Data[] ): DataField | undefined => { if (Array.isArray(link)) { + const { schemaPredicates } = ctx; + const isListNullable = + schemaPredicates !== undefined && + schemaPredicates.isListNullable(typename, fieldName); const newLink = new Array(link.length); for (let i = 0, l = link.length; i < l; i++) { const innerPrevData = prevData !== undefined ? prevData[i] : undefined; - newLink[i] = resolveLink(ctx, link[i], select, innerPrevData); + const childLink = resolveLink( + ctx, + link[i], + typename, + fieldName, + select, + innerPrevData + ); + if (childLink === undefined && !isListNullable) { + return undefined; + } else { + newLink[i] = childLink !== undefined ? childLink : null; + } } - // TODO: If SchemaPredicates.isListNullable is false we may need to return undefined for the entire list return newLink; } else if (link === null) { return null; From 4d30a199b6f546196e5d8f79683e1014e9b3269d Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 20:39:52 +0100 Subject: [PATCH 17/18] Extract shared getField code in SchemaPredicates --- src/ast/schemaPredicates.ts | 102 +++++++++++++++--------------------- 1 file changed, 42 insertions(+), 60 deletions(-) diff --git a/src/ast/schemaPredicates.ts b/src/ast/schemaPredicates.ts index 7d81c2c..8228fd2 100644 --- a/src/ast/schemaPredicates.ts +++ b/src/ast/schemaPredicates.ts @@ -21,70 +21,14 @@ export class SchemaPredicates { } isFieldNullable(typename: string, fieldName: string): boolean { - const type = this.schema.getType(typename); - expectObjectType(type, typename); - - const object = type as GraphQLObjectType; - if (object === undefined) { - warning( - false, - 'Invalid type: The type `%s` is not a type in the defined schema, ' + - 'but the GraphQL document expects it to exist.\n' + - 'Traversal will continue, however this may lead to undefined behavior!', - typename - ); - - return false; - } - - const field = object.getFields()[fieldName]; - if (field === undefined) { - warning( - false, - 'Invalid field: The field `%s` does not exist on `%s`, ' + - 'but the GraphQL document expects it to exist.\n' + - 'Traversal will continue, however this may lead to undefined behavior!', - fieldName, - typename - ); - - return false; - } - + const field = getField(this.schema, typename, fieldName); + if (field === undefined) return false; return isNullableType(field.type); } isListNullable(typename: string, fieldName: string): boolean { - const type = this.schema.getType(typename); - expectObjectType(type, typename); - - const object = type as GraphQLObjectType; - if (object === undefined) { - warning( - false, - 'Invalid type: The type `%s` is not a type in the defined schema, ' + - 'but the GraphQL document expects it to exist.\n' + - 'Traversal will continue, however this may lead to undefined behavior!', - typename - ); - - return false; - } - - const field = object.getFields()[fieldName]; - if (field === undefined) { - warning( - false, - 'Invalid field: The field `%s` does not exist on `%s`, ' + - 'but the GraphQL document expects it to exist.\n' + - 'Traversal will continue, however this may lead to undefined behavior!', - fieldName, - typename - ); - - return false; - } - + const field = getField(this.schema, typename, fieldName); + if (field === undefined) return false; const ofType = isNonNullType(field.type) ? field.type.ofType : field.type; return isListType(ofType) && isNullableType(ofType.ofType); } @@ -107,6 +51,44 @@ export class SchemaPredicates { } } +const getField = ( + schema: GraphQLSchema, + typename: string, + fieldName: string +) => { + const type = schema.getType(typename); + expectObjectType(type, typename); + + const object = type as GraphQLObjectType; + if (object === undefined) { + warning( + false, + 'Invalid type: The type `%s` is not a type in the defined schema, ' + + 'but the GraphQL document expects it to exist.\n' + + 'Traversal will continue, however this may lead to undefined behavior!', + typename + ); + + return undefined; + } + + const field = object.getFields()[fieldName]; + if (field === undefined) { + warning( + false, + 'Invalid field: The field `%s` does not exist on `%s`, ' + + 'but the GraphQL document expects it to exist.\n' + + 'Traversal will continue, however this may lead to undefined behavior!', + fieldName, + typename + ); + + return undefined; + } + + return field; +}; + const expectObjectType = (type: any, typename: string) => { invariant( type instanceof GraphQLObjectType, From f10753aa117c80f6597437a3b45a4b841ad87d03 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 4 Sep 2019 21:54:34 +0100 Subject: [PATCH 18/18] Remove .only from query test --- src/operations/query.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/operations/query.test.ts b/src/operations/query.test.ts index 1527276..97feb5c 100644 --- a/src/operations/query.test.ts +++ b/src/operations/query.test.ts @@ -43,7 +43,7 @@ describe('Query', () => { ); }); - it.only('test partial results', () => { + it('test partial results', () => { const result = query(store, { query: TODO_QUERY }); expect(result.partial).toBe(true); expect(result.data).toEqual({