fix(graphcache): Restore stale being set to true on blocked (but …

…in-flight) operations (#3493)
urql-graphql · Feb 2, 2024 · 1bfadff · 1bfadff
1 parent f7b78e2
commit 1bfadff
Show file tree

Hide file tree

Showing 5 changed files with 176 additions and 7 deletions.
diff --git a/.changeset/giant-beans-act.md 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
@@ -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
@@ -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<Operation>();
+    const { source: res$, next: nextRes } = makeSubject<OperationResult>();
+
+    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<Operation>): Source<OperationResult> =>
+      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<Operation>();
     const { source: res$, next: nextRes } = makeSubject<OperationResult>();
-
-    vi.spyOn(client, 'reexecuteOperation').mockImplementation(nextOp);
+    client.reexecuteOperation = nextOp;
 
     const normalQuery = gql`
       {
@@ -2911,11 +3060,11 @@ describe('commutativity', () => {
       }
     `;
 
-    const forward = (ops$: Source<Operation>): Source<OperationResult> =>
+    const forward = (operations$: Source<Operation>): Source<OperationResult> =>
       share(
         merge([
           pipe(
-            ops$,
+            operations$,
             filter(() => false)
           ) as any,
           res$,

diff --git 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<OperationResult> {
@@ -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
@@ -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) {