Repository: FormidableLabs/urql (ts)

Issue:
Remove dependency on graphql-tag
There are several improvements we can make to ensure that we don't need to depend on graphql-tag.

Previously though we ran into issues where using graphql-tag and urql in one project led to import issues with the graphql package itself for some reason, which is why we just used its utility for parsing GraphQL DSL queries.

If we switch back to parsing ourselves we have to probably make some improvements to the hashing. Specifically graphql-tag guarantees that the same two queries always yield the same instance of a DocumentNode on which we can store a hash.

Pull request description:
Refactor createRequest parsing and hashing
Fix #377
Supersedes #380 by also refactoring hashing

Patch:
diff --git a/package.json b/package.json
index b7a7cb01..2439d055 100644
--- a/package.json
+++ b/package.json
@@ -135,8 +135,6 @@
   },
   "dependencies": {
     "fast-json-stable-stringify": "^2.0.0",
-    "graphql-tag": "^2.10.1",
-    "prop-types": "^15.6.0",
     "wonka": "^3.2.0"
   }
 }
diff --git a/src/utils/hash.ts b/src/utils/hash.ts
new file mode 100644
index 00000000..58ab442c
--- /dev/null
+++ b/src/utils/hash.ts
@@ -0,0 +1,14 @@
+// When we have separate strings it's useful to run a progressive
+// version of djb2 where we pretend that we're still looping over
+// the same string
+export const phash = (h: number, x: string): number => {
+  h = h | 0;
+  for (let i = 0, l = x.length | 0; i < l; i++) {
+    h = (h << 5) + h + x.charCodeAt(i);
+  }
+
+  return h;
+};
+
+// This is a djb2 hashing function
+export const hash = (x: string): number => phash(5381 | 0, x) >>> 0;
diff --git a/src/utils/index.ts b/src/utils/index.ts
index 49861b08..0787acf9 100644
--- a/src/utils/index.ts
+++ b/src/utils/index.ts
@@ -1,5 +1,4 @@
 export * from './error';
-export * from './keyForQuery';
 export * from './request';
 export * from './typenames';
 export * from './toSuspenseSource';
diff --git a/src/utils/keyForQuery.ts b/src/utils/keyForQuery.ts
deleted file mode 100644
index 7ff9b5b0..00000000
--- a/src/utils/keyForQuery.ts
+++ /dev/null
@@ -1,71 +0,0 @@
-import stringify from 'fast-json-stable-stringify';
-import { DocumentNode, print } from 'graphql';
-
-interface NameCache {
-  [key: string]: string;
-}
-
-interface WithNameProperty {
-  name?: { value: string };
-  [key: string]: any;
-}
-
-interface WithCacheProperty {
-  __key?: number;
-  [key: string]: any;
-}
-
-const hash = (x: string): number => {
-  /* prettier-ignore */
-  for (var h = 5381 | 0, i = 0, l = x.length | 0; i < l; i++)
-    h = ((h << 5) + h) + x.charCodeAt(i);
-  return h >>> 0;
-};
-
-const docNameCache = Object.create(null) as NameCache;
-
-export const getKeyForQuery = (doc: DocumentNode): number => {
-  if ((doc as WithCacheProperty).__key !== undefined) {
-    return (doc as WithCacheProperty).__key as number;
-  }
-
-  // Using print() can be expensive, so we just check the definition nodes' names
-  let name = doc.definitions.reduce((acc, definition) => {
-    const node = definition as WithNameProperty;
-    return acc + (node.name !== undefined ? node.name.value : '');
-  }, '');
-
-  // This is for inputs that are not using constant references. In such a case
-  // we use the Document names, but we can't use those if they're not unique
-  if (process.env.NODE_ENV !== 'production' && name !== '') {
-    const printed = print(doc);
-    if (!(name in docNameCache)) {
-      docNameCache[name] = printed;
-    } else if (docNameCache[name] !== printed) {
-      console.warn(
-        'Warning: Encountered multiple DocumentNodes with the same name.'
-      );
-    }
-  }
-
-  // If no name is present the stringified document is used
-  if (name === '') {
-    name = print(doc);
-  }
-
-  const key = hash(name);
-  (doc as WithCacheProperty).__key = key;
-  return key;
-};
-
-export const getKeyForRequest = (
-  query: DocumentNode,
-  vars?: object
-): number => {
-  const docKey = getKeyForQuery(query);
-  if (vars === undefined || vars === null) {
-    return docKey;
-  }
-
-  return hash('' + docKey + stringify(vars));
-};
diff --git a/src/utils/request.ts b/src/utils/request.ts
index a81f8763..f5ab63c3 100644
--- a/src/utils/request.ts
+++ b/src/utils/request.ts
@@ -1,16 +1,39 @@
-import { DocumentNode } from 'graphql';
-import gql from 'graphql-tag';
-import { getKeyForRequest } from './keyForQuery';
+import { DocumentNode, parse, print } from 'graphql';
+import { hash, phash } from './hash';
+import stringify from 'fast-json-stable-stringify';
 import { GraphQLRequest, Operation, OperationContext } from '../types';
 
+interface Documents {
+  [key: number]: DocumentNode;
+}
+
+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,
   vars?: object
 ): GraphQLRequest => {
-  const query = typeof q === 'string' ? gql([q]) : q;
+  let key: number;
+  let query: DocumentNode;
+  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];
+    query = q;
+  } else {
+    key = hashQuery(print(q));
+    query = docs[key] !== undefined ? docs[key] : q;
+  }
+
+  docs[key] = query;
+  (query as any)[keyProp] = key;
 
   return {
-    key: getKeyForRequest(query, vars),
+    key: vars ? phash(key, stringify(vars)) >>> 0 : key,
     query,
     variables: vars || {},
   };
diff --git a/yarn.lock b/yarn.lock
index 0308f884..f126d366 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -4816,15 +4816,6 @@ prop-types-exact@^1.2.0:
     object.assign "^4.1.0"
     reflect.ownkeys "^0.2.0"
 
-prop-types@^15.6.0, prop-types@^15.7.2:
-  version "15.7.2"
-  resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.7.2.tgz#52c41e75b8c87e72b9d9360e0206b99dcbffa6c5"
-  integrity sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==
-  dependencies:
-    loose-envify "^1.4.0"
-    object-assign "^4.1.1"
-    react-is "^16.8.1"
-
 prop-types@^15.6.2:
   version "15.6.2"
   resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.6.2.tgz#05d5ca77b4453e985d60fc7ff8c859094a497102"
@@ -4833,6 +4824,15 @@ prop-types@^15.6.2:
     loose-envify "^1.3.1"
     object-assign "^4.1.1"
 
+prop-types@^15.7.2:
+  version "15.7.2"
+  resolved "https://registry.yarnpkg.com/prop-types/-/prop-types-15.7.2.tgz#52c41e75b8c87e72b9d9360e0206b99dcbffa6c5"
+  integrity sha512-8QQikdH7//R2vurIJSutZ1smHYTcLpRWEOlHnzcWHmBYrOGUysKwSsrC89BCiFj3CbrfJ/nXFdJepOVrY1GCHQ==
+  dependencies:
+    loose-envify "^1.4.0"
+    object-assign "^4.1.1"
+    react-is "^16.8.1"
+
 psl@^1.1.24:
   version "1.1.32"
   resolved "https://registry.yarnpkg.com/psl/-/psl-1.1.32.tgz#3f132717cf2f9c169724b2b6caf373cf694198db"


Test patch:
diff --git a/src/client.test.ts b/src/client.test.ts
index 485449be..7552c29f 100755
--- a/src/client.test.ts
+++ b/src/client.test.ts
@@ -3,11 +3,6 @@ import gql from 'graphql-tag';
 
 /** NOTE: Testing in this file is designed to test both the client and it's interaction with default Exchanges */
 
-jest.mock('./utils/keyForQuery', () => ({
-  getKeyForQuery: () => 123,
-  getKeyForRequest: () => 123,
-}));
-
 import { map, pipe, subscribe, tap } from 'wonka';
 import { createClient } from './client';
 
diff --git a/src/utils/keyForQuery.test.ts b/src/utils/keyForQuery.test.ts
deleted file mode 100644
index b94914aa..00000000
--- a/src/utils/keyForQuery.test.ts
+++ /dev/null
@@ -1,103 +0,0 @@
-import gql from 'graphql-tag';
-import { getKeyForQuery, getKeyForRequest } from './keyForQuery';
-
-const consoleWarn = console.warn;
-
-beforeEach(() => {
-  console.warn = jest.fn();
-});
-
-afterAll(() => {
-  console.warn = consoleWarn;
-});
-
-it('returns consistent hashes for DocumentNodes', () => {
-  const query = gql`
-    query Test {
-      test
-    }
-  `;
-
-  const different = gql`
-    query TestB {
-      test2
-    }
-  `;
-
-  expect((query as any).__key).toBe(undefined);
-
-  expect(getKeyForQuery(query)).toBe(getKeyForQuery(query));
-  expect(getKeyForQuery(query)).not.toBe(getKeyForQuery(different));
-
-  expect((query as any).__key).toBe(getKeyForQuery(query));
-});
-
-it('guesses the same hash for similar DocumentNodes', () => {
-  const query = () => gql`
-    query Test {
-      test
-    }
-  `;
-
-  const different = () => gql`
-    query TestB {
-      test2
-    }
-  `;
-
-  expect(getKeyForQuery(query())).toBe(getKeyForQuery(query()));
-  expect(getKeyForQuery(query())).not.toBe(getKeyForQuery(different()));
-});
-
-it('guesses the same hash for unnamed but similar DocumentNodes', () => {
-  const query = () => gql`
-    {
-      test
-    }
-  `;
-
-  const different = () => gql`
-    {
-      test2
-    }
-  `;
-
-  expect(getKeyForQuery(query())).toBe(getKeyForQuery(query()));
-  expect(getKeyForQuery(query())).not.toBe(getKeyForQuery(different()));
-});
-
-it('warns about different queries having identical names', () => {
-  const query = gql`
-    query SameName {
-      test
-    }
-  `;
-
-  const different = gql`
-    query SameName {
-      test2
-    }
-  `;
-
-  // This is expected but obviously problematic
-  expect(getKeyForQuery(query)).toBe(getKeyForQuery(different));
-
-  expect(console.warn).toHaveBeenCalled();
-});
-
-it('can key variables', () => {
-  const query = gql`
-    query SameName {
-      test
-    }
-  `;
-
-  const vars = { test: true };
-
-  expect(getKeyForQuery(query)).toBe(getKeyForRequest(query, undefined));
-  expect(getKeyForRequest(query, vars)).toBe(getKeyForRequest(query, vars));
-  expect(getKeyForRequest(query)).not.toBe(getKeyForRequest(query, vars));
-  expect(getKeyForRequest(query, { test: 2 })).not.toBe(
-    getKeyForRequest(query, vars)
-  );
-});
diff --git a/src/utils/request.test.ts b/src/utils/request.test.ts
index 6e206f13..82e87f74 100644
--- a/src/utils/request.test.ts
+++ b/src/utils/request.test.ts
@@ -1,21 +1,54 @@
-import { print } from 'graphql';
+import { parse, print } from 'graphql';
 import gql from 'graphql-tag';
 import { createRequest } from './request';
 
-const doc = print(
-  gql`
+it('should hash identical queries identically', () => {
+  const reqA = createRequest('{ test }');
+  const reqB = createRequest('{ test }');
+  expect(reqA.key).toBe(reqB.key);
+});
+
+it('should hash identical DocumentNodes identically', () => {
+  const reqA = createRequest(parse('{ testB }'));
+  const reqB = createRequest(parse('{ testB }'));
+  expect(reqA.key).toBe(reqB.key);
+  expect(reqA.query).toBe(reqB.query);
+});
+
+it('should use the hash from a key if available', () => {
+  const doc = parse('{ testC }');
+  (doc as any).__key = 1234;
+  const req = createRequest(doc);
+  expect(req.key).toBe(1234);
+});
+
+it('should hash DocumentNodes and strings identically', () => {
+  const docA = parse('{ field }');
+  const docB = print(docA).replace(/\s/g, ' ');
+  const reqA = createRequest(docA);
+  const reqB = createRequest(docB);
+  expect(reqA.key).toBe(reqB.key);
+  expect(reqA.query).toBe(reqB.query);
+});
+
+it('should hash graphql-tag documents correctly', () => {
+  const doc = gql`
     {
-      todos {
-        id
-      }
+      testD
     }
-  `
-);
+  `;
+  createRequest(doc);
+  expect((doc as any).__key).not.toBe(undefined);
+});
 
 it('should return a valid query object', () => {
+  const doc = gql`
+    {
+      testE
+    }
+  `;
   const val = createRequest(doc);
 
-  expect(print(val.query)).toBe(doc);
   expect(val).toMatchObject({
     key: expect.any(Number),
     query: expect.any(Object),
@@ -24,6 +57,13 @@ it('should return a valid query object', () => {
 });
 
 it('should return a valid query object with variables', () => {
+  const doc = print(
+    gql`
+      {
+        testF
+      }
+    `
+  );
   const val = createRequest(doc, { test: 5 });
 
   expect(print(val.query)).toBe(doc);