Repository: FormidableLabs/urql (ts)

Issue:
Query does not refetch when variables change
After upgrading to the latest @urql/core, queries stopped refetching when variables change

Pull request description:
Fix cache-and-network/stale on client.reexecuteOperation queue
Fix #668 

## Summary

This fixes an edge case that was missed in #662.

The queue wasn't marked as being started by the first `client.dispatchOperation`, which means that the queue is then emptied by the first `client.rexecuteOperation` call instead. This doesn't cause any issues usually, but with the `dedupExchange` we'll accidentally deduplicate reissued operations, which are usually used to implement fetching for `stale` results or `cache-and-network` operations.

## Set of changes

- Fix queuing behavior in `client.dispatchOperation`
- Add test for operation & result ordering
- Remove `client.exchange` (private variable)


Patch:
diff --git a/.changeset/curly-moons-kick.md b/.changeset/curly-moons-kick.md
new file mode 100644
index 00000000..267d09a9
--- /dev/null
+++ b/.changeset/curly-moons-kick.md
@@ -0,0 +1,5 @@
+---
+'@urql/core': patch
+---
+
+Fix oversight in edge case for #662. The operation queue wasn't marked as being active which caused `stale` results and `cache-and-network` operations from reissuing operations immediately (unqueued essentially) which would then be filtered out by the `dedupExchange`.
diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts
index 22dae1d7..c7b46c4a 100755
--- a/packages/core/src/client.ts
+++ b/packages/core/src/client.ts
@@ -77,7 +77,6 @@ export class Client {
   url: string;
   fetch?: typeof fetch;
   fetchOptions?: RequestInit | (() => RequestInit);
-  exchange: Exchange;
   suspense: boolean;
   preferGetMethod: boolean;
   requestPolicy: RequestPolicy;
@@ -112,26 +111,27 @@ export class Client {
 
     let isDispatching = false;
     this.dispatchOperation = (operation?: Operation) => {
-      if (operation) nextOperation(operation);
       if (!isDispatching) {
         isDispatching = true;
+        if (operation) nextOperation(operation);
         let queued: Operation | void;
         while ((queued = this.queue.shift())) nextOperation(queued);
         isDispatching = false;
+      } else if (operation) {
+        nextOperation(operation);
       }
     };
 
     const exchanges =
       opts.exchanges !== undefined ? opts.exchanges : defaultExchanges;
-
     // All exchange are composed into a single one and are called using the constructed client
     // and the fallback exchange stream
-    this.exchange = composeExchanges(exchanges);
+    const exchange = composeExchanges(exchanges);
 
     // All operations run through the exchanges in a pipeline-like fashion
     // and this observable then combines all their results
     this.results$ = share(
-      this.exchange({
+      exchange({
         client: this,
         forward: fallbackExchangeIO,
       })(this.operations$)


Test patch:
diff --git a/packages/core/src/__snapshots__/client.test.ts.snap b/packages/core/src/__snapshots__/client.test.ts.snap
index a22248b1..e8d83e74 100644
--- a/packages/core/src/__snapshots__/client.test.ts.snap
+++ b/packages/core/src/__snapshots__/client.test.ts.snap
@@ -6,7 +6,6 @@ Client {
   "createOperationContext": [Function],
   "createRequestOperation": [Function],
   "dispatchOperation": [Function],
-  "exchange": [Function],
   "executeMutation": [Function],
   "executeQuery": [Function],
   "executeSubscription": [Function],
diff --git a/packages/core/src/client.test.ts b/packages/core/src/client.test.ts
index 80f1862e..e1d2114a 100755
--- a/packages/core/src/client.test.ts
+++ b/packages/core/src/client.test.ts
@@ -3,8 +3,10 @@ import gql from 'graphql-tag';
 
 /** NOTE: Testing in this file is designed to test both the client and it's interaction with default Exchanges */
 
-import { map, pipe, subscribe, tap } from 'wonka';
+import { map, pipe, subscribe, filter, toArray, tap } from 'wonka';
+import { Exchange, Operation, OperationResult } from './types';
 import { createClient } from './client';
+import { queryOperation } from './test-utils';
 
 const url = 'https://hostname.com';
 
@@ -302,3 +304,76 @@ describe('executeSubscription', () => {
     expect(receivedOps[0]).toHaveProperty('operationName', 'subscription');
   });
 });
+
+describe('queuing behavior', () => {
+  it('queues reexecuteOperation, which dispatchOperation consumes', () => {
+    const output: Array<Operation | OperationResult> = [];
+
+    const exchange: Exchange = ({ client }) => ops$ => {
+      return pipe(
+        ops$,
+        filter(op => op.operationName !== 'teardown'),
+        tap(op => {
+          output.push(op);
+          if (
+            op.key === queryOperation.key &&
+            op.context.requestPolicy === 'cache-first'
+          ) {
+            client.reexecuteOperation({
+              ...op,
+              context: {
+                ...op.context,
+                requestPolicy: 'network-only',
+              },
+            });
+          }
+        }),
+        map(op => ({
+          data: op.key,
+          operation: op,
+        }))
+      );
+    };
+
+    const client = createClient({
+      url: 'test',
+      exchanges: [exchange],
+    });
+
+    pipe(
+      client.results$,
+      subscribe(result => {
+        output.push(result);
+      })
+    );
+
+    const results = pipe(
+      client.executeRequestOperation(queryOperation),
+      toArray
+    );
+
+    expect(output.length).toBe(4);
+    expect(results.length).toBe(2);
+
+    expect(output[0]).toHaveProperty('key', queryOperation.key);
+    expect(output[0]).toHaveProperty('context.requestPolicy', 'cache-first');
+
+    expect(output[1]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[1]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-first'
+    );
+
+    expect(output[2]).toHaveProperty('key', queryOperation.key);
+    expect(output[2]).toHaveProperty('context.requestPolicy', 'network-only');
+
+    expect(output[3]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[3]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'network-only'
+    );
+
+    expect(output[1]).toBe(results[0]);
+    expect(output[3]).toBe(results[1]);
+  });
+});
diff --git a/packages/next-urql/src/__tests__/with-urql-client.spec.tsx b/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
index 453a545d..d624285e 100644
--- a/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
+++ b/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
@@ -1,7 +1,7 @@
 import React from 'react';
 import { shallow, configure } from 'enzyme';
 import Adapter from 'enzyme-adapter-react-16';
-import { Client, defaultExchanges, composeExchanges } from 'urql';
+import { Client, defaultExchanges } from 'urql';
 
 import { withUrqlClient, NextUrqlPageContext } from '..';
 import * as init from '../init-urql-client';
@@ -122,9 +122,6 @@ describe('withUrqlClient', () => {
       const app = tree.find(MockApp);
 
       expect(app.props().urqlClient).toBeInstanceOf(Client);
-      expect(app.props().urqlClient.exchange.toString()).toEqual(
-        composeExchanges(defaultExchanges).toString()
-      );
       expect(mockMergeExchanges).toHaveBeenCalledTimes(1);
     });
   });