Repository: urql-graphql/urql
Pull request #3109 (2023-03-27 17:54:01)

refactor(core): Prevent result cloning in Client
## Summary

This gets rid of the last `{ ...result }` cloning code that we had left in the `Client`.

## Set of changes

- Remove `{ ...result, stale: true }` in favour of direct assignment
- Prevent issuing additional `stale: true` result for own result



Patch:
diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts
index 9790e5fc..4819decb 100755
--- a/packages/core/src/client.ts
+++ b/packages/core/src/client.ts
@@ -631,20 +631,18 @@ export const Client: new (opts: ClientOptions) => Client = function Client(
         // Add `stale: true` flag when a new operation is sent for queries
         switchMap(result => {
           const value$ = fromValue(result);
-          return result.stale
+          return result.stale || result.hasNext
             ? value$
             : merge([
                 value$,
                 pipe(
                   operations.source,
-                  filter(
-                    op =>
-                      op.kind === 'query' &&
-                      op.key === operation.key &&
-                      op.context.requestPolicy !== 'cache-only'
-                  ),
+                  filter(op => op.key === operation.key),
                   take(1),
-                  map(() => ({ ...result, stale: true }))
+                  map(() => {
+                    result.stale = true;
+                    return result;
+                  })
                 ),
               ]);
         })
@@ -750,37 +748,29 @@ export const Client: new (opts: ClientOptions) => Client = function Client(
             active.set(operation.key, (source = makeResultSource(operation)));
           }
 
-          const isNetworkOperation =
-            operation.context.requestPolicy === 'cache-and-network' ||
-            operation.context.requestPolicy === 'network-only';
-          const replay = replays.get(operation.key);
+          source = pipe(
+            source,
+            onStart(() => {
+              dispatchOperation(operation);
+            })
+          );
 
-          if (operation.kind !== 'query' || !replay || isNetworkOperation) {
-            source = pipe(
-              source,
-              onStart(() => {
-                dispatchOperation(operation);
-              })
+          const replay = replays.get(operation.key);
+          if (
+            operation.kind === 'query' &&
+            replay &&
+            (replay.stale || replay.hasNext)
+          ) {
+            return pipe(
+              merge([
+                source,
+                pipe(
+                  fromValue(replay),
+                  filter(replay => replay === replays.get(operation.key))
+                ),
+              ]),
+              switchMap(fromValue)
             );
-          }
-
-          if (operation.kind === 'query' && replay) {
-            return merge([
-              source,
-              pipe(
-                fromValue(replay),
-                filter(replay => {
-                  if (replay === replays.get(operation.key)) {
-                    if (isNetworkOperation && !replay.hasNext)
-                      replay.stale = true;
-                    return true;
-                  } else {
-                    if (!isNetworkOperation) dispatchOperation(operation);
-                    return false;
-                  }
-                })
-              ),
-            ]);
           } else {
             return source;
           }


Test patch:
diff --git a/packages/core/src/client.test.ts b/packages/core/src/client.test.ts
index 847cd671..46e35ca8 100755
--- a/packages/core/src/client.test.ts
+++ b/packages/core/src/client.test.ts
@@ -709,14 +709,14 @@ describe('shared sources behavior', () => {
     expect(resultTwo).toHaveBeenCalledWith({
       data: 1,
       operation: queryOperation,
-      stale: false,
+      stale: true,
       hasNext: false,
     });
 
     vi.advanceTimersByTime(1);
 
     // With cache-first we don't expect a new operation to be issued
-    expect(resultTwo).toHaveBeenCalledTimes(1);
+    expect(resultTwo).toHaveBeenCalledTimes(2);
   });
 
   it('dispatches the correct request policy on subsequent sources', async () => {
@@ -834,10 +834,21 @@ describe('shared sources behavior', () => {
       hasNext: false,
     });
 
+    expect(resultOne).toHaveBeenCalledWith({
+      data: 1,
+      operation,
+      stale: true,
+      hasNext: false,
+    });
+
+    expect(resultTwo).toHaveBeenCalledTimes(1);
+    expect(resultOne).toHaveBeenCalledTimes(2);
+
     vi.advanceTimersByTime(1);
 
     // With network-only we expect a new operation to be issued, hence a new result
     expect(resultTwo).toHaveBeenCalledTimes(2);
+    expect(resultOne).toHaveBeenCalledTimes(3);
 
     expect(resultTwo).toHaveBeenCalledWith({
       data: 2,
@@ -845,6 +856,13 @@ describe('shared sources behavior', () => {
       stale: false,
       hasNext: false,
     });
+
+    expect(resultOne).toHaveBeenCalledWith({
+      data: 2,
+      operation,
+      stale: false,
+      hasNext: false,
+    });
   });
 
   it('does not replay values from a past subscription', async () => {