Repository: urql-graphql/urql
Pull request #3076 (2023-03-17 16:34:29)

fix(core): Fix immediate cancellation of fetch requests
Follow-up to #3043

## Summary

The async generator implementation has regressed on the previous one since it doesn't include a delay that respects immediate `teardown` signals. This is because no `yield` was present after the initial microtick and before the `fetch` call.

I'm still trying to find a more elegant solution to this, but it doesn't seem possible to yield without emitting a value, and async generators may not abort on `await` statements, which seems counterintuitive.

Changeset hasn't been added since it's a follow-up fix.

## Set of changes

- Add `yield` after microtick delay to allow `fetchOperation` to abort early
- Update tests to correctly detect missing early-terminations



Patch:
diff --git a/packages/core/src/internal/fetchSource.ts b/packages/core/src/internal/fetchSource.ts
index 4345a22b..ca8df035 100644
--- a/packages/core/src/internal/fetchSource.ts
+++ b/packages/core/src/internal/fetchSource.ts
@@ -1,4 +1,4 @@
-import { Source, fromAsyncIterable } from 'wonka';
+import { Source, fromAsyncIterable, filter, pipe } from 'wonka';
 import { Operation, OperationResult, ExecutionResult } from '../types';
 import { makeResult, makeErrorResult, mergeResultPatch } from '../utils';
 
@@ -116,7 +116,7 @@ async function* fetchOperation(
 
     // Delay for a tick to give the Client a chance to cancel the request
     // if a teardown comes in immediately
-    await Promise.resolve();
+    yield await Promise.resolve();
 
     response = await (operation.context.fetch || fetch)(url, fetchOptions);
     const contentType = response.headers.get('Content-Type') || '';
@@ -194,5 +194,8 @@ export function makeFetchSource(
   url: string,
   fetchOptions: RequestInit
 ): Source<OperationResult> {
-  return fromAsyncIterable(fetchOperation(operation, url, fetchOptions));
+  return pipe(
+    fromAsyncIterable(fetchOperation(operation, url, fetchOptions)),
+    filter((result): result is OperationResult => !!result)
+  );
 }


Test patch:
diff --git a/packages/core/src/exchanges/fetch.test.ts b/packages/core/src/exchanges/fetch.test.ts
index 3c739aa8..76934e4c 100755
--- a/packages/core/src/exchanges/fetch.test.ts
+++ b/packages/core/src/exchanges/fetch.test.ts
@@ -147,8 +147,12 @@ describe('on teardown', () => {
     expect(true).toEqual(false);
   };
 
-  it('does not start the outgoing request on immediate teardowns', () => {
-    fetch.mockResolvedValue(new Response('text', { status: 200 }));
+  it('does not start the outgoing request on immediate teardowns', async () => {
+    fetch.mockImplementation(async () => {
+      await new Promise(() => {
+        /*noop*/
+      });
+    });
 
     const { unsubscribe } = pipe(
       fromValue(queryOperation),
@@ -157,27 +161,35 @@ describe('on teardown', () => {
     );
 
     unsubscribe();
+
+    // NOTE: We can only observe the async iterator's final run after a macro tick
+    await new Promise(resolve => setTimeout(resolve));
     expect(fetch).toHaveBeenCalledTimes(0);
-    expect(abort).toHaveBeenCalledTimes(0);
+    expect(abort).toHaveBeenCalledTimes(1);
   });
 
   it('aborts the outgoing request', async () => {
-    fetch.mockResolvedValue(new Response('text', { status: 200 }));
+    fetch.mockResolvedValue({
+      status: 200,
+      headers: new Map([['Content-Type', 'application/json']]),
+      text: vi.fn().mockResolvedValue('{ "data": null }'),
+    });
 
     const { unsubscribe } = pipe(
       fromValue(queryOperation),
       fetchExchange(exchangeArgs),
-      subscribe(fail)
+      subscribe(() => {
+        /*noop*/
+      })
     );
 
-    await Promise.resolve();
-
+    await new Promise(resolve => setTimeout(resolve));
     unsubscribe();
 
     // NOTE: We can only observe the async iterator's final run after a macro tick
     await new Promise(resolve => setTimeout(resolve));
     expect(fetch).toHaveBeenCalledTimes(1);
-    expect(abort).toHaveBeenCalled();
+    expect(abort).toHaveBeenCalledTimes(1);
   });
 
   it('does not call the query', () => {
diff --git a/packages/core/src/internal/fetchSource.test.ts b/packages/core/src/internal/fetchSource.test.ts
index 0aa40e24..c22b614c 100644
--- a/packages/core/src/internal/fetchSource.test.ts
+++ b/packages/core/src/internal/fetchSource.test.ts
@@ -164,8 +164,12 @@ describe('on teardown', () => {
     expect(true).toEqual(false);
   };
 
-  it('does not start the outgoing request on immediate teardowns', () => {
-    fetch.mockResolvedValue(new Response('text', { status: 200 }));
+  it('does not start the outgoing request on immediate teardowns', async () => {
+    fetch.mockImplementation(async () => {
+      await new Promise(() => {
+        /*noop*/
+      });
+    });
 
     const { unsubscribe } = pipe(
       makeFetchSource(queryOperation, 'https://test.com/graphql', {}),
@@ -173,19 +177,29 @@ describe('on teardown', () => {
     );
 
     unsubscribe();
+
+    // NOTE: We can only observe the async iterator's final run after a macro tick
+
+    await new Promise(resolve => setTimeout(resolve));
     expect(fetch).toHaveBeenCalledTimes(0);
-    expect(abort).toHaveBeenCalledTimes(0);
+    expect(abort).toHaveBeenCalledTimes(1);
   });
 
   it('aborts the outgoing request', async () => {
-    fetch.mockResolvedValue(new Response('text', { status: 200 }));
+    fetch.mockResolvedValue({
+      status: 200,
+      headers: new Map([['Content-Type', 'application/json']]),
+      text: vi.fn().mockResolvedValue('{ "data": null }'),
+    });
 
     const { unsubscribe } = pipe(
       makeFetchSource(queryOperation, 'https://test.com/graphql', {}),
-      subscribe(fail)
+      subscribe(() => {
+        /*noop*/
+      })
     );
 
-    await Promise.resolve();
+    await new Promise(resolve => setTimeout(resolve));
     unsubscribe();
 
     // NOTE: We can only observe the async iterator's final run after a macro tick