Repository: FormidableLabs/urql (ts)

Issue:
SSR pages re-executing queries
Hi :wave:

I've been debugging an issue where SSR pages are re-executing queries on the client (some of the time).

I believe this is caused by `cacheExchange` mutating `operation.query` when it [calls `formatDocument(operation.query)`](https://github.com/FormidableLabs/urql/blob/e6e2fa9/packages/core/src/exchanges/cache.ts#L28). 

There is a [comment](https://github.com/FormidableLabs/urql/blob/ec8c318/packages/core/src/utils/typenames.ts#L43) in `formatDocument()` which explains that mutating the `node` is ok because `visit()` clones the AST upwards. Although `visit()` does clone the AST upwards, it looks like mutating the `node` still mutates the original `operation.query` which is used when computing the query `__key`.

This issue only appears intermittently and likely with a specific version of node. I've managed to reproduce this issue locally using node `v10.20.1` and a production webpack build of our app. I have confirmed we see different `__key`'s for the same query on the client and server due to additional `__typename` entries on the server side only. Deep copying `operation.query` before passing it to `formatDocument()` resolves the issue for us, which suggests mutating the node is the cause.

Looking at other active issues, https://github.com/FormidableLabs/urql/issues/874 may be related.

urql version & exchanges: `1.9.5` `cacheExchange`

Thanks :)


Pull request description:
(core) - Fix formatDocument mutating original document
Resolve #879 

- Fix the formatDocument mutating parts of the original document
- Fix a formatted document from creating a different hash in createRequest

The latter works because `formatDocument` can be assumed to be called
by caches, like the cacheExchange, so they shouldn't ever alter the
resulting hash.

This does affect the minzipped size: `3.44kB => 3.46kB`

Patch:
diff --git a/.changeset/six-hornets-smile.md b/.changeset/six-hornets-smile.md
new file mode 100644
index 00000000..02665ca5
--- /dev/null
+++ b/.changeset/six-hornets-smile.md
@@ -0,0 +1,5 @@
+---
+'@urql/core': patch
+---
+
+Fix `formatDocument` mutating parts of the `DocumentNode` which may be shared by other documents and queries. Also ensure that a formatted document will always generate the same key in `createRequest` as the original document.
diff --git a/packages/core/src/utils/request.ts b/packages/core/src/utils/request.ts
index 5d33de19..1eaa1ded 100644
--- a/packages/core/src/utils/request.ts
+++ b/packages/core/src/utils/request.ts
@@ -10,7 +10,6 @@ interface Documents {
 const hashQuery = (q: string): number => hash(q.replace(/[\s,]+/g, ' ').trim());
 
 const docs: Documents = Object.create(null);
-const keyProp = '__key';
 
 export const createRequest = (
   q: string | DocumentNode,
@@ -21,8 +20,8 @@ export const createRequest = (
   if (typeof q === 'string') {
     key = hashQuery(q);
     query = docs[key] !== undefined ? docs[key] : parse(q);
-  } else if ((q as any)[keyProp] !== undefined) {
-    key = (q as any)[keyProp];
+  } else if ((q as any).__key !== undefined) {
+    key = (q as any).__key;
     query = q;
   } else {
     key = hashQuery(print(q));
@@ -30,7 +29,7 @@ export const createRequest = (
   }
 
   docs[key] = query;
-  (query as any)[keyProp] = key;
+  (query as any).__key = key;
 
   return {
     key: vars ? phash(key, stringifyVariables(vars)) >>> 0 : key,
diff --git a/packages/core/src/utils/typenames.ts b/packages/core/src/utils/typenames.ts
index 7a7cd434..166be910 100755
--- a/packages/core/src/utils/typenames.ts
+++ b/packages/core/src/utils/typenames.ts
@@ -40,23 +40,32 @@ const formatNode = (node: FieldNode | InlineFragmentNode) => {
       node => node.kind === Kind.FIELD && node.name.value === '__typename'
     )
   ) {
-    // NOTE: It's fine to mutate here as long as we return the node,
-    // which will instruct visit() to clone the AST upwards
-    (node.selectionSet.selections as SelectionNode[]).push({
-      kind: Kind.FIELD,
-      name: {
-        kind: Kind.NAME,
-        value: '__typename',
+    return {
+      ...node,
+      selectionSet: {
+        ...node.selectionSet,
+        selections: [
+          ...(node.selectionSet.selections as SelectionNode[]),
+          {
+            kind: Kind.FIELD,
+            name: {
+              kind: Kind.NAME,
+              value: '__typename',
+            },
+          },
+        ],
       },
-    });
-
-    return node;
+    };
   }
 };
 
-export const formatDocument = (node: DocumentNode) => {
-  return visit(node, {
+export const formatDocument = (node: DocumentNode): DocumentNode => {
+  const result = visit(node, {
     Field: formatNode,
     InlineFragment: formatNode,
   });
+
+  // Ensure that the hash of the resulting document won't suddenly change
+  result.__key = (node as any).__key;
+  return result;
 };


Test patch:
diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts
index 8e05934b..f7bcd490 100644
--- a/exchanges/graphcache/src/offlineExchange.test.ts
+++ b/exchanges/graphcache/src/offlineExchange.test.ts
@@ -1,10 +1,12 @@
-import gql from 'graphql-tag';
 import {
   createClient,
   ExchangeIO,
   Operation,
   OperationResult,
+  formatDocument,
 } from '@urql/core';
+
+import gql from 'graphql-tag';
 import { pipe, map, makeSubject, tap, publish } from 'wonka';
 import { offlineExchange } from './offlineExchange';
 
@@ -263,7 +265,7 @@ describe('offline', () => {
     expect(dispatchOperationSpy).toHaveBeenCalledTimes(1);
     expect((dispatchOperationSpy.mock.calls[0][0] as any).key).toEqual(1);
     expect((dispatchOperationSpy.mock.calls[0][0] as any).query).toEqual(
-      mutationOp.query
+      formatDocument(mutationOp.query)
     );
   });
 });
diff --git a/packages/core/src/utils/typenames.test.ts b/packages/core/src/utils/typenames.test.ts
index f48b7c78..39833b0e 100755
--- a/packages/core/src/utils/typenames.test.ts
+++ b/packages/core/src/utils/typenames.test.ts
@@ -1,5 +1,6 @@
 import { parse, print } from 'graphql';
 import { collectTypesFromResponse, formatDocument } from './typenames';
+import { createRequest } from './request';
 
 const formatTypeNames = (query: string) => {
   const typedNode = formatDocument(parse(query));
@@ -8,7 +9,7 @@ const formatTypeNames = (query: string) => {
 
 describe('formatTypeNames', () => {
   it('creates a new instance when adding typenames', () => {
-    const doc = parse(`{ todos { id } }`) as any;
+    const doc = parse(`{ id todos { id } }`) as any;
     const newDoc = formatDocument(doc) as any;
     expect(doc).not.toBe(newDoc);
     expect(doc.definitions).not.toBe(newDoc.definitions);
@@ -23,6 +24,29 @@ describe('formatTypeNames', () => {
     expect(doc.definitions[0].selectionSet.selections[0]).toBe(
       newDoc.definitions[0].selectionSet.selections[0]
     );
+    // Not equal again:
+    expect(doc.definitions[0].selectionSet.selections[1]).not.toBe(
+      newDoc.definitions[0].selectionSet.selections[1]
+    );
+    expect(doc.definitions[0].selectionSet.selections[1].selectionSet).not.toBe(
+      newDoc.definitions[0].selectionSet.selections[1].selectionSet
+    );
+    // Equal again:
+    expect(
+      doc.definitions[0].selectionSet.selections[1].selectionSet.selections[0]
+    ).toBe(
+      newDoc.definitions[0].selectionSet.selections[1].selectionSet
+        .selections[0]
+    );
+  });
+
+  it('preserves the hashed key of the resulting query', () => {
+    const doc = parse(`{ id todos { id } }`) as any;
+    const expectedKey = createRequest(doc).key;
+    const formattedDoc = formatDocument(doc);
+    expect(formattedDoc).not.toBe(doc);
+    const actualKey = createRequest(formattedDoc).key;
+    expect(expectedKey).toBe(actualKey);
   });
 
   it('adds typenames to a query string', () => {