(core) - Fix formatDocument mutating original document

.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.

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

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,16 +20,16 @@ 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));
     query = docs[key] !== undefined ? docs[key] : q;
   }
 
   docs[key] = query;
-  (query as any)[keyProp] = key;
+  (query as any).__key = key;
 
   return {
     key: vars ? phash(key, stringifyVariables(vars)) >>> 0 : key,

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', () => {

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