Repository: FormidableLabs/urql (ts)

Issue:
React 16.13.0 'Warning: Cannot update a component from inside the function body of a different component.'
After upgrading to React 16.3, I notice a significant performance issue on pages where I have multiple components all querying the same cache value. This appears to be due to an issue with the `useQuery` hook and/or the `cacheExchange`. 

**Expected behaviour**
A single network requests is made to populate the cache. Components render with cached value once available.

**Actual behaviour**
Performance absolutely tanks and the following message appears: `Warning: Cannot update a component from inside the function body of a different component.`

<img width="1098" alt="Screenshot 2020-03-10 at 19 12 41" src="https://user-images.githubusercontent.com/1704089/76350987-98a21700-6304-11ea-8d95-9d230defdfdd.png">


---
This doesn't seem to be unique to `urql` — [Apollo has a similar issue](https://github.com/apollographql/react-apollo/issues/3863).  I've [forked and tweaked the Apollo example repo](https://github.com/hugo/usequery) to use `urql` instead of `apollo`. The same notes about when the issue manifests seem to apply. 

Pull request description:
(react-urql) - Fix React warning on cross-component updates
Resolve #600 

## Summary

This prevents us from triggering updates across components in render by preventing `getCurrentValue` from being able to trigger updates in other subscriptions with a simple global flag.

## Set of changes

- Add global flag to `useSource`'s `getCurrentValue`
- Replace `use-subscription` with custom code

Patch:
diff --git a/.changeset/lucky-hotels-scream.md b/.changeset/lucky-hotels-scream.md
new file mode 100644
index 00000000..b1671ad5
--- /dev/null
+++ b/.changeset/lucky-hotels-scream.md
@@ -0,0 +1,5 @@
+---
+'@urql/exchange-graphcache': patch
+---
+
+Prevent variables from being filtered and queries from being altered before they're forwarded, which prevented additional untyped variables from being used inside updater functions.
diff --git a/.changeset/rude-toes-chew.md b/.changeset/rude-toes-chew.md
new file mode 100644
index 00000000..50913116
--- /dev/null
+++ b/.changeset/rude-toes-chew.md
@@ -0,0 +1,5 @@
+---
+'urql': patch
+---
+
+Avoid React v16.13.0's "Warning: Cannot update a component" by preventing cross-hook updates during render or initial mount.
diff --git a/exchanges/graphcache/src/ast/index.ts b/exchanges/graphcache/src/ast/index.ts
index ad9f4243..2bef59f5 100644
--- a/exchanges/graphcache/src/ast/index.ts
+++ b/exchanges/graphcache/src/ast/index.ts
@@ -1,4 +1,4 @@
-export { getFieldArguments, normalizeVariables } from './variables';
+export * from './variables';
 export * from './traversal';
 export * from './schemaPredicates';
 export * from './node';
diff --git a/exchanges/graphcache/src/ast/variables.ts b/exchanges/graphcache/src/ast/variables.ts
index 2831d0ce..ce41e2a0 100644
--- a/exchanges/graphcache/src/ast/variables.ts
+++ b/exchanges/graphcache/src/ast/variables.ts
@@ -30,25 +30,46 @@ export const getFieldArguments = (
   return argsSize > 0 ? args : null;
 };
 
+/** Returns a filtered form of variables with values missing that the query doesn't require */
+export const filterVariables = (
+  node: OperationDefinitionNode,
+  input: void | object
+) => {
+  if (!input || !node.variableDefinitions) {
+    return undefined;
+  }
+
+  const vars = makeDict();
+  for (let i = 0, l = node.variableDefinitions.length; i < l; i++) {
+    const name = getName(node.variableDefinitions[i].variable);
+    vars[name] = input[name];
+  }
+
+  return vars;
+};
+
 /** Returns a normalized form of variables with defaulted values */
 export const normalizeVariables = (
   node: OperationDefinitionNode,
   input: void | object
 ): Variables => {
-  const args: Variables = (input as Variables) || {};
   const vars = makeDict();
+  if (!input) return vars;
+
   if (node.variableDefinitions) {
     for (let i = 0, l = node.variableDefinitions.length; i < l; i++) {
       const def = node.variableDefinitions[i];
       const name = getName(def.variable);
-      let value = args[name];
-      if (value === undefined && def.defaultValue) {
-        value = valueFromASTUntyped(def.defaultValue, args);
-      }
-
-      vars[name] = value;
+      vars[name] =
+        input[name] === undefined && def.defaultValue
+          ? valueFromASTUntyped(def.defaultValue, input)
+          : input[name];
     }
   }
 
+  for (const key in input) {
+    if (!(key in vars)) vars[key] = input[key];
+  }
+
   return vars;
 };
diff --git a/exchanges/graphcache/src/cacheExchange.ts b/exchanges/graphcache/src/cacheExchange.ts
index 50c9b21b..ece4e253 100644
--- a/exchanges/graphcache/src/cacheExchange.ts
+++ b/exchanges/graphcache/src/cacheExchange.ts
@@ -29,6 +29,7 @@ import {
 import { query, write, writeOptimistic } from './operations';
 import { hydrateData } from './store/data';
 import { makeDict } from './helpers/dict';
+import { filterVariables, getMainOperation } from './ast';
 import { Store, noopDataState, reserveLayer } from './store';
 
 import {
@@ -134,12 +135,23 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
   };
 
   // This registers queries with the data layer to ensure commutativity
-  const prepareCacheForResult = (operation: Operation) => {
+  const prepareForwardedOperation = (operation: Operation) => {
     if (operation.operationName === 'query') {
       reserveLayer(store.data, operation.key);
     } else if (operation.operationName === 'teardown') {
       noopDataState(store.data, operation.key);
     }
+
+    return {
+      ...operation,
+      variables: operation.variables
+        ? filterVariables(
+            getMainOperation(operation.query),
+            operation.variables
+          )
+        : operation.variables,
+      query: formatDocument(operation.query),
+    };
   };
 
   // This executes an optimistic update for mutations and registers it if necessary
@@ -184,7 +196,7 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
   ): OperationResultWithMeta => {
     const res = query(store, operation);
     const cacheOutcome: CacheOutcome = res.data
-      ? !res.partial || operation.context.requestPolicy === 'cache-only'
+      ? !res.partial
         ? 'hit'
         : 'partial'
       : 'miss';
@@ -275,29 +287,49 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
     );
 
     // Filter by operations that are cacheable and attempt to query them from the cache
-    const cache$ = pipe(
+    const cacheOps$ = pipe(
       inputOps$,
-      filter(
-        op =>
+      filter(op => {
+        return (
           op.operationName === 'query' &&
           op.context.requestPolicy !== 'network-only'
-      ),
+        );
+      }),
       map(operationResultFromCache),
       share
     );
 
+    const nonCacheOps$ = pipe(
+      inputOps$,
+      filter(op => {
+        return (
+          op.operationName !== 'query' ||
+          op.context.requestPolicy === 'network-only'
+        );
+      })
+    );
+
     // Rebound operations that are incomplete, i.e. couldn't be queried just from the cache
-    const cacheOps$ = pipe(
-      cache$,
-      filter(res => res.outcome === 'miss'),
+    const cacheMissOps$ = pipe(
+      cacheOps$,
+      filter(res => {
+        return (
+          res.outcome === 'miss' &&
+          res.operation.context.requestPolicy !== 'cache-only'
+        );
+      }),
       map(res => addCacheOutcome(res.operation, 'miss'))
     );
 
     // Resolve OperationResults that the cache was able to assemble completely and trigger
     // a network request if the current operation's policy is cache-and-network
     const cacheResult$ = pipe(
-      cache$,
-      filter(res => res.outcome !== 'miss'),
+      cacheOps$,
+      filter(
+        res =>
+          res.outcome !== 'miss' ||
+          res.operation.context.requestPolicy === 'cache-only'
+      ),
       map(
         (res: OperationResultWithMeta): OperationResult => {
           const { operation, outcome } = res;
@@ -327,21 +359,8 @@ export const cacheExchange = (opts?: CacheExchangeOpts): Exchange => ({
     // Forward operations that aren't cacheable and rebound operations
     // Also update the cache with any network results
     const result$ = pipe(
-      merge([
-        pipe(
-          inputOps$,
-          filter(
-            op =>
-              !(
-                op.operationName === 'query' &&
-                op.context.requestPolicy !== 'network-only'
-              )
-          )
-        ),
-        cacheOps$,
-      ]),
-      filter(op => op.context.requestPolicy !== 'cache-only'),
-      tap(prepareCacheForResult),
+      merge([nonCacheOps$, cacheMissOps$]),
+      map(prepareForwardedOperation),
       forward,
       map(updateCacheWithResult)
     );
diff --git a/packages/react-urql/package.json b/packages/react-urql/package.json
index f50b1b1a..b322cea0 100644
--- a/packages/react-urql/package.json
+++ b/packages/react-urql/package.json
@@ -47,7 +47,6 @@
     "@testing-library/react-hooks": "^3.2.1",
     "@types/react": "^16.9.19",
     "@types/react-test-renderer": "^16.9.2",
-    "@types/use-subscription": "^1.0.0",
     "graphql": "^14.6.0",
     "graphql-tag": "^2.10.1",
     "react": "^16.13.0",
@@ -62,7 +61,6 @@
   },
   "dependencies": {
     "@urql/core": "^1.10.1",
-    "use-subscription": "^1.4.0",
     "wonka": "^4.0.7"
   }
 }
diff --git a/packages/react-urql/src/hooks/useSource.ts b/packages/react-urql/src/hooks/useSource.ts
index a399fcec..e9bc42b3 100644
--- a/packages/react-urql/src/hooks/useSource.ts
+++ b/packages/react-urql/src/hooks/useSource.ts
@@ -1,8 +1,6 @@
-/* eslint-disable @typescript-eslint/no-use-before-define */
 /* eslint-disable react-hooks/exhaustive-deps */
 
-import { useMemo, useEffect } from 'react';
-import { Subscription, Unsubscribe, useSubscription } from 'use-subscription';
+import { useMemo, useEffect, useState } from 'react';
 
 import {
   Source,
@@ -18,43 +16,38 @@ import {
 
 import { useClient } from '../context';
 
-export const useSource = <T>(source: Source<T>, init: T): T =>
-  useSubscription(
-    useMemo((): Subscription<T> => {
-      let hasUpdate = false;
-      let currentValue: T = init;
-
-      // Wrap the source and track its `currentValue`
-      const updateValue = pipe(
-        source,
-        onPush(value => {
-          currentValue = value;
-        })
-      );
-
-      return {
-        // getCurrentValue may be implemented by subscribing to the
-        // given source and immediately unsubscribing. Only synchronous
-        // values will therefore reach our `onPush` callback.
-        getCurrentValue(): T {
-          if (!hasUpdate) publish(updateValue).unsubscribe();
-          return currentValue;
-        },
-        // subscribe is just a regular subscription, but it also tracks
-        // `hasUpdate`. If we're subscribed and receive a new value we
-        // set `hasUpdate` to avoid `getCurrentValue` trying to subscribe
-        // again.
-        subscribe(onValue: () => void): Unsubscribe {
-          hasUpdate = true;
-          const { unsubscribe } = pipe(updateValue, subscribe(onValue));
-          return () => {
-            unsubscribe();
-            hasUpdate = false;
-          };
-        },
-      };
-    }, [source])
-  );
+let currentInit = false;
+
+export const useSource = <T>(source: Source<T>, init: T): T => {
+  const [state, setState] = useState(() => {
+    currentInit = true;
+    let initialValue = init;
+
+    pipe(
+      source,
+      onPush(value => {
+        initialValue = value;
+      }),
+      publish
+    ).unsubscribe();
+
+    currentInit = false;
+    return initialValue;
+  });
+
+  useEffect(() => {
+    return pipe(
+      source,
+      subscribe(value => {
+        if (!currentInit) {
+          setState(value);
+        }
+      })
+    ).unsubscribe as () => void;
+  }, [source]);
+
+  return state;
+};
 
 export const useBehaviourSubject = <T>(value: T) => {
   const client = useClient();
diff --git a/yarn.lock b/yarn.lock
index 25ded1f3..2daf5824 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -1883,10 +1883,6 @@
   version "2.0.3"
   resolved "https://registry.yarnpkg.com/@types/unist/-/unist-2.0.3.tgz#9c088679876f374eb5983f150d4787aa6fb32d7e"
 
-"@types/use-subscription@^1.0.0":
-  version "1.0.0"
-  resolved "https://registry.yarnpkg.com/@types/use-subscription/-/use-subscription-1.0.0.tgz#d146f8d834f70f50d48bd8246a481d096f11db19"
-
 "@types/yargs-parser@*":
   version "15.0.0"
   resolved "https://registry.yarnpkg.com/@types/yargs-parser/-/yargs-parser-15.0.0.tgz#cb3f9f741869e20cce330ffbeb9271590483882d"
@@ -13048,13 +13044,6 @@ use-subscription@1.1.1:
   resolved "https://registry.yarnpkg.com/use-subscription/-/use-subscription-1.1.1.tgz#5509363e9bb152c4fb334151d4dceb943beaa7bb"
   integrity sha512-gk4fPTYvNhs6Ia7u8/+K7bM7sZ7O7AMfWtS+zPO8luH+zWuiGgGcrW0hL4MRWZSzXo+4ofNorf87wZwBKz2YdQ==
 
-use-subscription@^1.4.0:
-  version "1.4.0"
-  resolved "https://registry.yarnpkg.com/use-subscription/-/use-subscription-1.4.0.tgz#c4e808cfed6fe6e1ac875df1369c63f3ddae9522"
-  integrity sha512-R7P7JWpeHp+dtEYsgDzIIgOmVqRfJjRjLOO0YIYk6twctUkUYe6Tz0pcabyTDGcMMRt9uMbFMfwBfxKHg9gCSw==
-  dependencies:
-    object-assign "^4.1.1"
-
 use@^3.1.0:
   version "3.1.1"
   resolved "https://registry.yarnpkg.com/use/-/use-3.1.1.tgz#d50c8cac79a19fbc20f2911f56eb973f4e10070f"


Test patch:
diff --git a/exchanges/graphcache/src/ast/variables.test.ts b/exchanges/graphcache/src/ast/variables.test.ts
index 1b012cce..9f06c511 100644
--- a/exchanges/graphcache/src/ast/variables.test.ts
+++ b/exchanges/graphcache/src/ast/variables.test.ts
@@ -1,6 +1,6 @@
 import gql from 'graphql-tag';
 import { getMainOperation } from './traversal';
-import { normalizeVariables } from './variables';
+import { normalizeVariables, filterVariables } from './variables';
 
 describe('normalizeVariables', () => {
   it('normalizes variables', () => {
@@ -54,4 +54,57 @@ describe('normalizeVariables', () => {
     const normalized = normalizeVariables(operation, {});
     expect(normalized).toEqual({});
   });
+
+  it('preserves missing variables', () => {
+    const operation = getMainOperation(
+      gql`
+        query {
+          field
+        }
+      `
+    );
+    (operation as any).variableDefinitions = undefined;
+    const normalized = normalizeVariables(operation, { test: true });
+    expect(normalized).toEqual({ test: true });
+  });
+});
+
+describe('filterVariables', () => {
+  it('returns undefined when no variables are defined', () => {
+    const operation = getMainOperation(
+      gql`
+        query {
+          field
+        }
+      `
+    );
+    const vars = filterVariables(operation, { test: true });
+    expect(vars).toBe(undefined);
+  });
+
+  it('filters out missing vars', () => {
+    const input = { x: true, y: false };
+    const operation = getMainOperation(
+      gql`
+        query($x: Int!) {
+          field
+        }
+      `
+    );
+    const vars = filterVariables(operation, input);
+    expect(vars).toEqual({ x: true });
+  });
+
+  it('ignores defaults', () => {
+    const input = { x: undefined };
+    const operation = getMainOperation(
+      gql`
+        query($x: Int! = 42) {
+          field
+        }
+      `
+    );
+    const vars = filterVariables(operation, input);
+    expect(vars).toEqual({ x: undefined });
+  });
 });
diff --git a/exchanges/graphcache/src/cacheExchange.test.ts b/exchanges/graphcache/src/cacheExchange.test.ts
index b462e771..e1a7ebc0 100644
--- a/exchanges/graphcache/src/cacheExchange.test.ts
+++ b/exchanges/graphcache/src/cacheExchange.test.ts
@@ -74,6 +74,44 @@ describe('data dependencies', () => {
     );
   });
 
+  it('respects cache-only operations', () => {
+    const client = createClient({ url: 'http://0.0.0.0' });
+    const op = client.createRequestOperation(
+      'query',
+      {
+        key: 1,
+        query: queryOne,
+      },
+      {
+        requestPolicy: 'cache-only',
+      }
+    );
+
+    const response = jest.fn(
+      (forwardOp: Operation): OperationResult => {
+        expect(forwardOp.key).toBe(op.key);
+        return { operation: forwardOp, data: queryOneData };
+      }
+    );
+
+    const { source: ops$, next } = makeSubject<Operation>();
+    const result = jest.fn();
+    const forward: ExchangeIO = ops$ => pipe(ops$, map(response));
+
+    pipe(cacheExchange({})({ forward, client })(ops$), tap(result), publish);
+
+    next(op);
+    expect(response).toHaveBeenCalledTimes(0);
+    expect(result).toHaveBeenCalledTimes(1);
+
+    expect(result.mock.calls[0][0]).toHaveProperty(
+      'operation.context.meta.cacheOutcome',
+      'miss'
+    );
+
+    expect(result.mock.calls[0][0].data).toBe(null);
+  });
+
   it('updates related queries when their data changes', () => {
     const queryMultiple = gql`
       {
diff --git a/exchanges/graphcache/src/store/store.test.ts b/exchanges/graphcache/src/store/store.test.ts
index 7fee5d2a..912c80be 100644
--- a/exchanges/graphcache/src/store/store.test.ts
+++ b/exchanges/graphcache/src/store/store.test.ts
@@ -25,6 +25,7 @@ const Todos = gql`
       text
       complete
       author {
+        __typename
         id
         name
       }
@@ -32,6 +33,64 @@ const Todos = gql`
   }
 `;
 
+const TodosWithoutTypename = gql`
+  query {
+    todos {
+      id
+      text
+      complete
+      author {
+        id
+        name
+      }
+    }
+  }
+`;
+
+const todosData = {
+  __typename: 'Query',
+  todos: [
+    {
+      id: '0',
+      text: 'Go to the shops',
+      complete: false,
+      __typename: 'Todo',
+      author: { id: '0', name: 'Jovi', __typename: 'Author' },
+    },
+    {
+      id: '1',
+      text: 'Pick up the kids',
+      complete: true,
+      __typename: 'Todo',
+      author: { id: '1', name: 'Phil', __typename: 'Author' },
+    },
+    {
+      id: '2',
+      text: 'Install urql',
+      complete: false,
+      __typename: 'Todo',
+      author: { id: '0', name: 'Jovi', __typename: 'Author' },
+    },
+  ],
+} as any;
+
+describe('Store', () => {
+  it('supports unformatted query documents', () => {
+    const store = new Store();
+
+    InMemoryData.initDataState(store.data, null);
+    // NOTE: This is the query without __typename annotations
+    write(store, { query: TodosWithoutTypename }, todosData);
+    InMemoryData.initDataState(store.data, null);
+
+    InMemoryData.initDataState(store.data, null);
+    const result = query(store, { query: TodosWithoutTypename });
+    InMemoryData.initDataState(store.data, null);
+
+    expect(result.data).toEqual(todosData);
+  });
+});
+
 describe('Store with KeyingConfig', () => {
   it('generates keys from custom keying function', () => {
     const store = new Store({
@@ -52,7 +111,7 @@ describe('Store with KeyingConfig', () => {
 });
 
 describe('Store with OptimisticMutationConfig', () => {
-  let store, todosData;
+  let store;
 
   beforeEach(() => {
     store = new Store({
@@ -64,32 +123,6 @@ describe('Store with OptimisticMutationConfig', () => {
         },
       },
     });
-    todosData = {
-      __typename: 'Query',
-      todos: [
-        {
-          id: '0',
-          text: 'Go to the shops',
-          complete: false,
-          __typename: 'Todo',
-          author: { id: '0', name: 'Jovi', __typename: 'Author' },
-        },
-        {
-          id: '1',
-          text: 'Pick up the kids',
-          complete: true,
-          __typename: 'Todo',
-          author: { id: '1', name: 'Phil', __typename: 'Author' },
-        },
-        {
-          id: '2',
-          text: 'Install urql',
-          complete: false,
-          __typename: 'Todo',
-          author: { id: '0', name: 'Jovi', __typename: 'Author' },
-        },
-      ],
-    };
     InMemoryData.initDataState(store.data, null);
     write(store, { query: Todos }, todosData);
     InMemoryData.initDataState(store.data, null);