From 04115f34d7f46c013dafa60e730ba440b605b505 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 12 Jun 2023 20:29:47 +0100 Subject: [PATCH 1/4] fix(graphcache): Allow partial optimistic results --- .changeset/five-icons-agree.md | 5 ++ .../graphcache/src/operations/write.test.ts | 6 ++- exchanges/graphcache/src/operations/write.ts | 49 +++++++++---------- 3 files changed, 32 insertions(+), 28 deletions(-) create mode 100644 .changeset/five-icons-agree.md diff --git a/.changeset/five-icons-agree.md b/.changeset/five-icons-agree.md new file mode 100644 index 0000000000..78eb832e19 --- /dev/null +++ b/.changeset/five-icons-agree.md @@ -0,0 +1,5 @@ +--- +'@urql/exchange-graphcache': patch +--- + +Make "Invalid undefined" warning heuristic smarter and allow for partial optimistic results. Previously, when a partial optimistic result would be passed, a warning would be issued, and in production, fields would be deleted from the cache. Instead, we now only issue a warning if these fields aren't cached already. diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts index 822809f3b3..13c03732f4 100644 --- a/exchanges/graphcache/src/operations/write.test.ts +++ b/exchanges/graphcache/src/operations/write.test.ts @@ -155,22 +155,24 @@ describe('Query', () => { ); }); - it('should skip undefined values that are expected', () => { + it.only('should skip undefined values that are expected', () => { const query = gql` { field } `; - write(store, { query }, { field: 'test' } as any); // This should not overwrite the field write(store, { query }, { field: undefined } as any); // Because of us writing an undefined field expect(console.warn).toHaveBeenCalledTimes(2); + expect((console.warn as any).mock.calls[0][0]).toMatch( /The field `field` does not exist on `Query`/ ); + write(store, { query }, { field: 'test' } as any); + write(store, { query }, { field: undefined } as any); InMemoryData.initDataState('read', store.data, null); // The field must still be `'test'` expect(InMemoryData.readRecord('Query', 'field')).toBe('test'); diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 33b552e100..488b7460e9 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -240,34 +240,31 @@ const writeSelection = ( let fieldValue = data[ctx.optimistic ? fieldName : fieldAlias]; // Development check of undefined fields - if (process.env.NODE_ENV !== 'production') { - if ( - rootField === 'query' && - fieldValue === undefined && - !deferRef && - !ctx.optimistic - ) { - const expected = - node.selectionSet === undefined - ? 'scalar (number, boolean, etc)' - : 'selection set'; - - warn( - 'Invalid undefined: The field at `' + - fieldKey + - '` is `undefined`, but the GraphQL query expects a ' + - expected + - ' for this field.', - 13 - ); - - continue; // Skip this field - } else if (ctx.store.schema && typename && fieldName !== '__typename') { - isFieldAvailableOnType(ctx.store.schema, typename, fieldName); + if (rootField === 'query' && fieldValue === undefined && !deferRef) { + if (process.env.NODE_ENV !== 'production') { + if (ctx.store.schema && typename && fieldName !== '__typename') { + isFieldAvailableOnType(ctx.store.schema, typename, fieldName); + } + + if (!entityKey || !InMemoryData.hasField(entityKey, fieldKey)) { + const expected = + node.selectionSet === undefined + ? 'scalar (number, boolean, etc)' + : 'selection set'; + + warn( + 'Invalid undefined: The field at `' + + fieldKey + + '` is `undefined`, but the GraphQL query expects a ' + + expected + + ' for this field.', + 13 + ); + } } - } - if ( + continue; // Skip this field + } else if ( // Skip typename fields and assume they've already been written above fieldName === '__typename' || // Fields marked as deferred that aren't defined must be skipped From 072d1cb9e0973ea8d3b5689f06e7134ac7381729 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 12 Jun 2023 21:02:32 +0100 Subject: [PATCH 2/4] Update tests and order of writeSelection checks --- .../graphcache/src/operations/query.test.ts | 2 -- .../graphcache/src/operations/write.test.ts | 6 ++-- exchanges/graphcache/src/operations/write.ts | 31 ++++++++++--------- 3 files changed, 19 insertions(+), 20 deletions(-) diff --git a/exchanges/graphcache/src/operations/query.test.ts b/exchanges/graphcache/src/operations/query.test.ts index 6c99ddf447..6e48bd99e0 100644 --- a/exchanges/graphcache/src/operations/query.test.ts +++ b/exchanges/graphcache/src/operations/query.test.ts @@ -164,8 +164,6 @@ describe('Query', () => { todos: [{ __typename: 'Todo', id: '0', text: 'Solve bug' }], }); - // The warning should be called for `__typename` - expect(console.warn).toHaveBeenCalledTimes(1); expect(console.error).not.toHaveBeenCalled(); }); diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts index 13c03732f4..c0f0e5efd5 100644 --- a/exchanges/graphcache/src/operations/write.test.ts +++ b/exchanges/graphcache/src/operations/write.test.ts @@ -155,7 +155,7 @@ describe('Query', () => { ); }); - it.only('should skip undefined values that are expected', () => { + it('should skip undefined values that are expected', () => { const query = gql` { field @@ -165,10 +165,10 @@ describe('Query', () => { // This should not overwrite the field write(store, { query }, { field: undefined } as any); // Because of us writing an undefined field - expect(console.warn).toHaveBeenCalledTimes(2); + expect(console.warn).toHaveBeenCalledTimes(1); expect((console.warn as any).mock.calls[0][0]).toMatch( - /The field `field` does not exist on `Query`/ + /Invalid undefined: The field at `field`/ ); write(store, { query }, { field: 'test' } as any); diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 488b7460e9..1ff76e105d 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -239,13 +239,17 @@ const writeSelection = ( const fieldAlias = getFieldAlias(node); let fieldValue = data[ctx.optimistic ? fieldName : fieldAlias]; - // Development check of undefined fields - if (rootField === 'query' && fieldValue === undefined && !deferRef) { + if ( + // Skip typename fields and assume they've already been written above + fieldName === '__typename' || + // Fields marked as deferred that aren't defined must be skipped + // Otherwise, we also ignore undefined values in optimistic updaters + (fieldValue === undefined && + (deferRef || (ctx.optimistic && rootField === 'query'))) + ) { + continue; + } else if (rootField === 'query' && fieldValue === undefined && !deferRef) { if (process.env.NODE_ENV !== 'production') { - if (ctx.store.schema && typename && fieldName !== '__typename') { - isFieldAvailableOnType(ctx.store.schema, typename, fieldName); - } - if (!entityKey || !InMemoryData.hasField(entityKey, fieldKey)) { const expected = node.selectionSet === undefined @@ -264,15 +268,12 @@ const writeSelection = ( } continue; // Skip this field - } else if ( - // Skip typename fields and assume they've already been written above - fieldName === '__typename' || - // Fields marked as deferred that aren't defined must be skipped - // Otherwise, we also ignore undefined values in optimistic updaters - (fieldValue === undefined && - (deferRef || (ctx.optimistic && rootField === 'query'))) - ) { - continue; + } + + if (process.env.NODE_ENV !== 'production') { + if (ctx.store.schema && typename && fieldName !== '__typename') { + isFieldAvailableOnType(ctx.store.schema, typename, fieldName); + } } // Add the current alias to the walked path before processing the field's value From f27453501a2e786e52c09dd2da6b2edd3c7001e7 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 12 Jun 2023 22:43:53 +0100 Subject: [PATCH 3/4] Update fieldValue check to run after optimistic update --- .../graphcache/src/operations/write.test.ts | 4 +- exchanges/graphcache/src/operations/write.ts | 42 ++++++++++--------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/exchanges/graphcache/src/operations/write.test.ts b/exchanges/graphcache/src/operations/write.test.ts index c0f0e5efd5..d2b0a5fcb2 100644 --- a/exchanges/graphcache/src/operations/write.test.ts +++ b/exchanges/graphcache/src/operations/write.test.ts @@ -165,9 +165,9 @@ describe('Query', () => { // This should not overwrite the field write(store, { query }, { field: undefined } as any); // Because of us writing an undefined field - expect(console.warn).toHaveBeenCalledTimes(1); + expect(console.warn).toHaveBeenCalledTimes(2); - expect((console.warn as any).mock.calls[0][0]).toMatch( + expect((console.warn as any).mock.calls[1][0]).toMatch( /Invalid undefined: The field at `field`/ ); diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 1ff76e105d..420afc8280 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -248,26 +248,6 @@ const writeSelection = ( (deferRef || (ctx.optimistic && rootField === 'query'))) ) { continue; - } else if (rootField === 'query' && fieldValue === undefined && !deferRef) { - if (process.env.NODE_ENV !== 'production') { - if (!entityKey || !InMemoryData.hasField(entityKey, fieldKey)) { - const expected = - node.selectionSet === undefined - ? 'scalar (number, boolean, etc)' - : 'selection set'; - - warn( - 'Invalid undefined: The field at `' + - fieldKey + - '` is `undefined`, but the GraphQL query expects a ' + - expected + - ' for this field.', - 13 - ); - } - } - - continue; // Skip this field } if (process.env.NODE_ENV !== 'production') { @@ -296,6 +276,28 @@ const writeSelection = ( fieldValue = ensureData(resolver(fieldArgs || {}, ctx.store, ctx)); } + if (fieldValue === undefined) { + if (process.env.NODE_ENV !== 'production') { + if (!entityKey || !InMemoryData.hasField(entityKey, fieldKey)) { + const expected = + node.selectionSet === undefined + ? 'scalar (number, boolean, etc)' + : 'selection set'; + + warn( + 'Invalid undefined: The field at `' + + fieldKey + + '` is `undefined`, but the GraphQL query expects a ' + + expected + + ' for this field.', + 13 + ); + } + } + + continue; // Skip this field + } + if (node.selectionSet) { // Process the field and write links for the child entities that have been written if (entityKey && rootField === 'query') { From 023aed5260086969d686051462a0ec07547b4af4 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 12 Jun 2023 23:42:44 +0100 Subject: [PATCH 4/4] Allow optimistic results to be fully partial --- exchanges/graphcache/src/operations/write.ts | 29 ++++++++++++++++---- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts index 420afc8280..02a8f0ff3e 100644 --- a/exchanges/graphcache/src/operations/write.ts +++ b/exchanges/graphcache/src/operations/write.ts @@ -210,7 +210,13 @@ const writeSelection = ( const rootField = ctx.store.rootNames[entityKey!] || 'query'; const isRoot = !!ctx.store.rootNames[entityKey!]; - const typename = isRoot ? entityKey : data.__typename; + let typename = isRoot ? entityKey : data.__typename; + if (!typename && entityKey && ctx.optimistic) { + typename = InMemoryData.readRecord(entityKey, '__typename') as + | string + | undefined; + } + if (!typename) { warn( "Couldn't find __typename when writing.\n" + @@ -278,7 +284,11 @@ const writeSelection = ( if (fieldValue === undefined) { if (process.env.NODE_ENV !== 'production') { - if (!entityKey || !InMemoryData.hasField(entityKey, fieldKey)) { + if ( + !entityKey || + !InMemoryData.hasField(entityKey, fieldKey) || + (ctx.optimistic && !InMemoryData.readRecord(entityKey, '__typename')) + ) { const expected = node.selectionSet === undefined ? 'scalar (number, boolean, etc)' @@ -306,7 +316,10 @@ const writeSelection = ( ctx, getSelectionSet(node), ensureData(fieldValue), - key + key, + ctx.optimistic + ? InMemoryData.readLink(entityKey || typename, fieldKey) + : undefined ); InMemoryData.writeLink(entityKey || typename, fieldKey, link); } else { @@ -353,7 +366,8 @@ const writeField = ( ctx: Context, select: SelectionSet, data: null | Data | NullArray, - parentFieldKey?: string + parentFieldKey?: string, + prevLink?: Link ): Link | undefined => { if (Array.isArray(data)) { const newData = new Array(data.length); @@ -365,7 +379,8 @@ const writeField = ( ? joinKeys(parentFieldKey, `${i}`) : undefined; // Recursively write array data - const links = writeField(ctx, select, data[i], indexKey); + const prevIndex = prevLink != null ? prevLink[i] : undefined; + const links = writeField(ctx, select, data[i], indexKey, prevIndex); // 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 @@ -377,7 +392,9 @@ const writeField = ( return getFieldError(ctx) ? undefined : null; } - const entityKey = ctx.store.keyOfEntity(data); + const entityKey = + ctx.store.keyOfEntity(data) || + (typeof prevLink === 'string' ? prevLink : null); const typename = data.__typename; if (