From 6a687091b67dc3d0ee78353ca79b0aa401ba983c Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Thu, 4 Feb 2021 14:11:13 +0000 Subject: [PATCH] (graphcache) - Handle fields with associated GraphQLError as cache misses and provide errors to updaters (#1356) * Add Context.path tracking the currently field alias path * Remove active ctx.path tracking from query in production It's not needed to track current errors from results there, so it can alternatively be fully removed. * Add note to ResolveInfo type on why path isn't exposed via types * Provide the current field's GraphQLError on an updater's context * Update docs to add information about the error field * Overwrite null values as undefined when field has an associated error When a field has an associated GraphQLError then the `fieldValue` should be written as `undefined` rather than `null`. * Replace isFieldMissing with getFieldError * Isolate path/errorMap on context in context.__internal This is to further hide the implementation from the user and to ensure that sub-write methods like `updateQuery` or `writeFragment` on the Store don't interfere with this logic as they'd build up their own paths. * Fix leftover global errorMap in shared.ts * Add tests for errored fields set to undefined rather than null * Add changeset * Move updateContext in write.ts' updater call * Deduplicate writeOptimistic logic with startWrite --- .changeset/smart-emus-jam.md | 5 + docs/api/graphcache.md | 30 ++-- exchanges/graphcache/src/cacheExchange.ts | 9 +- exchanges/graphcache/src/operations/query.ts | 43 +++++- exchanges/graphcache/src/operations/shared.ts | 67 +++++++-- .../graphcache/src/operations/write.test.ts | 75 +++++++++- exchanges/graphcache/src/operations/write.ts | 128 +++++++++--------- exchanges/graphcache/src/types.ts | 4 +- 8 files changed, 261 insertions(+), 100 deletions(-) create mode 100644 .changeset/smart-emus-jam.md diff --git a/.changeset/smart-emus-jam.md b/.changeset/smart-emus-jam.md new file mode 100644 index 0000000000..791401db26 --- /dev/null +++ b/.changeset/smart-emus-jam.md @@ -0,0 +1,5 @@ +--- +'@urql/exchange-graphcache': major +--- + +Add improved error awareness to Graphcache. When Graphcache now receives a `GraphQLError` (via a `CombinedError`) it checks whether the `GraphQLError`'s `path` matches up with `null` values in the `data`. Any `null` values that the write operation now sees in the data will be replaced with a "cache miss" value (i.e. `undefined`) when it has an associated error. This means that errored fields from your GraphQL API will be marked as uncached and won't be cached. Instead the client will now attempt a refetch of the data so that errors aren't preventing future refetches or with schema awareness it will attempt a refetch automatically. Additionally, the `updates` functions will now be able to check whether the current field has any errors associated with it with `info.error`. diff --git a/docs/api/graphcache.md b/docs/api/graphcache.md index d22826ecc1..ce12e46b15 100644 --- a/docs/api/graphcache.md +++ b/docs/api/graphcache.md @@ -119,6 +119,13 @@ An `UpdateResolver` receives four arguments when it's called: `result`, `args`, | `cache` | `Cache` | The cache using which data can be read or written. [See `Cache`.](#cache) | | `info` | `Info` | Additional metadata and information about the current operation and the current field. [See `Info`.](#info) | +It's possible to derive more information about the current update using the `info` argument. For +instance this metadata contains the current `fieldName` of the updater which may be used to make an +updater function more reusable, along with `parentKey` and other key fields. It also contains +`variables` and `fragments` which remain the same for the entire write operation, and additionally +it may have the `error` field set to describe whether the current field is `null` because the API +encountered a `GraphQLError`. + [Read more about how to set up `updates` on the "Custom Updates" page.](../graphcache/custom-updates.md) @@ -463,17 +470,18 @@ This is a metadata object that is passed to every resolver and updater function. information about the current GraphQL document and query, and also some information on the current field that a given resolver or updater is called on. -| Argument | Type | Description | -| ---------------- | -------------------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------ | -| `parent` | `Data` | The field's parent entity's data, as it was written or read up until now, which means it may be incomplete. [Use `cache.resolve`](#resolve) to read from it. | -| `parentTypeName` | `string` | The field's parent entity's typename | -| `parentKey` | `string` | The field's parent entity's cache key (if any) | -| `parentFieldKey` | `string` | The current key's cache key, which is the parent entity's key combined with the current field's key (This is mostly obsolete) | -| `fieldName` | `string` | The current field's name | -| `fragments` | `{ [name: string]: FragmentDefinitionNode }` | A dictionary of fragments from the current GraphQL document | -| `variables` | `object` | The current GraphQL operation's variables (may be an empty object) | -| `partial` | `?boolean` | This may be set to `true` at any point in time (by your custom resolver or by _Graphcache_) to indicate that some data is uncached and missing | -| `optimistic` | `?boolean` | This is only `true` when an optimistic mutation update is running | +| Argument | Type | Description | +| ---------------- | -------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `parent` | `Data` | The field's parent entity's data, as it was written or read up until now, which means it may be incomplete. [Use `cache.resolve`](#resolve) to read from it. | +| `parentTypeName` | `string` | The field's parent entity's typename | +| `parentKey` | `string` | The field's parent entity's cache key (if any) | +| `parentFieldKey` | `string` | The current key's cache key, which is the parent entity's key combined with the current field's key (This is mostly obsolete) | +| `fieldName` | `string` | The current field's name | +| `fragments` | `{ [name: string]: FragmentDefinitionNode }` | A dictionary of fragments from the current GraphQL document | +| `variables` | `object` | The current GraphQL operation's variables (may be an empty object) | +| `error` | `GraphQLError \| undefined` | The current GraphQLError for a given field. This will always be `undefined` for resolvers and optimistic updaters, but may be present for updaters when the API has returned an error for a given field. | +| `partial` | `?boolean` | This may be set to `true` at any point in time (by your custom resolver or by _Graphcache_) to indicate that some data is uncached and missing | +| `optimistic` | `?boolean` | This is only `true` when an optimistic mutation update is running | > **Note:** Using `info` is regarded as a last resort. Please only use information from it if > there's no other solution to get to the metadata you need. We don't regard the `Info` API as diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts index 906f06f096..9adf894947 100644 --- a/exchanges/graphcache/src/cacheExchange.ts +++ b/exchanges/graphcache/src/cacheExchange.ts @@ -225,8 +225,13 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({ if (result.data) { // Write the result to cache and collect all dependencies that need to be // updated - const writeDependencies = write(store, operation, result.data, key) - .dependencies; + const writeDependencies = write( + store, + operation, + result.data, + result.error, + key + ).dependencies; collectPendingOperations(pendingOperations, writeDependencies); const queryResult = query(store, operation, result.data, key); diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index 6d811a7992..06f9ab01df 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/exchanges/graphcache/src/operations/query.ts @@ -129,12 +129,18 @@ const readRoot = ( while ((node = iterate())) { const fieldAlias = getFieldAlias(node); const fieldValue = originalData[fieldAlias]; + // Add the current alias to the walked path before processing the field's value + if (process.env.NODE_ENV !== 'production') + ctx.__internal.path.push(fieldAlias); + // Process the root field's value if (node.selectionSet && fieldValue !== null) { const fieldData = ensureData(fieldValue); data[fieldAlias] = readRootField(ctx, getSelectionSet(node), fieldData); } else { data[fieldAlias] = fieldValue; } + // After processing the field, remove the current alias from the path again + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); } return data; @@ -147,8 +153,15 @@ const readRootField = ( ): Data | NullArray | null => { if (Array.isArray(originalData)) { const newData = new Array(originalData.length); - for (let i = 0, l = originalData.length; i < l; i++) + for (let i = 0, l = originalData.length; i < l; i++) { + // Add the current index to the walked path before reading the field's value + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i); + // Recursively read the root field's value newData[i] = readRootField(ctx, select, originalData[i]); + // After processing the field, remove the current index from the path + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); + } + return newData; } else if (originalData === null) { return null; @@ -289,14 +302,20 @@ const readSelection = ( isFieldAvailableOnType(store.schema, typename, fieldName); } + // We directly assign typenames and skip the field afterwards + if (fieldName === '__typename') { + data[fieldAlias] = typename; + continue; + } + // We temporarily store the data field in here, but undefined // means that the value is missing from the cache let dataFieldValue: void | DataField; + // Add the current alias to the walked path before processing the field's value + if (process.env.NODE_ENV !== 'production') + ctx.__internal.path.push(fieldAlias); - if (fieldName === '__typename') { - data[fieldAlias] = typename; - continue; - } else if (resultValue !== undefined && node.selectionSet === undefined) { + if (resultValue !== undefined && node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly from the result dataFieldValue = resultValue; } else if ( @@ -377,6 +396,8 @@ const readSelection = ( } } + // After processing the field, remove the current alias from the path again + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); // 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 @@ -420,6 +441,8 @@ const resolveResolverResult = ( !store.schema || isListNullable(store.schema, typename, fieldName); const data = new Array(result.length); for (let i = 0, l = result.length; i < l; i++) { + // Add the current index to the walked path before reading the field's value + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i); // Recursively read resolver result const childResult = resolveResolverResult( ctx, @@ -431,7 +454,9 @@ const resolveResolverResult = ( prevData != null ? prevData[i] : undefined, result[i] ); - + // After processing the field, remove the current index from the path + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); + // Check the result for cache-missed values if (childResult === undefined && !_isListNullable) { return undefined; } else { @@ -478,6 +503,9 @@ const resolveLink = ( store.schema && isListNullable(store.schema, typename, fieldName); const newLink = new Array(link.length); for (let i = 0, l = link.length; i < l; i++) { + // Add the current index to the walked path before reading the field's value + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i); + // Recursively read the link const childLink = resolveLink( ctx, link[i], @@ -486,6 +514,9 @@ const resolveLink = ( select, prevData != null ? prevData[i] : undefined ); + // After processing the field, remove the current index from the path + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); + // Check the result for cache-missed values if (childLink === undefined && !_isListNullable) { return undefined; } else { diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 8a0aa75751..d4b79e083a 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -1,4 +1,10 @@ -import { FieldNode, InlineFragmentNode, FragmentDefinitionNode } from 'graphql'; +import { CombinedError } from '@urql/core'; +import { + GraphQLError, + FieldNode, + InlineFragmentNode, + FragmentDefinitionNode, +} from 'graphql'; import { isInlineFragment, @@ -24,31 +30,63 @@ export interface Context { parentFieldKey: string; parent: Data; fieldName: string; + error: GraphQLError | undefined; partial: boolean; optimistic: boolean; + __internal: { + path: Array; + errorMap: { [path: string]: GraphQLError } | undefined; + }; } export const contextRef: { current: Context | null } = { current: null }; +// Checks whether the current data field is a cache miss because of a GraphQLError +export const getFieldError = (ctx: Context): GraphQLError | undefined => + ctx.__internal.path.length > 0 && ctx.__internal.errorMap + ? ctx.__internal.errorMap[ctx.__internal.path.join('.')] + : undefined; + export const makeContext = ( store: Store, variables: Variables, fragments: Fragments, typename: string, entityKey: string, - optimistic?: boolean -): Context => ({ - store, - variables, - fragments, - parent: { __typename: typename }, - parentTypeName: typename, - parentKey: entityKey, - parentFieldKey: '', - fieldName: '', - partial: false, - optimistic: !!optimistic, -}); + optimistic?: boolean, + error?: CombinedError | undefined +): Context => { + const ctx: Context = { + store, + variables, + fragments, + parent: { __typename: typename }, + parentTypeName: typename, + parentKey: entityKey, + parentFieldKey: '', + fieldName: '', + error: undefined, + partial: false, + optimistic: !!optimistic, + __internal: { + path: [], + errorMap: undefined, + }, + }; + + if (error && error.graphQLErrors) { + for (let i = 0; i < error.graphQLErrors.length; i++) { + const graphQLError = error.graphQLErrors[i]; + if (graphQLError.path && graphQLError.path.length) { + if (!ctx.__internal.errorMap) + ctx.__internal.errorMap = Object.create(null); + ctx.__internal.errorMap![graphQLError.path.join('.')] = graphQLError; + } + } + } + + return ctx; +}; export const updateContext = ( ctx: Context, @@ -64,6 +102,7 @@ export const updateContext = ( ctx.parentKey = entityKey; ctx.parentFieldKey = fieldKey; ctx.fieldName = fieldName; + ctx.error = getFieldError(ctx); }; const isFragmentHeuristicallyMatching = ( diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts index 901431d045..773838e854 100644 --- a/exchanges/graphcache/src/operations/write.test.ts +++ b/exchanges/graphcache/src/operations/write.test.ts @@ -1,6 +1,6 @@ /* eslint-disable @typescript-eslint/no-var-requires */ -import { gql } from '@urql/core'; +import { gql, CombinedError } from '@urql/core'; import { minifyIntrospectionQuery } from '@urql/introspection'; import { write } from './write'; @@ -174,4 +174,77 @@ describe('Query', () => { // The field must still be `'test'` expect(InMemoryData.readRecord('Query', 'field')).toBe('test'); }); + + it('should write errored records as undefined rather than null', () => { + const query = gql` + { + missingField + setField + } + `; + + write( + store, + { query }, + { missingField: null, setField: 'test' } as any, + new CombinedError({ + graphQLErrors: [ + { + message: 'Test', + path: ['missingField'], + }, + ], + }) + ); + + InMemoryData.initDataState('read', store.data, null); + + // The setField must still be `'test'` + expect(InMemoryData.readRecord('Query', 'setField')).toBe('test'); + // The missingField must still be `undefined` + expect(InMemoryData.readRecord('Query', 'missingField')).toBe(undefined); + }); + + it('should write errored links as undefined rather than null', () => { + const query = gql` + { + missingTodoItem: todos { + id + text + } + missingTodo: todo { + id + text + } + } + `; + + write( + store, + { query }, + { + missingTodoItem: [null, { __typename: 'Todo', id: 1, text: 'Learn' }], + missingTodo: null, + } as any, + new CombinedError({ + graphQLErrors: [ + { + message: 'Test', + path: ['missingTodoItem', 0], + }, + { + message: 'Test', + path: ['missingTodo'], + }, + ], + }) + ); + + InMemoryData.initDataState('read', store.data, null); + expect(InMemoryData.readLink('Query', 'todos')).toEqual([ + undefined, + 'Todo:1', + ]); + expect(InMemoryData.readLink('Query', 'todo')).toEqual(undefined); + }); }); diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index d45d2b1748..fbc4410501 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -1,4 +1,5 @@ import { FieldNode, DocumentNode, FragmentDefinitionNode } from 'graphql'; +import { CombinedError } from '@urql/core'; import { getFragments, @@ -35,12 +36,14 @@ import { } from '../store'; import * as InMemoryData from '../store/data'; + import { Context, makeSelectionIterator, ensureData, makeContext, updateContext, + getFieldError, } from './shared'; export interface WriteResult { @@ -53,85 +56,66 @@ export const write = ( store: Store, request: OperationRequest, data: Data, + error?: CombinedError | undefined, key?: number ): WriteResult => { initDataState('write', store.data, key || null); - const result = startWrite(store, request, data); + const result = startWrite(store, request, data, error); clearDataState(); return result; }; -export const startWrite = ( +export const writeOptimistic = ( store: Store, request: OperationRequest, - data: Data -) => { - const operation = getMainOperation(request.query); - const result: WriteResult = { data, dependencies: getCurrentDependencies() }; - const operationName = store.rootFields[operation.operation]; - - const ctx = makeContext( - store, - normalizeVariables(operation, request.variables), - getFragments(request.query), - operationName, - operationName - ); - - if (process.env.NODE_ENV !== 'production') { - pushDebugNode(operationName, operation); - } - - writeSelection(ctx, operationName, getSelectionSet(operation), data); - + key: number +): WriteResult => { if (process.env.NODE_ENV !== 'production') { - popDebugNode(); + invariant( + getMainOperation(request.query).operation === 'mutation', + 'writeOptimistic(...) was called with an operation that is not a mutation.\n' + + 'This case is unsupported and should never occur.', + 10 + ); } + initDataState('write', store.data, key, true); + const result = startWrite(store, request, {} as Data, undefined, true); + clearDataState(); return result; }; -export const writeOptimistic = ( +export const startWrite = ( store: Store, request: OperationRequest, - key: number -): WriteResult => { - initDataState('write', store.data, key, true); - + data: Data, + error?: CombinedError | undefined, + isOptimistic?: boolean +) => { const operation = getMainOperation(request.query); - const result: WriteResult = { - data: {} as Data, - dependencies: getCurrentDependencies(), - }; + const result: WriteResult = { data, dependencies: getCurrentDependencies() }; const operationName = store.rootFields[operation.operation]; - invariant( - operationName === store.rootFields['mutation'], - 'writeOptimistic(...) was called with an operation that is not a mutation.\n' + - 'This case is unsupported and should never occur.', - 10 - ); - - if (process.env.NODE_ENV !== 'production') { - pushDebugNode(operationName, operation); - } - const ctx = makeContext( store, normalizeVariables(operation, request.variables), getFragments(request.query), operationName, operationName, - true + !!isOptimistic, + error ); - writeSelection(ctx, operationName, getSelectionSet(operation), result.data!); + if (process.env.NODE_ENV !== 'production') { + pushDebugNode(operationName, operation); + } + + writeSelection(ctx, operationName, getSelectionSet(operation), data); if (process.env.NODE_ENV !== 'production') { popDebugNode(); } - clearDataState(); return result; }; @@ -174,7 +158,8 @@ export const writeFragment = ( variables || {}, fragments, typename, - entityKey + entityKey, + undefined ); writeSelection(ctx, entityKey, getSelectionSet(fragment), dataToWrite); @@ -246,9 +231,14 @@ const writeSelection = ( } } - if (fieldName === '__typename') { - continue; - } else if (ctx.optimistic && isRoot) { + // We simply skip all typenames fields and assume they've already been written above + if (fieldName === '__typename') continue; + + // Add the current alias to the walked path before processing the field's value + ctx.__internal.path.push(fieldAlias); + + // Execute optimistic mutation functions on root fields + if (ctx.optimistic && isRoot) { const resolver = ctx.store.optimisticMutations[fieldName]; if (!resolver) continue; @@ -278,29 +268,34 @@ const writeSelection = ( InMemoryData.writeRecord( entityKey || typename, fieldKey, - fieldValue as EntityField + (fieldValue !== null || !getFieldError(ctx) + ? fieldValue + : undefined) as EntityField ); } if (isRoot) { - // We have to update the context to reflect up-to-date ResolveInfo - updateContext( - ctx, - data, - typename, - typename, - joinKeys(typename, fieldKey), - fieldName - ); - // We run side-effect updates after the default, normalized updates // so that the data is already available in-store if necessary const updater = ctx.store.updates[typename][fieldName]; if (updater) { + // We have to update the context to reflect up-to-date ResolveInfo + updateContext( + ctx, + data, + typename, + typename, + joinKeys(typename, fieldKey), + fieldName + ); + data[fieldName] = fieldValue; updater(data, fieldArgs || {}, ctx.store, ctx); } } + + // After processing the field, remove the current alias from the path again + ctx.__internal.path.pop(); } }; @@ -312,24 +307,27 @@ const writeField = ( select: SelectionSet, data: null | Data | NullArray, parentFieldKey?: string -): Link => { +): Link | undefined => { if (Array.isArray(data)) { const newData = new Array(data.length); for (let i = 0, l = data.length; i < l; i++) { - const item = data[i]; + // Add the current index to the walked path before processing the link + ctx.__internal.path.push(i); // Append the current index to the parentFieldKey fallback const indexKey = parentFieldKey ? joinKeys(parentFieldKey, `${i}`) : undefined; // Recursively write array data - const links = writeField(ctx, select, item, indexKey); + const links = writeField(ctx, select, data[i], indexKey); // Link cannot be expressed as a recursive type newData[i] = links as string | null; + // After processing the field, remove the current index from the path + ctx.__internal.path.pop(); } return newData; } else if (data === null) { - return null; + return getFieldError(ctx) ? undefined : null; } const entityKey = ctx.store.keyOfEntity(data); diff --git a/exchanges/graphcache/src/types.ts b/exchanges/graphcache/src/types.ts index 1dec403b1b..415616287d 100644 --- a/exchanges/graphcache/src/types.ts +++ b/exchanges/graphcache/src/types.ts @@ -1,5 +1,5 @@ import { TypedDocumentNode } from '@urql/core'; -import { DocumentNode, FragmentDefinitionNode } from 'graphql'; +import { GraphQLError, DocumentNode, FragmentDefinitionNode } from 'graphql'; // Helper types export type NullArray = Array; @@ -65,8 +65,10 @@ export interface ResolveInfo { fieldName: string; fragments: Fragments; variables: Variables; + error: GraphQLError | undefined; partial?: boolean; optimistic?: boolean; + __internal?: unknown; } export interface QueryInput {