From 429d0804f8291f4ea851df7fe9ab1bee841a8107 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 24 Jun 2020 16:53:34 +0100 Subject: [PATCH 1/3] Fix formatDocument mutating original document - 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. --- packages/core/src/utils/request.ts | 7 +++-- packages/core/src/utils/typenames.test.ts | 26 +++++++++++++++++- packages/core/src/utils/typenames.ts | 33 ++++++++++++++--------- 3 files changed, 49 insertions(+), 17 deletions(-) diff --git a/packages/core/src/utils/request.ts b/packages/core/src/utils/request.ts index 5d33de1997..1eaa1ded4b 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.test.ts b/packages/core/src/utils/typenames.test.ts index f48b7c78dd..39833b0e9f 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', () => { diff --git a/packages/core/src/utils/typenames.ts b/packages/core/src/utils/typenames.ts index 7a7cd434fc..166be9102f 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; }; From d465a0cb6878b93272ce7bc23ede7af32420273a Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Wed, 24 Jun 2020 16:55:50 +0100 Subject: [PATCH 2/3] Add changeset --- .changeset/six-hornets-smile.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/six-hornets-smile.md diff --git a/.changeset/six-hornets-smile.md b/.changeset/six-hornets-smile.md new file mode 100644 index 0000000000..02665ca5fa --- /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. From c653842f50ca58b53dbd6dad46ea208327e21ec1 Mon Sep 17 00:00:00 2001 From: Phil Pluckthun Date: Mon, 29 Jun 2020 13:34:57 +0100 Subject: [PATCH 3/3] Update offlineExchange.test.ts --- exchanges/graphcache/src/offlineExchange.test.ts | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/exchanges/graphcache/src/offlineExchange.test.ts b/exchanges/graphcache/src/offlineExchange.test.ts index 8e05934bb3..f7bcd49065 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) ); }); });