From 7b971a06a738f6d9332e9dbe785d53a1fb8c177d Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:01:35 +0000 Subject: [PATCH 01/13] Add Context.path tracking the currently field alias path --- exchanges/graphcache/src/operations/query.ts | 42 +++++++++++++++---- exchanges/graphcache/src/operations/shared.ts | 2 + exchanges/graphcache/src/operations/write.ts | 21 +++++++--- 3 files changed, 53 insertions(+), 12 deletions(-) diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index 6d811a7992..8d2b47d54b 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/exchanges/graphcache/src/operations/query.ts @@ -129,12 +129,17 @@ 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 + ctx.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 + ctx.path.pop(); } return data; @@ -147,8 +152,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 + ctx.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 + ctx.path.pop(); + } + return newData; } else if (originalData === null) { return null; @@ -289,14 +301,18 @@ const readSelection = ( isFieldAvailableOnType(store.schema, typename, fieldName); } - // We temporarily store the data field in here, but undefined - // means that the value is missing from the cache - let dataFieldValue: void | DataField; - + // We directly assign typenames and skip the field afterwards if (fieldName === '__typename') { data[fieldAlias] = typename; continue; - } else if (resultValue !== undefined && node.selectionSet === undefined) { + } + + // 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 + ctx.path.push(fieldAlias); + 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 +393,8 @@ const readSelection = ( } } + // After processing the field, remove the current alias from the path again + ctx.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 +438,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 + ctx.path.push(i); // Recursively read resolver result const childResult = resolveResolverResult( ctx, @@ -431,7 +451,9 @@ const resolveResolverResult = ( prevData != null ? prevData[i] : undefined, result[i] ); - + // After processing the field, remove the current index from the path + ctx.path.pop(); + // Check the result for cache-missed values if (childResult === undefined && !_isListNullable) { return undefined; } else { @@ -478,6 +500,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 + ctx.path.push(i); + // Recursively read the link const childLink = resolveLink( ctx, link[i], @@ -486,6 +511,9 @@ const resolveLink = ( select, prevData != null ? prevData[i] : undefined ); + // After processing the field, remove the current index from the path + ctx.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..1a278a73e5 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -26,6 +26,7 @@ export interface Context { fieldName: string; partial: boolean; optimistic: boolean; + path: Array; } export const contextRef: { current: Context | null } = { current: null }; @@ -48,6 +49,7 @@ export const makeContext = ( fieldName: '', partial: false, optimistic: !!optimistic, + path: [], }); export const updateContext = ( diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index d45d2b1748..691dec8f07 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -246,9 +246,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.path.push(fieldAlias); + + // Execute optimistic mutation functions on root fields + if (ctx.optimistic && isRoot) { const resolver = ctx.store.optimisticMutations[fieldName]; if (!resolver) continue; @@ -301,6 +306,9 @@ const writeSelection = ( updater(data, fieldArgs || {}, ctx.store, ctx); } } + + // After processing the field, remove the current alias from the path again + ctx.path.pop(); } }; @@ -316,15 +324,18 @@ const writeField = ( 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.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.path.pop(); } return newData; From adfa3f02d4b0eddf1b0e3004a0f5a67b99ab8b41 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:06:29 +0000 Subject: [PATCH 02/13] 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. --- exchanges/graphcache/src/operations/query.ts | 21 ++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index 8d2b47d54b..18fc867868 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/exchanges/graphcache/src/operations/query.ts @@ -130,7 +130,7 @@ const readRoot = ( const fieldAlias = getFieldAlias(node); const fieldValue = originalData[fieldAlias]; // Add the current alias to the walked path before processing the field's value - ctx.path.push(fieldAlias); + if (process.env.NODE_ENV !== 'production') ctx.path.push(fieldAlias); // Process the root field's value if (node.selectionSet && fieldValue !== null) { const fieldData = ensureData(fieldValue); @@ -139,7 +139,7 @@ const readRoot = ( data[fieldAlias] = fieldValue; } // After processing the field, remove the current alias from the path again - ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.path.pop(); } return data; @@ -154,11 +154,11 @@ const readRootField = ( const newData = new Array(originalData.length); for (let i = 0, l = originalData.length; i < l; i++) { // Add the current index to the walked path before reading the field's value - ctx.path.push(i); + if (process.env.NODE_ENV !== 'production') ctx.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 - ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.path.pop(); } return newData; @@ -311,7 +311,8 @@ const readSelection = ( // 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 - ctx.path.push(fieldAlias); + if (process.env.NODE_ENV !== 'production') ctx.path.push(fieldAlias); + if (resultValue !== undefined && node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly from the result dataFieldValue = resultValue; @@ -394,7 +395,7 @@ const readSelection = ( } // After processing the field, remove the current alias from the path again - ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.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 @@ -439,7 +440,7 @@ const resolveResolverResult = ( 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 - ctx.path.push(i); + if (process.env.NODE_ENV !== 'production') ctx.path.push(i); // Recursively read resolver result const childResult = resolveResolverResult( ctx, @@ -452,7 +453,7 @@ const resolveResolverResult = ( result[i] ); // After processing the field, remove the current index from the path - ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.path.pop(); // Check the result for cache-missed values if (childResult === undefined && !_isListNullable) { return undefined; @@ -501,7 +502,7 @@ const resolveLink = ( 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 - ctx.path.push(i); + if (process.env.NODE_ENV !== 'production') ctx.path.push(i); // Recursively read the link const childLink = resolveLink( ctx, @@ -512,7 +513,7 @@ const resolveLink = ( prevData != null ? prevData[i] : undefined ); // After processing the field, remove the current index from the path - ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.path.pop(); // Check the result for cache-missed values if (childLink === undefined && !_isListNullable) { return undefined; From 169a02a071dce6b2af848f07dd03c25194ea3b50 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:07:39 +0000 Subject: [PATCH 03/13] Add note to ResolveInfo type on why path isn't exposed via types --- exchanges/graphcache/src/types.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/exchanges/graphcache/src/types.ts b/exchanges/graphcache/src/types.ts index 1dec403b1b..386104850e 100644 --- a/exchanges/graphcache/src/types.ts +++ b/exchanges/graphcache/src/types.ts @@ -67,6 +67,7 @@ export interface ResolveInfo { variables: Variables; partial?: boolean; optimistic?: boolean; + // path: Array; is not actively exposed as it leaks alias information and is hence not reliably stable } export interface QueryInput { From 7c8992701966dd0e23aff1ea235a180541804bca Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:35:43 +0000 Subject: [PATCH 04/13] Provide the current field's GraphQLError on an updater's context --- exchanges/graphcache/src/cacheExchange.ts | 9 +++++-- exchanges/graphcache/src/operations/shared.ts | 24 ++++++++++++++++++- exchanges/graphcache/src/operations/write.ts | 6 +++++ exchanges/graphcache/src/types.ts | 3 ++- 4 files changed, 38 insertions(+), 4 deletions(-) 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/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 1a278a73e5..2d45ef049e 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,6 +30,7 @@ export interface Context { parentFieldKey: string; parent: Data; fieldName: string; + error: GraphQLError | undefined; partial: boolean; optimistic: boolean; path: Array; @@ -31,6 +38,19 @@ export interface Context { export const contextRef: { current: Context | null } = { current: null }; +let errorMap: { [path: string]: GraphQLError } | undefined; + +export const initErrorMap = (error?: CombinedError | undefined) => { + errorMap = undefined; + for (let i = 0; error && i < error.graphQLErrors.length; i++) { + const graphQLError = error.graphQLErrors[i]; + if (graphQLError.path && graphQLError.path.length) { + if (!errorMap) errorMap = Object.create(null); + errorMap![graphQLError.path.join('.')] = graphQLError; + } + } +}; + export const makeContext = ( store: Store, variables: Variables, @@ -47,6 +67,7 @@ export const makeContext = ( parentKey: entityKey, parentFieldKey: '', fieldName: '', + error: undefined, partial: false, optimistic: !!optimistic, path: [], @@ -66,6 +87,7 @@ export const updateContext = ( ctx.parentKey = entityKey; ctx.parentFieldKey = fieldKey; ctx.fieldName = fieldName; + ctx.error = errorMap && errorMap[ctx.path.join('.')]; }; const isFragmentHeuristicallyMatching = ( diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 691dec8f07..7e8a04c31c 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, + initErrorMap, } from './shared'; export interface WriteResult { @@ -53,8 +56,10 @@ export const write = ( store: Store, request: OperationRequest, data: Data, + error?: CombinedError | undefined, key?: number ): WriteResult => { + initErrorMap(error); initDataState('write', store.data, key || null); const result = startWrite(store, request, data); clearDataState(); @@ -96,6 +101,7 @@ export const writeOptimistic = ( request: OperationRequest, key: number ): WriteResult => { + initErrorMap(); initDataState('write', store.data, key, true); const operation = getMainOperation(request.query); diff --git a/exchanges/graphcache/src/types.ts b/exchanges/graphcache/src/types.ts index 386104850e..8432df9978 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,6 +65,7 @@ export interface ResolveInfo { fieldName: string; fragments: Fragments; variables: Variables; + error: GraphQLError | undefined; partial?: boolean; optimistic?: boolean; // path: Array; is not actively exposed as it leaks alias information and is hence not reliably stable From 77df231e37df90b708e7aa9cc0737209e9f4683b Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:39:40 +0000 Subject: [PATCH 05/13] Update docs to add information about the error field --- docs/api/graphcache.md | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) 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 From 8c65588bff0516290129491cf130ee449176c087 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:49:02 +0000 Subject: [PATCH 06/13] 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`. --- exchanges/graphcache/src/operations/shared.ts | 11 ++++++++++- exchanges/graphcache/src/operations/write.ts | 9 ++++++--- 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 2d45ef049e..a317050567 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -42,7 +42,11 @@ let errorMap: { [path: string]: GraphQLError } | undefined; export const initErrorMap = (error?: CombinedError | undefined) => { errorMap = undefined; - for (let i = 0; error && i < error.graphQLErrors.length; i++) { + for ( + let i = 0; + error && error.graphQLErrors && i < error.graphQLErrors.length; + i++ + ) { const graphQLError = error.graphQLErrors[i]; if (graphQLError.path && graphQLError.path.length) { if (!errorMap) errorMap = Object.create(null); @@ -51,6 +55,11 @@ export const initErrorMap = (error?: CombinedError | undefined) => { } }; +export const isFieldMissing = (ctx: Context): boolean => { + // Checks whether the current data field is a cache miss because of a GraphQLError + return ctx.path.length > 0 && !!errorMap && !!errorMap[ctx.path.join('.')]; +}; + export const makeContext = ( store: Store, variables: Variables, diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 7e8a04c31c..c4fd008d18 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -44,6 +44,7 @@ import { makeContext, updateContext, initErrorMap, + isFieldMissing, } from './shared'; export interface WriteResult { @@ -289,7 +290,9 @@ const writeSelection = ( InMemoryData.writeRecord( entityKey || typename, fieldKey, - fieldValue as EntityField + (fieldValue !== null || !isFieldMissing(ctx) + ? fieldValue + : undefined) as EntityField ); } @@ -326,7 +329,7 @@ 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++) { @@ -346,7 +349,7 @@ const writeField = ( return newData; } else if (data === null) { - return null; + return isFieldMissing(ctx) ? undefined : null; } const entityKey = ctx.store.keyOfEntity(data); From d334ac42862f448ec3ae62a6130d2d0fc744e9c4 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 11:53:18 +0000 Subject: [PATCH 07/13] Replace isFieldMissing with getFieldError --- exchanges/graphcache/src/operations/shared.ts | 9 ++++----- exchanges/graphcache/src/operations/write.ts | 6 +++--- 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index a317050567..9b0bf534bc 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -55,10 +55,9 @@ export const initErrorMap = (error?: CombinedError | undefined) => { } }; -export const isFieldMissing = (ctx: Context): boolean => { - // Checks whether the current data field is a cache miss because of a GraphQLError - return ctx.path.length > 0 && !!errorMap && !!errorMap[ctx.path.join('.')]; -}; +// Checks whether the current data field is a cache miss because of a GraphQLError +export const getFieldError = (ctx: Context): GraphQLError | undefined => + ctx.path.length > 0 && !!errorMap ? errorMap[ctx.path.join('.')] : undefined; export const makeContext = ( store: Store, @@ -96,7 +95,7 @@ export const updateContext = ( ctx.parentKey = entityKey; ctx.parentFieldKey = fieldKey; ctx.fieldName = fieldName; - ctx.error = errorMap && errorMap[ctx.path.join('.')]; + ctx.error = getFieldError(ctx); }; const isFragmentHeuristicallyMatching = ( diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index c4fd008d18..edf40a33c7 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -44,7 +44,7 @@ import { makeContext, updateContext, initErrorMap, - isFieldMissing, + getFieldError, } from './shared'; export interface WriteResult { @@ -290,7 +290,7 @@ const writeSelection = ( InMemoryData.writeRecord( entityKey || typename, fieldKey, - (fieldValue !== null || !isFieldMissing(ctx) + (fieldValue !== null || !getFieldError(ctx) ? fieldValue : undefined) as EntityField ); @@ -349,7 +349,7 @@ const writeField = ( return newData; } else if (data === null) { - return isFieldMissing(ctx) ? undefined : null; + return getFieldError(ctx) ? undefined : null; } const entityKey = ctx.store.keyOfEntity(data); From c672b87383807277b697c04dcc1c20665289cf3c Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 12:01:32 +0000 Subject: [PATCH 08/13] 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. --- exchanges/graphcache/src/operations/query.ts | 22 +++--- exchanges/graphcache/src/operations/shared.ts | 73 +++++++++++-------- exchanges/graphcache/src/operations/write.ts | 26 ++++--- exchanges/graphcache/src/types.ts | 2 +- 4 files changed, 68 insertions(+), 55 deletions(-) diff --git a/exchanges/graphcache/src/operations/query.ts b/exchanges/graphcache/src/operations/query.ts index 18fc867868..06f9ab01df 100644 --- a/exchanges/graphcache/src/operations/query.ts +++ b/exchanges/graphcache/src/operations/query.ts @@ -130,7 +130,8 @@ const readRoot = ( 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.path.push(fieldAlias); + 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); @@ -139,7 +140,7 @@ const readRoot = ( data[fieldAlias] = fieldValue; } // After processing the field, remove the current alias from the path again - if (process.env.NODE_ENV !== 'production') ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); } return data; @@ -154,11 +155,11 @@ const readRootField = ( const newData = new Array(originalData.length); 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.path.push(i); + 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.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); } return newData; @@ -311,7 +312,8 @@ const readSelection = ( // 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.path.push(fieldAlias); + if (process.env.NODE_ENV !== 'production') + ctx.__internal.path.push(fieldAlias); if (resultValue !== undefined && node.selectionSet === undefined) { // The field is a scalar and can be retrieved directly from the result @@ -395,7 +397,7 @@ const readSelection = ( } // After processing the field, remove the current alias from the path again - if (process.env.NODE_ENV !== 'production') ctx.path.pop(); + 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 @@ -440,7 +442,7 @@ const resolveResolverResult = ( 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.path.push(i); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i); // Recursively read resolver result const childResult = resolveResolverResult( ctx, @@ -453,7 +455,7 @@ const resolveResolverResult = ( result[i] ); // After processing the field, remove the current index from the path - if (process.env.NODE_ENV !== 'production') ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); // Check the result for cache-missed values if (childResult === undefined && !_isListNullable) { return undefined; @@ -502,7 +504,7 @@ const resolveLink = ( 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.path.push(i); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.push(i); // Recursively read the link const childLink = resolveLink( ctx, @@ -513,7 +515,7 @@ const resolveLink = ( prevData != null ? prevData[i] : undefined ); // After processing the field, remove the current index from the path - if (process.env.NODE_ENV !== 'production') ctx.path.pop(); + if (process.env.NODE_ENV !== 'production') ctx.__internal.path.pop(); // Check the result for cache-missed values if (childLink === undefined && !_isListNullable) { return undefined; diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 9b0bf534bc..147e48e5ad 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -33,31 +33,21 @@ export interface Context { error: GraphQLError | undefined; partial: boolean; optimistic: boolean; - path: Array; + __internal: { + path: Array; + errorMap: { [path: string]: GraphQLError } | undefined; + }; } export const contextRef: { current: Context | null } = { current: null }; let errorMap: { [path: string]: GraphQLError } | undefined; -export const initErrorMap = (error?: CombinedError | undefined) => { - errorMap = undefined; - for ( - let i = 0; - error && error.graphQLErrors && i < error.graphQLErrors.length; - i++ - ) { - const graphQLError = error.graphQLErrors[i]; - if (graphQLError.path && graphQLError.path.length) { - if (!errorMap) errorMap = Object.create(null); - errorMap![graphQLError.path.join('.')] = graphQLError; - } - } -}; - // Checks whether the current data field is a cache miss because of a GraphQLError export const getFieldError = (ctx: Context): GraphQLError | undefined => - ctx.path.length > 0 && !!errorMap ? errorMap[ctx.path.join('.')] : undefined; + ctx.__internal.path.length > 0 && !!errorMap + ? errorMap[ctx.__internal.path.join('.')] + : undefined; export const makeContext = ( store: Store, @@ -65,21 +55,40 @@ export const makeContext = ( fragments: Fragments, typename: string, entityKey: string, - optimistic?: boolean -): Context => ({ - store, - variables, - fragments, - parent: { __typename: typename }, - parentTypeName: typename, - parentKey: entityKey, - parentFieldKey: '', - fieldName: '', - error: undefined, - partial: false, - optimistic: !!optimistic, - path: [], -}); + 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, diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index edf40a33c7..3e885b2565 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -43,7 +43,6 @@ import { ensureData, makeContext, updateContext, - initErrorMap, getFieldError, } from './shared'; @@ -60,9 +59,8 @@ export const write = ( error?: CombinedError | undefined, key?: number ): WriteResult => { - initErrorMap(error); initDataState('write', store.data, key || null); - const result = startWrite(store, request, data); + const result = startWrite(store, request, data, error); clearDataState(); return result; }; @@ -70,7 +68,8 @@ export const write = ( export const startWrite = ( store: Store, request: OperationRequest, - data: Data + data: Data, + error?: CombinedError | undefined ) => { const operation = getMainOperation(request.query); const result: WriteResult = { data, dependencies: getCurrentDependencies() }; @@ -81,7 +80,9 @@ export const startWrite = ( normalizeVariables(operation, request.variables), getFragments(request.query), operationName, - operationName + operationName, + false, + error ); if (process.env.NODE_ENV !== 'production') { @@ -102,7 +103,6 @@ export const writeOptimistic = ( request: OperationRequest, key: number ): WriteResult => { - initErrorMap(); initDataState('write', store.data, key, true); const operation = getMainOperation(request.query); @@ -129,7 +129,8 @@ export const writeOptimistic = ( getFragments(request.query), operationName, operationName, - true + true, + undefined ); writeSelection(ctx, operationName, getSelectionSet(operation), result.data!); @@ -181,7 +182,8 @@ export const writeFragment = ( variables || {}, fragments, typename, - entityKey + entityKey, + undefined ); writeSelection(ctx, entityKey, getSelectionSet(fragment), dataToWrite); @@ -257,7 +259,7 @@ const writeSelection = ( if (fieldName === '__typename') continue; // Add the current alias to the walked path before processing the field's value - ctx.path.push(fieldAlias); + ctx.__internal.path.push(fieldAlias); // Execute optimistic mutation functions on root fields if (ctx.optimistic && isRoot) { @@ -317,7 +319,7 @@ const writeSelection = ( } // After processing the field, remove the current alias from the path again - ctx.path.pop(); + ctx.__internal.path.pop(); } }; @@ -334,7 +336,7 @@ const writeField = ( const newData = new Array(data.length); for (let i = 0, l = data.length; i < l; i++) { // Add the current index to the walked path before processing the link - ctx.path.push(i); + ctx.__internal.path.push(i); // Append the current index to the parentFieldKey fallback const indexKey = parentFieldKey ? joinKeys(parentFieldKey, `${i}`) @@ -344,7 +346,7 @@ const writeField = ( // 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.path.pop(); + ctx.__internal.path.pop(); } return newData; diff --git a/exchanges/graphcache/src/types.ts b/exchanges/graphcache/src/types.ts index 8432df9978..415616287d 100644 --- a/exchanges/graphcache/src/types.ts +++ b/exchanges/graphcache/src/types.ts @@ -68,7 +68,7 @@ export interface ResolveInfo { error: GraphQLError | undefined; partial?: boolean; optimistic?: boolean; - // path: Array; is not actively exposed as it leaks alias information and is hence not reliably stable + __internal?: unknown; } export interface QueryInput { From a7609664c18189c09ef3abb3a53fc19336554d61 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 12:20:26 +0000 Subject: [PATCH 09/13] Fix leftover global errorMap in shared.ts --- exchanges/graphcache/src/operations/shared.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/exchanges/graphcache/src/operations/shared.ts b/exchanges/graphcache/src/operations/shared.ts index 147e48e5ad..d4b79e083a 100644 --- a/exchanges/graphcache/src/operations/shared.ts +++ b/exchanges/graphcache/src/operations/shared.ts @@ -41,12 +41,10 @@ export interface Context { export const contextRef: { current: Context | null } = { current: null }; -let errorMap: { [path: string]: GraphQLError } | undefined; - // 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 && !!errorMap - ? errorMap[ctx.__internal.path.join('.')] + ctx.__internal.path.length > 0 && ctx.__internal.errorMap + ? ctx.__internal.errorMap[ctx.__internal.path.join('.')] : undefined; export const makeContext = ( From 75667cfd0eade14db081202c63e66bb8c7481a28 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 12:25:33 +0000 Subject: [PATCH 10/13] Add tests for errored fields set to undefined rather than null --- .../graphcache/src/operations/write.test.ts | 75 ++++++++++++++++++- 1 file changed, 74 insertions(+), 1 deletion(-) 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); + }); }); From 352c83ae28daf72eaf53ca7fcc81ba55b0b2a658 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Tue, 2 Feb 2021 12:31:04 +0000 Subject: [PATCH 11/13] Add changeset --- .changeset/smart-emus-jam.md | 5 +++++ 1 file changed, 5 insertions(+) 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`. From 9c15fd4e07bc5844b86ab2e1077584d537f9bccf Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 3 Feb 2021 13:25:04 +0000 Subject: [PATCH 12/13] Move updateContext in write.ts' updater call --- exchanges/graphcache/src/operations/write.ts | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 3e885b2565..1fd43077ca 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -299,20 +299,20 @@ const writeSelection = ( } 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); } From 6a284f596c4cbf97daa18843b842f1afe7bcc3df Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 3 Feb 2021 13:53:12 +0000 Subject: [PATCH 13/13] Deduplicate writeOptimistic logic with startWrite --- exchanges/graphcache/src/operations/write.ts | 70 +++++++------------- 1 file changed, 23 insertions(+), 47 deletions(-) diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 1fd43077ca..fbc4410501 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -65,11 +65,32 @@ export const write = ( return result; }; +export const writeOptimistic = ( + store: Store, + request: OperationRequest, + key: number +): WriteResult => { + if (process.env.NODE_ENV !== 'production') { + 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 startWrite = ( store: Store, request: OperationRequest, data: Data, - error?: CombinedError | undefined + error?: CombinedError | undefined, + isOptimistic?: boolean ) => { const operation = getMainOperation(request.query); const result: WriteResult = { data, dependencies: getCurrentDependencies() }; @@ -81,7 +102,7 @@ export const startWrite = ( getFragments(request.query), operationName, operationName, - false, + !!isOptimistic, error ); @@ -98,51 +119,6 @@ export const startWrite = ( return result; }; -export const writeOptimistic = ( - store: Store, - request: OperationRequest, - key: number -): WriteResult => { - initDataState('write', store.data, key, true); - - const operation = getMainOperation(request.query); - const result: WriteResult = { - data: {} as 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, - undefined - ); - - writeSelection(ctx, operationName, getSelectionSet(operation), result.data!); - - if (process.env.NODE_ENV !== 'production') { - popDebugNode(); - } - - clearDataState(); - return result; -}; - export const writeFragment = ( store: Store, query: DocumentNode,