Repository: FormidableLabs/urql (ts)

Issue:
Best practice to update cache on mutation insert/delete for large applications?
This is more of a question related to best practice for create/delete mutations. I noticed that the urql graph cache does not update automatically when things are added / removed via a mutation. After exploring the documentation and examples, I was able to figure out a solution that works, but I'm not sure if it's the best way. So before doing this for all of the mutations across our application, I figured I'd check in here :).

Some background - we're using https://hasura.io to drive much of our graphql API, and their insert/delete mutations work with multiple objects (similar to batching). Here are some example mutations / queries (note they're all returning `__typename: star`).

```graphql
mutation StarNode($projectId: Int!, $nodeId: Int!) {
  createStars(objects: [{ projectId: $projectId, nodeId: $nodeId }]) {
    returning {
      id
      __typename # of type star
    }
  }
}

mutation UnstarNode($projectId: Int!, $nodeId: Int!) {
  deleteStars(where: { nodeId: { _eq: $nodeId }, projectId: { _eq: $projectId } }) {
    returning {
      id
      __typename # of type star
    }
  }
}

query GetNodeStar($projectId: Int!, $nodeId: Int!) {
  stars(where: { nodeId: { _eq: $nodeId }, projectId: { _eq: $projectId } }) {
    id
    __typename # of type star
  }
}
```

After some fiddling, this is what I came up with. Is this recommended way to make sure the UI / cache updates in response to insert/delete mutations? It works, but it feels like it won't scale well in a large application that has lots of different queries, with different variables, etc - particularly because we have to make sure to pass the originally used `variables`. The creation of this cache is quite far away from the queries happening in our application, so it feels like it will be easy to make a mistake and not cover all the variable combinations, etc.

```ts
import { cacheExchange } from '@urql/exchange-graphcache';

// @ts-ignore
clientCache = cacheExchange({
  updates: {
    Mutation: {
      createStars: (result: StarNodeMutation, args, cache, info) => {
        cache.updateQuery(
          { query: GetNodeStarDocument, variables: info.variables },
          // @ts-ignore
          (data: GetNodeStarQuery | null) => {
            if (data) data.stars.push(...(result.createStars?.returning || []));

            return data;
          },
        );
      },
      deleteStars: (result: UnstarNodeMutation, args, cache, info) => {
        cache.updateQuery(
          { query: GetNodeStarDocument, variables: info.variables },
          // @ts-ignore
          (data: GetNodeStarQuery | null) => {
            const deletedIds = (result.deleteStars?.returning || []).map(s => s.id);
            if (data) {
              data.stars = data.stars.filter(s => !deletedIds.includes(s.id));
            }

            return data;
          },
        );
      },
    },
  },
});
```

### Random Idea?

I'm left wondering if URQL could remove much of this complexity with a hint from the developer? Something like this (the relevant lines are those with comments):

```tsx
const NodeStarButton = ({ node, projectId, isStarred }: { node: INodeInfo; projectId: number; isStarred: boolean }) => {
  // these are using URQL useMutation hook under the hood, with the same response signature
  const [, starNodeMutation] = useStarNodeMutation();
  const [, unstarNodeMutation] = useUnstarNodeMutation();

  return (
    <Button
      text="Star"
      icon={isStarred ? 'star' : 'star-empty'}
      intent="primary"
      onClick={async () => {
        if (isStarred) {
          unstarNodeMutation(
            { projectId: projectId, nodeId: node.id },
            {
              // any nodes in the response with __typename: star should be removed from the cache
              deleting: ['star'],
            },
          );
        } else {
          starNodeMutation(
            { projectId: projectId, nodeId: node.id },
            {
              // any nodes in the response with __typename: star should be added to the cache
              creating: ['star'],
            },
          );
        }
      }}
    />
  );
};
```

Basically allowing the user to hint that any nodes with typename `[type1, type2, ...]` in the response should automatically be added or removed from the cache. This does not cover more complex cases of course, but it seems like it could cover a pretty common use case (I want something generally removed from the cache, across all queries, when deleted, etc). End user still has the `updates` feature in graph cache for more complex cases.

This is a random idea, I admittedly do not quite understand all the URQL internals yet, so this might not make much sense.

### Typings

A note on typings - we generate typings from our GraphQL API and client queries in order to guarantee that nothing every gets out of sync. The way that the `cacheExchange` types are put together, we can't use our types in these update functions. The @ts-ignore statements you see in the code snippet above are to ignore the typings problem. 

If this seems like a legitimate thing to fix LMK and I can create a separate issue for typings improvements.

Here's what vscode reports without the ignores, for the two different classes of ignore in code above:

<img width="984" alt="Screen Shot 2020-02-28 at 7 39 15 PM" src="https://user-images.githubusercontent.com/847542/75614474-b4384100-5afe-11ea-89d8-946251c2f93c.png">

<img width="1121" alt="Screen Shot 2020-02-28 at 7 39 39 PM" src="https://user-images.githubusercontent.com/847542/75614476-b5696e00-5afe-11ea-8200-0f9736420e55.png">

Pull request description:
(graphcache) - Fix variables being filtered in operations
Resolve #559 
Resolve https://github.com/FormidableLabs/urql-exchange-graphcache/issues/146

## Summary

This allows untyped variables to be used inside operations and therefore inside updaters, which may be necessary to pass additional data to some updaters.

The filtering is preserved, it's moved to before the forwarding in `cacheExchange` however.

## Set of changes

- Refactor `cacheExchange` logic
- Add `filterVariables` and fix `normalizeVariables`
- Use `formatDocument` and `filterVariables` only when operation is forwarded


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/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)
     );


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);