Repository: FormidableLabs/urql
Pull request #884 (2020-06-29 11:22:17)

(next-urql) - disable suspense when we aren't binding getInitialProps

Fixes: https://github.com/FormidableLabs/urql/issues/883

[New sandbox](https://codesandbox.io/s/next-urql-pokedex-ko54t)
[Generated bundle](https://pkg.csb.dev/FormidableLabs/urql/commit/52ae6e87/next-urql)

## Summary

disable suspense when we aren't binding getInitialProps, enabling thi…s without getInitialProps means we aren't using prepass which results in react-dom/server throwing an error about suspense not being supported server-side

## Set of changes

- disable suspense when we aren't binding getInitialProps


changeset-bot[bot]: ###  💥  No Changeset

Latest commit: 64cda568350f6a65a57945c81953e9f65165c9e5

Merging this PR will not cause any packages to be released. If these changes should not cause updates to packages in this repo, this is fine 🙂

**If these changes should be published to npm, you need to add a changeset.**

<details><summary>This PR includes no changesets</summary>

  When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

</details>

[Click here to learn what changesets are, and how to add one](https://github.com/changesets/changesets/blob/master/docs/adding-a-changeset.md).

[Click here if you're a maintainer who wants to add a changeset to this PR](https://github.com/FormidableLabs/urql/new/suspense-disabled-without-getInitialProps?filename=.changeset/proud-ligers-act.md&value=---%0A%22next-urql%22%3A%20patch%0A---%0A%0A(next-urql)%20-%20disable%20suspense%20when%20we%20aren't%20binding%20getInitialProps%0A)



Patch:
diff --git a/.changeset/kind-goats-carry.md b/.changeset/kind-goats-carry.md
new file mode 100644
index 00000000..60ef1815
--- /dev/null
+++ b/.changeset/kind-goats-carry.md
@@ -0,0 +1,5 @@
+---
+'next-urql': patch
+---
+
+Disable suspense on the `Client` when we aren't using `react-ssr-prepass`.
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/.changeset/six-pumas-taste.md b/.changeset/six-pumas-taste.md
new file mode 100644
index 00000000..1a683077
--- /dev/null
+++ b/.changeset/six-pumas-taste.md
@@ -0,0 +1,5 @@
+---
+'@urql/core': patch
+---
+
+Fix `ssrExchange` invalidating results on the client-side too eagerly, by delaying invalidation by a tick
diff --git a/.codesandbox/ci.json b/.codesandbox/ci.json
index 27063483..0053fbd7 100644
--- a/.codesandbox/ci.json
+++ b/.codesandbox/ci.json
@@ -1,6 +1,7 @@
 {
   "packages": [
     "packages/core",
+    "packages/next-urql",
     "packages/react-urql",
     "packages/preact-urql",
     "packages/svelte-urql",
diff --git a/packages/core/src/exchanges/ssr.ts b/packages/core/src/exchanges/ssr.ts
index 65e49ea6..53a56025 100644
--- a/packages/core/src/exchanges/ssr.ts
+++ b/packages/core/src/exchanges/ssr.ts
@@ -84,6 +84,19 @@ const deserializeResult = (
 export const ssrExchange = (params?: SSRExchangeParams): SSRExchange => {
   const data: SSRData = {};
 
+  // On the client-side, we delete results from the cache as they're resolved
+  // this is delayed so that concurrent queries don't delete each other's data
+  const invalidateQueue: number[] = [];
+  const invalidate = (result: OperationResult) => {
+    invalidateQueue.push(result.operation.key);
+    if (invalidateQueue.length === 1) {
+      Promise.resolve().then(() => {
+        let key: number | void;
+        while ((key = invalidateQueue.shift())) delete data[key];
+      });
+    }
+  };
+
   const isCached = (operation: Operation) => {
     return !shouldSkip(operation) && data[operation.key] !== undefined;
   };
@@ -131,12 +144,7 @@ export const ssrExchange = (params?: SSRExchangeParams): SSRExchange => {
       );
     } else {
       // On the client we delete results from the cache as they're resolved
-      cachedOps$ = pipe(
-        cachedOps$,
-        tap((result: OperationResult) => {
-          delete data[result.operation.key];
-        })
-      );
+      cachedOps$ = pipe(cachedOps$, tap(invalidate));
     }
 
     return merge([forwardedOps$, cachedOps$]);
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;
 };
diff --git a/packages/next-urql/src/init-urql-client.ts b/packages/next-urql/src/init-urql-client.ts
index 5ef7d5cb..ade570e7 100644
--- a/packages/next-urql/src/init-urql-client.ts
+++ b/packages/next-urql/src/init-urql-client.ts
@@ -3,7 +3,10 @@ import 'isomorphic-unfetch';
 
 let urqlClient: Client | null = null;
 
-export function initUrqlClient(clientOptions: ClientOptions): Client | null {
+export function initUrqlClient(
+  clientOptions: ClientOptions,
+  canEnableSuspense: boolean
+): Client | null {
   // Create a new Client for every server-side rendered request.
   // This ensures we reset the state for each rendered page.
   // If there is an exising client instance on the client-side, use it.
@@ -11,7 +14,7 @@ export function initUrqlClient(clientOptions: ClientOptions): Client | null {
   if (isServer || !urqlClient) {
     urqlClient = createClient({
       ...clientOptions,
-      suspense: isServer || clientOptions.suspense,
+      suspense: canEnableSuspense && (isServer || clientOptions.suspense),
     });
     // Serialize the urqlClient to null on the client-side.
     // This ensures we don't share client and server instances of the urqlClient.
diff --git a/packages/next-urql/src/with-urql-client.tsx b/packages/next-urql/src/with-urql-client.tsx
index e8a7af87..e6feae25 100644
--- a/packages/next-urql/src/with-urql-client.tsx
+++ b/packages/next-urql/src/with-urql-client.tsx
@@ -30,6 +30,10 @@ export function withUrqlClient(
 ) {
   if (!options) options = {};
   return (AppOrPage: NextPage<any> | typeof NextApp) => {
+    const shouldBindGetInitialprops = Boolean(
+      AppOrPage.getInitialProps || options!.ssr
+    );
+
     const withUrql = ({ urqlClient, urqlState, ...rest }: WithUrqlProps) => {
       // eslint-disable-next-line react-hooks/rules-of-hooks
       const client = React.useMemo(() => {
@@ -52,7 +56,7 @@ export function withUrqlClient(
         }
 
         // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-        return initUrqlClient(clientConfig)!;
+        return initUrqlClient(clientConfig, shouldBindGetInitialprops)!;
       }, [urqlClient, urqlState]);
 
       return (
@@ -65,7 +69,7 @@ export function withUrqlClient(
     // Set the displayName to indicate use of withUrqlClient.
     withUrql.displayName = `withUrqlClient(${getDisplayName(AppOrPage)})`;
 
-    if (AppOrPage.getInitialProps || options!.ssr) {
+    if (shouldBindGetInitialprops) {
       withUrql.getInitialProps = async (appOrPageCtx: NextUrqlContext) => {
         const { AppTree } = appOrPageCtx;
 
@@ -86,7 +90,7 @@ export function withUrqlClient(
             fetchExchange,
           ];
         }
-        const urqlClient = initUrqlClient(clientConfig);
+        const urqlClient = initUrqlClient(clientConfig, true);
 
         if (urqlClient) {
           (ctx as NextUrqlContext).urqlClient = urqlClient;


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/exchanges/ssr.test.ts b/packages/core/src/exchanges/ssr.test.ts
index 70a88ee7..f8e14abb 100644
--- a/packages/core/src/exchanges/ssr.test.ts
+++ b/packages/core/src/exchanges/ssr.test.ts
@@ -119,7 +119,7 @@ it('resolves cached query results correctly', () => {
   expect(onPush).toHaveBeenCalledWith(queryResponse);
 });
 
-it('deletes cached results in non-suspense environments', () => {
+it('deletes cached results in non-suspense environments', async () => {
   client.suspense = false;
   const onPush = jest.fn();
   const ssr = ssrExchange();
@@ -133,6 +133,8 @@ it('deletes cached results in non-suspense environments', () => {
   pipe(exchange, forEach(onPush));
   next(queryOperation);
 
+  await Promise.resolve();
+
   expect(Object.keys(ssr.extractData()).length).toBe(0);
   expect(onPush).toHaveBeenCalledWith(queryResponse);
 
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', () => {
diff --git a/packages/next-urql/src/__tests__/init-urql-client.spec.ts b/packages/next-urql/src/__tests__/init-urql-client.spec.ts
index e300dcf0..be08a746 100644
--- a/packages/next-urql/src/__tests__/init-urql-client.spec.ts
+++ b/packages/next-urql/src/__tests__/init-urql-client.spec.ts
@@ -1,12 +1,27 @@
 import { initUrqlClient } from '../init-urql-client';
 
 describe('initUrqlClient', () => {
-  it('should return the urqlClient instance', () => {
-    const urqlClient = initUrqlClient({
-      url: 'http://localhost:3000',
-    });
+  it('should return the urqlClient instance (suspense)', () => {
+    const urqlClient = initUrqlClient(
+      {
+        url: 'http://localhost:3000',
+      },
+      true
+    );
 
     expect(urqlClient).toHaveProperty('url', 'http://localhost:3000');
     expect(urqlClient).toHaveProperty('suspense', true);
   });
+
+  it('should return the urqlClient instance (no-suspense)', () => {
+    const urqlClient = initUrqlClient(
+      {
+        url: 'http://localhost:3000',
+      },
+      false
+    );
+
+    expect(urqlClient).toHaveProperty('url', 'http://localhost:3000');
+    expect(urqlClient).toHaveProperty('suspense', false);
+  });
 });
diff --git a/packages/next-urql/src/__tests__/with-urql-client.spec.tsx b/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
index 525c78a9..95e4b668 100644
--- a/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
+++ b/packages/next-urql/src/__tests__/with-urql-client.spec.tsx
@@ -97,11 +97,14 @@ describe('withUrqlClient', () => {
     it('should allow a user to access the ctx object from Next on the server', async () => {
       Component.getInitialProps &&
         (await Component.getInitialProps(mockContext));
-      expect(spyInitUrqlClient).toHaveBeenCalledWith({
-        url: 'http://localhost:3000',
-        fetchOptions: { headers: { Authorization: token } },
-        exchanges: [mockSsrExchange],
-      });
+      expect(spyInitUrqlClient).toHaveBeenCalledWith(
+        {
+          url: 'http://localhost:3000',
+          fetchOptions: { headers: { Authorization: token } },
+          exchanges: [mockSsrExchange],
+        },
+        true
+      );
     });
   });
 
diff --git a/packages/react-urql/src/test-utils/ssr.test.tsx b/packages/react-urql/src/test-utils/ssr.test.tsx
index a7ed5178..4083cead 100644
--- a/packages/react-urql/src/test-utils/ssr.test.tsx
+++ b/packages/react-urql/src/test-utils/ssr.test.tsx
@@ -148,13 +148,15 @@ describe('client-side rehydration', () => {
     });
   });
 
-  it('can rehydrate results on the client', () => {
+  it('can rehydrate results on the client', async () => {
     ssr.restoreData({ [queryOperation.key]: queryResponse });
 
     expect(() => {
       pipe(client.executeRequestOperation(queryOperation), publish);
     }).not.toThrow();
 
+    await Promise.resolve();
+
     const data = ssr.extractData();
     expect(Object.keys(data).length).toBe(0);
   });