Repository: FormidableLabs/urql (ts)

Issue:
RFC: Stale to be "true" if any background fetching is happening
## Summary

Stale to be "true" if any background fetching is happening

## Proposed Solution

Hey!

First of all, thanks for the awesome library. Currently, I lead a team that migrates big application from Apollo to urql, and I have very high hopes that this is a correct decision.

Most of our needs are covered by what urql has to offer at the moment, except for one. Comparing to Apollo, urql provides very limited information about current state of the rendered query (e.g. **networkStatus**). I've carefully read reasoning in this [thread](https://github.com/FormidableLabs/urql/issues/417), and can definitely see the logic in an overall strategy, however few details are messing the whole picture for me right now.

Our app mostly consists of different tables with data. User can view them, and modify / delete any row using dropdown menu. When that happens, we run delete mutation, invalidate deleted entity in the cache using `cache.invalidate` in updater, and wait for query that populates the table to be refetched.

Unfortunately, I have not found a way to us to know if that query is refetched. You may suggest for us to not only invalidate an entity, but also update query result in cache manually, but in our case we can't make that due to side-effects on delete. The easiest & safest way would to just show kind of "Syncing..." status in the table until data is freshly fetched.

Same goes for cases when variables change. Often we have a header in the table with range picker or toggles that modify query variables. Query can take few seconds to resolve after change, and whole this time users has no visual feedback to their actions.

I think it can be handled (probably) by custom hooks, maybe exchanges and stuff like that... But from what I see now, this library aims to become a real competitor for Apollo. However, in my opinion, it will not happen until such fundamental functionality has to be hacked around (sorry if I sound rude).

 
## Requirements

I agree that `fetching` flag should only be responsible for explicitly called action.

So it will be only logical that `stale` flag should indicate _staleness of the data_ - which is by dictionary definition **stale** in case query variables change or data becomes invalidated in the cache manually. That way, it would be explicit & straightforward - you will always know if the query is being refetched if you need that info, if no - you just use `fetching`.

Pull request:
(core) - Reemit results as stale if they're refetched in the background
Resolve #1372 

## Summary

Usually we have a couple of refetches that are emitted as `cache-and-network`. In these cases the cache exchanges (default or Graphcache) take care of emitting a first result with the `stale` flag set. In other cases we may have a subsequent `network-only` reexecution which happens in the background. In these cases no part of the exchange pipeline or the `Client` took responsibility of emitting a `stale` result.

This PR changes this behaviour to allows the `Client`'s `executeRequestOperation` stream to add-on a single `stale` result onto a prior result if it detects a `network-only` operation.

- Has been tested in `threed-web` against Graphcache's `cache.invalidate` which triggers a `network-only` refetch.
- This has also been fixed in 5c1b1fd to be applied to all operations that are not `cache-only`

## Set of changes

- Add a `switchMap` add-on stream per result that reemits the result as `stale` if it detects a `network-only` operation.
  - Only emit this `stale` add-on result when the previous result wasn't already `stale`
  - Ensure that this won't emit when the cache responds synchronously
  - Apply this to `cache-and-network` and `cache-first` (with cache misses) too
  - Emit this at most once since we expect a result afterwards

Patch:
diff --git a/.changeset/plenty-carrots-prove.md b/.changeset/plenty-carrots-prove.md
new file mode 100644
index 00000000..9cb1a292
--- /dev/null
+++ b/.changeset/plenty-carrots-prove.md
@@ -0,0 +1,10 @@
+---
+'@urql/core': major
+'@urql/preact': minor
+'@urql/svelte': minor
+'@urql/vue': minor
+'urql': minor
+---
+
+Remove deprecated `operationName` property from `Operation`s. The new `Operation.kind` property is now preferred. If you're creating new operations you may also use the `makeOperation` utility instead.
+When upgrading `@urql/core` please ensure that your package manager didn't install any duplicates of it. You may deduplicate it manually using `npx yarn-deduplicate` (for Yarn) or `npm dedupe` (for npm).
diff --git a/.changeset/strong-emus-confess.md b/.changeset/strong-emus-confess.md
new file mode 100644
index 00000000..4bb537ee
--- /dev/null
+++ b/.changeset/strong-emus-confess.md
@@ -0,0 +1,5 @@
+---
+'@urql/core': minor
+---
+
+Reemit an `OperationResult` as `stale: true` if it's being reexecuted as `network-only` operation to give bindings immediate feedback on background refetches.
diff --git a/exchanges/graphcache/src/operations/write.ts b/exchanges/graphcache/src/operations/write.ts
index fbc44105..2e2244e3 100644
--- a/exchanges/graphcache/src/operations/write.ts
+++ b/exchanges/graphcache/src/operations/write.ts
@@ -94,23 +94,23 @@ export const startWrite = (
 ) => {
   const operation = getMainOperation(request.query);
   const result: WriteResult = { data, dependencies: getCurrentDependencies() };
-  const operationName = store.rootFields[operation.operation];
+  const kind = store.rootFields[operation.operation];
 
   const ctx = makeContext(
     store,
     normalizeVariables(operation, request.variables),
     getFragments(request.query),
-    operationName,
-    operationName,
+    kind,
+    kind,
     !!isOptimistic,
     error
   );
 
   if (process.env.NODE_ENV !== 'production') {
-    pushDebugNode(operationName, operation);
+    pushDebugNode(kind, operation);
   }
 
-  writeSelection(ctx, operationName, getSelectionSet(operation), data);
+  writeSelection(ctx, kind, getSelectionSet(operation), data);
 
   if (process.env.NODE_ENV !== 'production') {
     popDebugNode();
diff --git a/packages/core/src/client.ts b/packages/core/src/client.ts
index 9b90a27e..827bbd23 100755
--- a/packages/core/src/client.ts
+++ b/packages/core/src/client.ts
@@ -254,9 +254,31 @@ export class Client {
       )
     );
 
+    const refetch$ = pipe(
+      this.operations$,
+      filter(
+        (op: Operation) =>
+          op.kind === operation.kind &&
+          op.key === operation.key &&
+          op.context.requestPolicy !== 'cache-only'
+      )
+    );
+
     const result$ = pipe(
       operationResults$,
       takeUntil(teardown$),
+      switchMap(result => {
+        if (result.stale) return fromValue(result);
+
+        return merge([
+          fromValue(result),
+          pipe(
+            refetch$,
+            take(1),
+            map(() => ({ ...result, stale: true }))
+          ),
+        ]);
+      }),
       onStart<OperationResult>(() => {
         this.onOperationStart(operation);
       }),
diff --git a/packages/core/src/types.ts b/packages/core/src/types.ts
index 2aea604a..a61dea9c 100644
--- a/packages/core/src/types.ts
+++ b/packages/core/src/types.ts
@@ -57,9 +57,6 @@ export interface Operation<Data = any, Variables = any>
   extends GraphQLRequest<Data, Variables> {
   readonly kind: OperationType;
   context: OperationContext;
-
-  /** @deprecated use Operation.kind instead */
-  readonly operationName: OperationType;
 }
 
 /** Resulting data from an [operation]{@link Operation}. */
diff --git a/packages/core/src/utils/deprecation.ts b/packages/core/src/utils/deprecation.ts
deleted file mode 100644
index f1abf4e8..00000000
--- a/packages/core/src/utils/deprecation.ts
+++ /dev/null
@@ -1,29 +0,0 @@
-export interface Warning {
-  key: string;
-  message: string;
-}
-
-/**
- * Module-scoped state to track if deprecation warnings have already been issued
- * for a particular key.
- */
-let issuedWarnings: Record<string, boolean> = {};
-
-/**
- * If a deprecation warning has not already been issued, use `console.warn()` to
- * issue it with an eye-catching prefix string.
- */
-export const deprecationWarning = ({ key, message }: Warning) => {
-  if (!issuedWarnings[key]) {
-    console.warn(`[WARNING: Deprecated] ${message}`);
-
-    issuedWarnings[key] = true;
-  }
-};
-
-/**
- * Clears all issued warnings - intended for use in testing.
- */
-export const _clearWarnings = () => {
-  issuedWarnings = {};
-};
diff --git a/packages/core/src/utils/index.ts b/packages/core/src/utils/index.ts
index ee2437c4..bba813b8 100644
--- a/packages/core/src/utils/index.ts
+++ b/packages/core/src/utils/index.ts
@@ -6,7 +6,6 @@ export * from './stringifyVariables';
 export * from './maskTypename';
 export * from './withPromise';
 export * from './operation';
-export * from './deprecation';
 
 export const noop = () => {
   /* noop */
diff --git a/packages/core/src/utils/operation.ts b/packages/core/src/utils/operation.ts
index 70a7bdfe..78eb157c 100644
--- a/packages/core/src/utils/operation.ts
+++ b/packages/core/src/utils/operation.ts
@@ -4,16 +4,6 @@ import {
   OperationContext,
   OperationType,
 } from '../types';
-import { Warning, deprecationWarning } from './deprecation';
-
-// TODO: Remove when the deprecated `operationName` property is removed
-const DEPRECATED: Record<string, Warning> = {
-  operationName: {
-    key: 'Operation.operationName',
-    message:
-      'The "Operation.operationName" property has been deprecated and will be removed in a future release of urql. Use "Operation.kind" instead.',
-  },
-};
 
 function makeOperation<Data = any, Variables = object>(
   kind: OperationType,
@@ -36,12 +26,6 @@ function makeOperation(kind, request, context) {
     variables: request.variables,
     kind,
     context,
-
-    get operationName(): OperationType {
-      deprecationWarning(DEPRECATED.operationName);
-
-      return this.kind;
-    },
   };
 }
 
diff --git a/scripts/jest/setup.js b/scripts/jest/setup.js
index ab9c0181..78436ea7 100644
--- a/scripts/jest/setup.js
+++ b/scripts/jest/setup.js
@@ -17,13 +17,3 @@ global.console = {
 jest.spyOn(console, 'log');
 jest.spyOn(console, 'warn');
 jest.spyOn(console, 'error');
-
-jest.mock('../../packages/core/src/utils/deprecation.ts', () => ({
-  deprecationWarning({ message }) {
-    const error = new Error(`Deprecation Warnings are thrown in tests.\n${message}`);
-    if (!/jest-snapshot|pretty-format|jest-jasmine2/i.test(error.stack)) {
-      throw error;
-    }
-  },
-  _clearWarnings() {},
-}));


Test patch:
diff --git a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap
index d8a6ac57..34b8d666 100644
--- a/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap
+++ b/exchanges/multipart-fetch/src/__snapshots__/multipartFetchExchange.test.ts.snap
@@ -15,7 +15,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -185,7 +184,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -357,7 +355,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -531,7 +528,6 @@ Object {
     },
     "key": 3,
     "kind": "mutation",
-    "operationName": "mutation",
     "query": Object {
       "__key": 3781860981,
       "definitions": Array [
@@ -681,7 +677,6 @@ Object {
     },
     "key": 3,
     "kind": "mutation",
-    "operationName": "mutation",
     "query": Object {
       "__key": 1193185401,
       "definitions": Array [
diff --git a/exchanges/refocus/src/refocusExchange.test.ts b/exchanges/refocus/src/refocusExchange.test.ts
index e90777d1..d8bbdf64 100644
--- a/exchanges/refocus/src/refocusExchange.test.ts
+++ b/exchanges/refocus/src/refocusExchange.test.ts
@@ -88,8 +88,5 @@ it(`attaches a listener and redispatches queries on call`, () => {
     key: 1,
     query: queryOne,
     kind: 'query',
-
-    // TODO: Remove this when the deprecated "operationName" property is removed
-    operationName: 'query',
   });
 });
diff --git a/packages/core/src/client.test.ts b/packages/core/src/client.test.ts
index 809db44d..3f1a32ce 100755
--- a/packages/core/src/client.test.ts
+++ b/packages/core/src/client.test.ts
@@ -2,9 +2,19 @@ import { print } from 'graphql';
 
 /** NOTE: Testing in this file is designed to test both the client and its interaction with default Exchanges */
 
-import { Source, map, pipe, subscribe, filter, toArray, tap } from 'wonka';
+import {
+  Source,
+  delay,
+  map,
+  pipe,
+  subscribe,
+  filter,
+  toArray,
+  tap,
+} from 'wonka';
 import { gql } from './gql';
 import { Exchange, Operation, OperationResult } from './types';
+import { makeOperation } from './utils';
 import { createClient } from './client';
 import { queryOperation } from './test-utils';
 
@@ -151,9 +161,6 @@ describe('synchronous methods', () => {
         context: expect.anything(),
         key: expect.any(Number),
         kind: 'query',
-
-        // TODO: Remove this when the deprecated `operationName` property is removed
-        operationName: 'query',
       },
     });
   });
@@ -292,6 +299,14 @@ describe('executeSubscription', () => {
 });
 
 describe('queuing behavior', () => {
+  beforeEach(() => {
+    jest.useFakeTimers();
+  });
+
+  afterEach(() => {
+    jest.useRealTimers();
+  });
+
   it('queues reexecuteOperation, which dispatchOperation consumes', () => {
     const output: Array<Operation | OperationResult> = [];
 
@@ -362,4 +377,187 @@ describe('queuing behavior', () => {
     expect(output[1]).toBe(results[0]);
     expect(output[3]).toBe(results[1]);
   });
+
+  it('reemits previous results as stale if the operation is reexecuted as network-only', async () => {
+    const output: OperationResult[] = [];
+
+    const exchange: Exchange = () => {
+      let countRes = 0;
+      return ops$ => {
+        return pipe(
+          ops$,
+          filter(op => op.kind !== 'teardown'),
+          map(op => ({
+            data: ++countRes,
+            operation: op,
+          })),
+          delay(1)
+        );
+      };
+    };
+
+    const client = createClient({
+      url: 'test',
+      exchanges: [exchange],
+    });
+
+    const { unsubscribe } = pipe(
+      client.executeRequestOperation(queryOperation),
+      subscribe(result => {
+        output.push(result);
+      })
+    );
+
+    jest.advanceTimersByTime(1);
+
+    expect(output.length).toBe(1);
+    expect(output[0]).toHaveProperty('data', 1);
+    expect(output[0]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[0]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-first'
+    );
+
+    client.reexecuteOperation(
+      makeOperation(queryOperation.kind, queryOperation, {
+        ...queryOperation.context,
+        requestPolicy: 'network-only',
+      })
+    );
+
+    await Promise.resolve();
+
+    expect(output.length).toBe(2);
+    expect(output[1]).toHaveProperty('data', 1);
+    expect(output[1]).toHaveProperty('stale', true);
+    expect(output[1]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[1]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-first'
+    );
+
+    jest.advanceTimersByTime(1);
+
+    expect(output.length).toBe(3);
+    expect(output[2]).toHaveProperty('data', 2);
+    expect(output[2]).toHaveProperty('stale', undefined);
+    expect(output[2]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[2]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'network-only'
+    );
+
+    unsubscribe();
+  });
+
+  it('does not reemit previous results as stale if it was marked as stale already', async () => {
+    const output: OperationResult[] = [];
+    const exchange: Exchange = () => ops$ => {
+      return pipe(
+        ops$,
+        filter(op => op.kind !== 'teardown'),
+        map(op => ({
+          data: 1,
+          operation: op,
+          stale: true,
+        })),
+        delay(1)
+      );
+    };
+
+    const client = createClient({
+      url: 'test',
+      exchanges: [exchange],
+    });
+
+    const { unsubscribe } = pipe(
+      client.executeRequestOperation(queryOperation),
+      subscribe(result => {
+        output.push(result);
+      })
+    );
+
+    jest.advanceTimersByTime(1);
+
+    expect(output.length).toBe(1);
+    expect(output[0]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[0]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-first'
+    );
+
+    client.reexecuteOperation(
+      makeOperation(queryOperation.kind, queryOperation, {
+        ...queryOperation.context,
+        requestPolicy: 'network-only',
+      })
+    );
+
+    await Promise.resolve();
+    jest.advanceTimersByTime(1);
+
+    expect(output.length).toBe(2);
+    expect(output[1]).toHaveProperty('stale', true);
+    expect(output[1]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[1]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'network-only'
+    );
+
+    unsubscribe();
+  });
+
+  it.skip('does not reemit previous results as stale if cache emits first', async () => {
+    const output: OperationResult[] = [];
+
+    const exchange: Exchange = () => ops$ => {
+      return pipe(
+        ops$,
+        filter(op => op.kind !== 'teardown'),
+        map(op => ({
+          data: 1,
+          operation: op,
+          stale: true,
+        }))
+      );
+    };
+
+    const client = createClient({
+      url: 'test',
+      exchanges: [exchange],
+    });
+
+    const { unsubscribe } = pipe(
+      client.executeRequestOperation(queryOperation),
+      subscribe(result => {
+        output.push(result);
+      })
+    );
+
+    expect(output.length).toBe(1);
+    expect(output[0]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[0]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-first'
+    );
+
+    client.reexecuteOperation(
+      makeOperation(queryOperation.kind, queryOperation, {
+        ...queryOperation.context,
+        requestPolicy: 'cache-and-network',
+      })
+    );
+
+    await Promise.resolve();
+
+    expect(output.length).toBe(2);
+    expect(output[1]).toHaveProperty('stale', false);
+    expect(output[1]).toHaveProperty('operation.key', queryOperation.key);
+    expect(output[1]).toHaveProperty(
+      'operation.context.requestPolicy',
+      'cache-and-network'
+    );
+
+    unsubscribe();
+  });
 });
diff --git a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap
index f0691c4e..6d1d342c 100644
--- a/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap
+++ b/packages/core/src/exchanges/__snapshots__/fetch.test.ts.snap
@@ -15,7 +15,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -185,7 +184,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -357,7 +355,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
diff --git a/packages/core/src/exchanges/__snapshots__/subscription.test.ts.snap b/packages/core/src/exchanges/__snapshots__/subscription.test.ts.snap
index 53c422fa..864ba6fc 100644
--- a/packages/core/src/exchanges/__snapshots__/subscription.test.ts.snap
+++ b/packages/core/src/exchanges/__snapshots__/subscription.test.ts.snap
@@ -15,7 +15,6 @@ Object {
     },
     "key": 4,
     "kind": "subscription",
-    "operationName": "subscription",
     "query": Object {
       "__key": 2616620656,
       "definitions": Array [
diff --git a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap
index 994af45b..2dbf72ae 100644
--- a/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap
+++ b/packages/core/src/internal/__snapshots__/fetchSource.test.ts.snap
@@ -15,7 +15,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -175,7 +174,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -335,7 +333,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -499,7 +496,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
@@ -679,7 +675,6 @@ Object {
     },
     "key": 2,
     "kind": "query",
-    "operationName": "query",
     "query": Object {
       "__key": 3044551916,
       "definitions": Array [
diff --git a/packages/core/src/utils/deprecation.test.ts b/packages/core/src/utils/deprecation.test.ts
deleted file mode 100644
index 89c0da4e..00000000
--- a/packages/core/src/utils/deprecation.test.ts
+++ /dev/null
@@ -1,29 +0,0 @@
-jest.unmock('./deprecation.ts');
-
-import { _clearWarnings, deprecationWarning } from './deprecation';
-
-describe('deprecationWarning()', () => {
-  let warn: jest.SpyInstance;
-
-  const key = 'deprecation.test';
-  const message = 'Test deprecation message.';
-
-  beforeAll(() => {
-    warn = jest.spyOn(console, 'warn');
-  });
-
-  afterEach(_clearWarnings);
-
-  afterAll(() => {
-    warn.mockRestore();
-  });
-
-  it('only calls console.warn once per key', () => {
-    deprecationWarning({ key, message });
-    deprecationWarning({ key, message });
-    deprecationWarning({ key, message });
-
-    expect(warn).toBeCalledTimes(1);
-    expect(warn).toBeCalledWith(`[WARNING: Deprecated] ${message}`);
-  });
-});