From 1bfadffe88f21dc95f5d0f4aa44c5abf9ac8e4ae Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Fri, 2 Feb 2024 14:31:56 +0000 Subject: [PATCH] fix(graphcache): Restore `stale` being set to `true` on blocked (but in-flight) operations (#3493) --- .changeset/giant-beans-act.md | 5 + exchanges/graphcache/e2e-tests/query.spec.tsx | 3 +- .../graphcache/src/cacheExchange.test.ts | 159 +++++++++++++++++- exchanges/graphcache/src/cacheExchange.ts | 11 +- exchanges/graphcache/src/store/data.ts | 5 + 5 files changed, 176 insertions(+), 7 deletions(-) create mode 100644 .changeset/giant-beans-act.md diff --git a/.changeset/giant-beans-act.md b/.changeset/giant-beans-act.md new file mode 100644 index 0000000000..7981f01794 --- /dev/null +++ b/.changeset/giant-beans-act.md @@ -0,0 +1,5 @@ +--- +'@urql/exchange-graphcache': patch +--- + +Set `stale: true` on cache results, even if a reexecution has been blocked by the loop protection, if the operation is already pending and in-flight. diff --git a/exchanges/graphcache/e2e-tests/query.spec.tsx b/exchanges/graphcache/e2e-tests/query.spec.tsx index 747b98cf9c..c882319564 100644 --- a/exchanges/graphcache/e2e-tests/query.spec.tsx +++ b/exchanges/graphcache/e2e-tests/query.spec.tsx @@ -31,7 +31,8 @@ const schema = buildSchema(` `); const rootValue = { - movie: () => { + movie: async () => { + await new Promise(resolve => setTimeout(resolve, 50)); return { id: 'foo', title: 'title', diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts index 9b4eff7142..f4b89baf27 100644 --- a/exchanges/graphcache/src/cacheExchange.test.ts +++ b/exchanges/graphcache/src/cacheExchange.test.ts @@ -6,8 +6,8 @@ import { OperationResult, CombinedError, } from '@urql/core'; -import { print, stripIgnoredCharacters } from 'graphql'; +import { print, stripIgnoredCharacters } from 'graphql'; import { vi, expect, it, describe } from 'vitest'; import { @@ -2197,6 +2197,156 @@ describe('schema awareness', () => { }); }); +describe('looping protection', () => { + it('applies stale to blocked looping queries', () => { + let normalData: OperationResult | undefined; + let extendedData: OperationResult | undefined; + + const client = createClient({ + url: 'http://0.0.0.0', + exchanges: [], + }); + + const { source: ops$, next: nextOp } = makeSubject(); + const { source: res$, next: nextRes } = makeSubject(); + + vi.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp); + + const normalQuery = gql` + { + __typename + item { + __typename + id + } + } + `; + + const extendedQuery = gql` + { + __typename + item { + __typename + extended: id + extra @_optional + } + } + `; + + const forward = (ops$: Source): Source => + share( + merge([ + pipe( + ops$, + filter(() => false) + ) as any, + res$, + ]) + ); + + pipe( + cacheExchange()({ forward, client, dispatchDebug })(ops$), + tap(result => { + if (result.operation.kind === 'query') { + if (result.operation.key === 1) { + normalData = result; + } else if (result.operation.key === 2) { + extendedData = result; + } + } + }), + publish + ); + + const normalOp = client.createRequestOperation( + 'query', + { + key: 1, + query: normalQuery, + variables: undefined, + }, + { + requestPolicy: 'cache-first', + } + ); + + const extendedOp = client.createRequestOperation( + 'query', + { + key: 2, + query: extendedQuery, + variables: undefined, + }, + { + requestPolicy: 'cache-first', + } + ); + + nextOp(normalOp); + + nextRes({ + operation: normalOp, + data: { + __typename: 'Query', + item: { + __typename: 'Node', + id: 'id', + }, + }, + stale: false, + hasNext: false, + }); + + expect(normalData).toMatchObject({ stale: false }); + expect(client.reexecuteOperation).toHaveBeenCalledTimes(0); + + nextOp(extendedOp); + + expect(extendedData).toMatchObject({ stale: true }); + expect(client.reexecuteOperation).toHaveBeenCalledTimes(1); + + // Out of band re-execute first operation + nextOp(normalOp); + nextRes({ + ...queryResponse, + operation: normalOp, + data: { + __typename: 'Query', + item: { + __typename: 'Node', + id: 'id', + }, + }, + }); + + expect(normalData).toMatchObject({ stale: false }); + expect(extendedData).toMatchObject({ stale: true }); + expect(client.reexecuteOperation).toHaveBeenCalledTimes(3); + + nextOp(extendedOp); + + expect(normalData).toMatchObject({ stale: false }); + expect(extendedData).toMatchObject({ stale: true }); + expect(client.reexecuteOperation).toHaveBeenCalledTimes(3); + + nextRes({ + ...queryResponse, + operation: extendedOp, + data: { + __typename: 'Query', + item: { + __typename: 'Node', + extended: 'id', + extra: 'extra', + }, + }, + }); + + expect(extendedData).toMatchObject({ stale: false }); + expect(client.reexecuteOperation).toHaveBeenCalledTimes(4); + }); +}); + describe('commutativity', () => { it('applies results that come in out-of-order commutatively and consistently', () => { vi.useFakeTimers(); @@ -2873,8 +3023,7 @@ describe('commutativity', () => { }); const { source: ops$, next: nextOp } = makeSubject(); const { source: res$, next: nextRes } = makeSubject(); - - vi.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp); + client.reexecuteOperation = nextOp; const normalQuery = gql` { @@ -2911,11 +3060,11 @@ describe('commutativity', () => { } `; - const forward = (ops$: Source): Source => + const forward = (operations$: Source): Source => share( merge([ pipe( - ops$, + operations$, filter(() => false) ) as any, res$, diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts index 1955286524..260ca9db47 100644 --- a/exchanges/graphcache/src/cacheExchange.ts +++ b/exchanges/graphcache/src/cacheExchange.ts @@ -32,6 +32,7 @@ import { noopDataState, hydrateData, reserveLayer, + hasLayer, } from './store/data'; interface OperationResultWithMeta extends Partial { @@ -377,6 +378,14 @@ export const cacheExchange = (requestPolicy === 'cache-first' && res.outcome === 'partial' && !reexecutingOperations.has(res.operation.key))); + // Set stale to true anyway, even if the reexecute will be blocked, if the operation + // is in progress. We can be reasonably sure of that if a layer has been reserved for it. + const stale = + requestPolicy !== 'cache-only' && + (shouldReexecute || + (res.outcome === 'partial' && + reexecutingOperations.has(res.operation.key) && + hasLayer(store.data, res.operation.key))); const result: OperationResult = { operation: addMetadata(res.operation, { @@ -385,7 +394,7 @@ export const cacheExchange = data: res.data, error: res.error, extensions: res.extensions, - stale: shouldReexecute && !res.hasNext, + stale: stale && !res.hasNext, hasNext: shouldReexecute && res.hasNext, }; diff --git a/exchanges/graphcache/src/store/data.ts b/exchanges/graphcache/src/store/data.ts index 7c3badefa8..174610a1a7 100644 --- a/exchanges/graphcache/src/store/data.ts +++ b/exchanges/graphcache/src/store/data.ts @@ -528,6 +528,11 @@ export const reserveLayer = ( data.commutativeKeys.add(layerKey); }; +/** Checks whether a given layer exists */ +export const hasLayer = (data: InMemoryData, layerKey: number) => + data.commutativeKeys.has(layerKey) || + data.optimisticOrder.indexOf(layerKey) > -1; + /** Creates an optimistic layer of links and records */ const createLayer = (data: InMemoryData, layerKey: number) => { if (data.optimisticOrder.indexOf(layerKey) === -1) {