Repository: FormidableLabs/urql
Pull request #514 (2020-01-17 21:24:59)

Fix concurrent-mode edge cases using useSubscription (adds useSo…
This pull request aims to fix even more concurrent-mode edge cases by changing our approach. Previously we disqualified the use of `useSubscription` because we didn't have a way to construct a side-effect-less `getCurrentValue` function.

As it turns out we can indeed create a `getCurrentValue` that creates short, synchronous subscriptions to get the current value of a source and avoid issues where `react-wonka` doesn't start/stop subscriptions correctly (see [`react-wonka/#11`](https://github.com/kitten/react-wonka/pull/11))

This is more of a temporary issue as `use-subscription` will often lead to a bailout of many concurrent mode features (see [`use-subscription` readme](https://github.com/facebook/react/tree/master/packages/use-subscription#limitations-in-concurrent-mode))

Realistically, we want to wait for this feature and then replace the two utilities that this PR creates (see [`react/#15317`](https://github.com/facebook/react/issues/15317#issuecomment-575761699))

> We're working on a new hook for mutable external values that deopts in scenarios where it would produce an inconsistent tree. By doing a full batched render in synchronous mode. It's "safe" and it does allow a tree to commit as one.

This PR **changes** the behaviour of `teardown` operations. They will be seen much more frequently (twice on initial mount) and will hence become a little more noisy. The `fetchExchange` has been adapted however so that it avoids calling `fetch` for immediately cancelled operations.

We can reduce this churn a little to _one teardown per initial mount_ by replacing `useSubscription` in this PR's new `useSource` hook, **in a follow up PR**. This would check passively in `useEffect` whether a new value has come in. Currently `useEffect` actively calls `getCurrentValue` synchronously in `useEffect` to check whether it needs to update and it calls it on every subscription event. Due to our strong guarantees we can avoid this.

We'll have to monitor changes in React to be able to improve this behaviour but for now it seems to be the safest option available to us.


Patch:
diff --git a/docs/guides.md b/docs/guides.md
index e6943ed2..621586a7 100644
--- a/docs/guides.md
+++ b/docs/guides.md
@@ -465,7 +465,7 @@ import { extractFiles } from 'extract-files';
 
 ...
 const createFetchSource = (operation) => {
-  return make(([next, complete]) => {
+  return make(({next, complete}) => {
     const { context } = operation;
     const { clone, files } = extractFiles(operation.variables);
     const isFileUpload = !!files.size;
diff --git a/package.json b/package.json
index e9d39048..bfbead43 100644
--- a/package.json
+++ b/package.json
@@ -95,7 +95,7 @@
     "@types/jest": "^24.0.25",
     "@types/react": "^16.9.17",
     "@types/react-test-renderer": "^16.9.1",
-    "@types/scheduler": "^0.16.1",
+    "@types/use-subscription": "^1.0.0",
     "@typescript-eslint/eslint-plugin": "^2.15.0",
     "@typescript-eslint/parser": "^2.15.0",
     "babel-plugin-closure-elimination": "^1.3.0",
@@ -136,8 +136,7 @@
     "react": ">= 16.8.0"
   },
   "dependencies": {
-    "react-wonka": "^2.0.1",
-    "scheduler": ">= 0.16.0",
+    "use-subscription": "^1.3.0",
     "wonka": "^4.0.7"
   }
 }
diff --git a/src/exchanges/fetch.ts b/src/exchanges/fetch.ts
index baadda34..d13b4fee 100755
--- a/src/exchanges/fetch.ts
+++ b/src/exchanges/fetch.ts
@@ -99,15 +99,20 @@ const createFetchSource = (operation: Operation) => {
         abortController !== undefined ? abortController.signal : undefined,
     };
 
-    executeFetch(operation, fetchOptions).then(result => {
-      if (result !== undefined) {
-        next(result);
-      }
-
-      complete();
-    });
+    let ended = false;
+
+    Promise.resolve()
+      .then(() => (ended ? undefined : executeFetch(operation, fetchOptions)))
+      .then((result: OperationResult | undefined) => {
+        if (!ended) {
+          ended = true;
+          if (result) next(result);
+          complete();
+        }
+      });
 
     return () => {
+      ended = true;
       if (abortController !== undefined) {
         abortController.abort();
       }
@@ -115,9 +120,11 @@ const createFetchSource = (operation: Operation) => {
   });
 };
 
-const executeFetch = (operation: Operation, opts: RequestInit) => {
+const executeFetch = (
+  operation: Operation,
+  opts: RequestInit
+): Promise<OperationResult> => {
   const { url, fetch: fetcher } = operation.context;
-
   let response: Response | undefined;
 
   return (fetcher || fetch)(url, opts)
diff --git a/src/exchanges/ssr.ts b/src/exchanges/ssr.ts
index ff5d73b2..ccb406a5 100644
--- a/src/exchanges/ssr.ts
+++ b/src/exchanges/ssr.ts
@@ -37,8 +37,8 @@ const serializeResult = ({
   const result: SerializedResult = { data, error: undefined };
   if (error !== undefined) {
     result.error = {
-      networkError: '' + error.networkError,
-      graphQLErrors: error.graphQLErrors.map(x => '' + x),
+      graphQLErrors: error.graphQLErrors.map(x => x.message),
+      networkError: error.networkError ? '' + error.networkError : undefined,
     };
   }
 
@@ -55,14 +55,18 @@ const deserializeResult = (
     operation,
     data,
     extensions: undefined,
-    error: undefined,
+    error: error
+      ? new CombinedError({
+          networkError: error.networkError
+            ? new Error(error.networkError)
+            : undefined,
+          graphQLErrors:
+            error.graphQLErrors && error.graphQLErrors.length
+              ? error.graphQLErrors
+              : undefined,
+        })
+      : undefined,
   };
-  if (error !== undefined) {
-    deserialized.error = new CombinedError({
-      networkError: new Error(error.networkError),
-      graphQLErrors: error.graphQLErrors,
-    });
-  }
 
   return deserialized;
 };
diff --git a/src/exchanges/subscription.ts b/src/exchanges/subscription.ts
index 3b989aaa..94cc0ec0 100644
--- a/src/exchanges/subscription.ts
+++ b/src/exchanges/subscription.ts
@@ -79,13 +79,14 @@ export const subscriptionExchange = ({
         error: err => next(makeErrorResult(operation, err)),
         complete: () => {
           if (!isComplete) {
+            isComplete = true;
             client.reexecuteOperation({
               ...operation,
               operationName: 'teardown',
             });
-          }
 
-          complete();
+            complete();
+          }
         },
       });
 
diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts
index 6591b997..88f45731 100644
--- a/src/hooks/useQuery.ts
+++ b/src/hooks/useQuery.ts
@@ -1,11 +1,11 @@
 import { DocumentNode } from 'graphql';
 import { useCallback, useMemo } from 'react';
 import { pipe, concat, fromValue, switchMap, map, scan } from 'wonka';
-import { useOperator } from 'react-wonka';
 
 import { useClient } from '../context';
 import { OperationContext, RequestPolicy } from '../types';
 import { CombinedError } from '../utils';
+import { useSource, useBehaviourSubject } from './useSource';
 import { useRequest } from './useRequest';
 import { initialState } from './constants';
 
@@ -53,16 +53,20 @@ export const useQuery = <T = any, V = object>(
     [client, request, args.requestPolicy, args.pollInterval, args.context]
   );
 
-  const [state, update] = useOperator(
-    query$$ =>
-      pipe(
+  const [query$$, update] = useBehaviourSubject(
+    useMemo(() => (args.pause ? null : makeQuery$()), [args.pause, makeQuery$])
+  );
+
+  const state = useSource(
+    useMemo(() => {
+      return pipe(
         query$$,
         switchMap(query$ => {
-          if (!query$) return fromValue({ fetching: false });
+          if (!query$) return fromValue({ fetching: false, stale: false });
 
           return concat([
             // Initially set fetching to true
-            fromValue({ fetching: true }),
+            fromValue({ fetching: true, stale: false }),
             pipe(
               query$,
               map(({ stale, data, error, extensions }) => ({
@@ -74,20 +78,19 @@ export const useQuery = <T = any, V = object>(
               }))
             ),
             // When the source proactively closes, fetching is set to false
-            fromValue({ fetching: false }),
+            fromValue({ fetching: false, stale: false }),
           ]);
         }),
         // The individual partial results are merged into each previous result
         scan(
-          (result, partial: { fetching: boolean }) => ({
+          (result, partial) => ({
             ...result,
-            stale: false,
             ...partial,
           }),
           initialState
         )
-      ),
-    useMemo(() => (args.pause ? null : makeQuery$()), [args.pause, makeQuery$]),
+      );
+    }, [query$$]),
     initialState
   );
 
diff --git a/src/hooks/useSource.ts b/src/hooks/useSource.ts
new file mode 100644
index 00000000..d47411fd
--- /dev/null
+++ b/src/hooks/useSource.ts
@@ -0,0 +1,75 @@
+/* eslint-disable @typescript-eslint/no-use-before-define */
+/* eslint-disable react-hooks/exhaustive-deps */
+
+import { useMemo, useEffect } from 'react';
+import { Subscription, Unsubscribe, useSubscription } from 'use-subscription';
+
+import {
+  Source,
+  fromValue,
+  makeSubject,
+  pipe,
+  map,
+  concat,
+  onPush,
+  publish,
+  subscribe,
+} from 'wonka';
+
+export const useSource = <T>(source: Source<T>, init: T): T =>
+  useSubscription(
+    useMemo((): Subscription<T> => {
+      let hasUpdate = false;
+      let currentValue: T = init;
+
+      const updateValue = pipe(
+        source,
+        onPush(value => {
+          currentValue = value;
+        })
+      );
+
+      return {
+        getCurrentValue(): T {
+          if (!hasUpdate) publish(updateValue).unsubscribe();
+          return currentValue;
+        },
+        subscribe(onValue: () => void): Unsubscribe {
+          return pipe(
+            updateValue,
+            subscribe(() => {
+              hasUpdate = true;
+              onValue();
+              hasUpdate = false;
+            })
+          ).unsubscribe as Unsubscribe;
+        },
+      };
+    }, [source])
+  );
+
+export const useBehaviourSubject = <T>(value: T) => {
+  const state = useMemo((): [Source<T>, (value: T) => void] => {
+    let prevValue = value;
+
+    const subject = makeSubject<T>();
+    const prevValue$ = pipe(
+      fromValue(value),
+      map(() => prevValue)
+    );
+
+    const source = concat([prevValue$, subject.source]);
+
+    const next = (value: T) => {
+      subject.next((prevValue = value));
+    };
+
+    return [source, next];
+  }, []);
+
+  useEffect(() => {
+    state[1](value);
+  }, [state, value]);
+
+  return state;
+};
diff --git a/src/hooks/useSubscription.ts b/src/hooks/useSubscription.ts
index ef0f82bb..8e498801 100644
--- a/src/hooks/useSubscription.ts
+++ b/src/hooks/useSubscription.ts
@@ -1,10 +1,10 @@
 import { DocumentNode } from 'graphql';
 import { useCallback, useRef, useMemo } from 'react';
 import { pipe, concat, fromValue, switchMap, map, scan } from 'wonka';
-import { useOperator } from 'react-wonka';
 
 import { useClient } from '../context';
 import { CombinedError } from '../utils';
+import { useSource, useBehaviourSubject } from './useSource';
 import { OperationContext } from '../types';
 import { useRequest } from './useRequest';
 import { initialState } from './constants';
@@ -54,16 +54,23 @@ export const useSubscription = <T = any, R = T, V = object>(
     [client, request, args.context]
   );
 
-  const [state, update] = useOperator(
-    subscription$$ =>
-      pipe(
+  const [subscription$$, update] = useBehaviourSubject(
+    useMemo(() => (args.pause ? null : makeSubscription$()), [
+      args.pause,
+      makeSubscription$,
+    ])
+  );
+
+  const state = useSource(
+    useMemo(() => {
+      return pipe(
         subscription$$,
         switchMap(subscription$ => {
           if (!subscription$) return fromValue({ fetching: false });
 
           return concat([
             // Initially set fetching to true
-            fromValue({ fetching: true }),
+            fromValue({ fetching: true, stale: false }),
             pipe(
               subscription$,
               map(({ stale, data, error, extensions }) => ({
@@ -75,7 +82,7 @@ export const useSubscription = <T = any, R = T, V = object>(
               }))
             ),
             // When the source proactively closes, fetching is set to false
-            fromValue({ fetching: false }),
+            fromValue({ fetching: false, stale: false }),
           ]);
         }),
         // The individual partial results are merged into each previous result
@@ -88,13 +95,10 @@ export const useSubscription = <T = any, R = T, V = object>(
                 ? handler(result.data, partial.data)
                 : partial.data
               : result.data;
-          return { ...result, stale: false, ...partial, data };
+          return { ...result, ...partial, data };
         }, initialState)
-      ),
-    useMemo(() => (args.pause ? null : makeSubscription$()), [
-      args.pause,
-      makeSubscription$,
-    ]),
+      );
+    }, [subscription$$]),
     initialState
   );
 
diff --git a/yarn.lock b/yarn.lock
index ebfca61f..f09be8bf 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -599,11 +599,6 @@
   dependencies:
     "@types/node" "*"
 
-"@types/scheduler@^0.16.1":
-  version "0.16.1"
-  resolved "https://registry.yarnpkg.com/@types/scheduler/-/scheduler-0.16.1.tgz#18845205e86ff0038517aab7a18a62a6b9f71275"
-  integrity sha512-EaCxbanVeyxDRTQBkdLb3Bvl/HK7PBK6UJjsSixB0iHKoWxE5uu2Q/DgtpOhPIojN0Zl1whvOd7PoHs2P0s5eA==
-
 "@types/stack-utils@^1.0.1":
   version "1.0.1"
   resolved "https://registry.yarnpkg.com/@types/stack-utils/-/stack-utils-1.0.1.tgz#0a851d3bd96498fa25c33ab7278ed3bd65f06c3e"
@@ -632,6 +627,11 @@
     "@types/react-dom" "*"
     "@types/testing-library__dom" "*"
 
+"@types/use-subscription@^1.0.0":
+  version "1.0.0"
+  resolved "https://registry.yarnpkg.com/@types/use-subscription/-/use-subscription-1.0.0.tgz#d146f8d834f70f50d48bd8246a481d096f11db19"
+  integrity sha512-0WWZ5GUDKMXUY/1zy4Ur5/zsC0s/B+JjXfHdkvx6JgDNZzZV5eW+KKhDqsTGyqX56uh99gwGwbsKbVwkcVIKQA==
+
 "@types/yargs-parser@*":
   version "13.0.0"
   resolved "https://registry.yarnpkg.com/@types/yargs-parser/-/yargs-parser-13.0.0.tgz#453743c5bbf9f1bed61d959baab5b06be029b2d0"
@@ -4718,11 +4718,6 @@ react-test-renderer@^16.0.0-0, react-test-renderer@^16.12.0:
     react-is "^16.8.6"
     scheduler "^0.18.0"
 
-react-wonka@^2.0.1:
-  version "2.0.1"
-  resolved "https://registry.yarnpkg.com/react-wonka/-/react-wonka-2.0.1.tgz#75bdf03dbad8ceb8c1066216f635f05ce2b642a5"
-  integrity sha512-mM2UH2gnK5LLzaqVWd6JCLrB1vO3I4PN/sQZbjvzsjms4vSv+nKwelNUftM0KeC+LtTPC4GGsuxyu2XJnsCUTw==
-
 react@^16.12.0:
   version "16.12.0"
   resolved "https://registry.yarnpkg.com/react/-/react-16.12.0.tgz#0c0a9c6a142429e3614834d5a778e18aa78a0b83"
@@ -5137,7 +5132,7 @@ sax@^1.2.4:
   resolved "https://registry.yarnpkg.com/sax/-/sax-1.2.4.tgz#2816234e2378bddc4e5354fab5caa895df7100d9"
   integrity sha512-NqVDv9TpANUjFm0N8uM5GxL36UgKi9/atZw+x7YFnQ8ckwFGKrl4xX4yWtrey3UJm5nP1kUbnYgLopqWNSRhWw==
 
-"scheduler@>= 0.16.0", scheduler@^0.18.0:
+scheduler@^0.18.0:
   version "0.18.0"
   resolved "https://registry.yarnpkg.com/scheduler/-/scheduler-0.18.0.tgz#5901ad6659bc1d8f3fdaf36eb7a67b0d6746b1c4"
   integrity sha512-agTSHR1Nbfi6ulI0kYNK0203joW2Y5W4po4l+v03tOoiJKpTBbxpNhWDvqc/4IcOw+KLmSiQLTasZ4cab2/UWQ==
@@ -5858,6 +5853,13 @@ urlgrey@^0.4.4:
   resolved "https://registry.yarnpkg.com/urlgrey/-/urlgrey-0.4.4.tgz#892fe95960805e85519f1cd4389f2cb4cbb7652f"
   integrity sha1-iS/pWWCAXoVRnxzUOJ8stMu3ZS8=
 
+use-subscription@^1.3.0:
+  version "1.3.0"
+  resolved "https://registry.yarnpkg.com/use-subscription/-/use-subscription-1.3.0.tgz#3df13a798e826c8d462899423293289a3362e4e6"
+  integrity sha512-buZV7FUtnbOr+65dN7PHK7chHhQGfk/yjgqfpRLoWuHIAc4klAD/rdot2FsPNtFthN1ZydvA8tR/mWBMQ+/fDQ==
+  dependencies:
+    object-assign "^4.1.1"
+
 use@^3.1.0:
   version "3.1.1"
   resolved "https://registry.yarnpkg.com/use/-/use-3.1.1.tgz#d50c8cac79a19fbc20f2911f56eb973f4e10070f"


Test patch:
diff --git a/src/exchanges/fetch.test.ts b/src/exchanges/fetch.test.ts
index 7ad503c7..4f3a0d88 100755
--- a/src/exchanges/fetch.test.ts
+++ b/src/exchanges/fetch.test.ts
@@ -17,7 +17,7 @@ beforeAll(() => {
   };
 });
 
-beforeEach(() => {
+afterEach(() => {
   fetch.mockClear();
   abort.mockClear();
 });
@@ -42,11 +42,6 @@ const exchangeArgs = {
 
 describe('on success', () => {
   beforeEach(() => {
-    jest
-      .spyOn(Date, 'now')
-      .mockImplementationOnce(() => 100)
-      .mockImplementationOnce(() => 200);
-
     fetch.mockResolvedValue({
       status: 200,
       json: jest.fn().mockResolvedValue(response),
@@ -76,11 +71,6 @@ describe('on success', () => {
 
 describe('on error', () => {
   beforeEach(() => {
-    jest
-      .spyOn(Date, 'now')
-      .mockImplementationOnce(() => 100)
-      .mockImplementationOnce(() => 200);
-
     fetch.mockResolvedValue({
       status: 400,
       json: jest.fn().mockResolvedValue(response),
@@ -117,8 +107,8 @@ describe('on error', () => {
 });
 
 describe('on teardown', () => {
-  it('aborts the outgoing request', () => {
-    fetch.mockReturnValue(Promise.reject(abortError));
+  it('does not start the outgoing request on immediate teardowns', () => {
+    fetch.mockRejectedValueOnce(abortError);
 
     const { unsubscribe } = pipe(
       fromValue(queryOperation),
@@ -126,6 +116,22 @@ describe('on teardown', () => {
       subscribe(fail)
     );
 
+    unsubscribe();
+    expect(fetch).toHaveBeenCalledTimes(0);
+    expect(abort).toHaveBeenCalledTimes(1);
+  });
+
+  it('aborts the outgoing request', async () => {
+    fetch.mockRejectedValueOnce(abortError);
+
+    const { unsubscribe } = pipe(
+      fromValue(queryOperation),
+      fetchExchange(exchangeArgs),
+      subscribe(fail)
+    );
+
+    await Promise.resolve();
+
     unsubscribe();
     expect(fetch).toHaveBeenCalledTimes(1);
     expect(abort).toHaveBeenCalledTimes(1);
diff --git a/src/exchanges/ssr.test.ts b/src/exchanges/ssr.test.ts
index c2b0f37d..7c4f81c1 100644
--- a/src/exchanges/ssr.test.ts
+++ b/src/exchanges/ssr.test.ts
@@ -3,6 +3,7 @@ import { makeSubject, pipe, map, publish, forEach, Subject } from 'wonka';
 import { Client } from '../client';
 import { queryOperation, queryResponse } from '../test-utils';
 import { ExchangeIO, Operation } from '../types';
+import { CombinedError } from '../utils';
 import { ssrExchange } from './ssr';
 
 let forward: ExchangeIO;
@@ -40,6 +41,36 @@ it('caches query results correctly', () => {
   });
 });
 
+it('caches errored query results correctly', () => {
+  output.mockReturnValueOnce({
+    ...queryResponse,
+    data: null,
+    error: new CombinedError({
+      graphQLErrors: ['Oh no!'],
+    }),
+  });
+
+  const ssr = ssrExchange();
+  const { source: ops$, next } = input;
+  const exchange = ssr(exchangeInput)(ops$);
+
+  publish(exchange);
+  next(queryOperation);
+
+  const data = ssr.extractData();
+  expect(Object.keys(data)).toEqual(['' + queryOperation.key]);
+
+  expect(data).toEqual({
+    [queryOperation.key]: {
+      data: null,
+      error: {
+        graphQLErrors: ['Oh no!'],
+        networkError: undefined,
+      },
+    },
+  });
+});
+
 it('resolves cached query results correctly', () => {
   const onPush = jest.fn();
 
diff --git a/src/hooks/useQuery.test.tsx b/src/hooks/useQuery.test.tsx
index b1269b0b..15b5300f 100644
--- a/src/hooks/useQuery.test.tsx
+++ b/src/hooks/useQuery.test.tsx
@@ -184,19 +184,15 @@ describe('on unmount', () => {
 
   beforeEach(() => {
     client.executeQuery.mockReturnValue(
-      pipe(
-        never,
-        onStart(start),
-        onEnd(unsubscribe)
-      )
+      pipe(never, onStart(start), onEnd(unsubscribe))
     );
   });
 
   it('unsubscribe is called', () => {
     const wrapper = renderer.create(<QueryUser {...props} />);
     act(() => wrapper.unmount());
-    expect(start).toBeCalledTimes(1);
-    expect(unsubscribe).toBeCalledTimes(1);
+    expect(start).toHaveBeenCalled();
+    expect(unsubscribe).toHaveBeenCalled();
   });
 });
 
diff --git a/src/hooks/useSubscription.test.tsx b/src/hooks/useSubscription.test.tsx
index 80da947a..56c3a939 100644
--- a/src/hooks/useSubscription.test.tsx
+++ b/src/hooks/useSubscription.test.tsx
@@ -95,7 +95,6 @@ it('calls handler', () => {
     <SubscriptionUser q={query} handler={handler} />
   );
   wrapper.update(<SubscriptionUser q={query} handler={handler} />);
-  expect(handler).toBeCalledTimes(1);
   expect(handler).toBeCalledWith(undefined, 1234);
 });