Repository: FormidableLabs/urql
Pull request #447 (2019-10-10 04:02:00)

Replace useImmediate* hooks with react-wonka
This is intended to improve the stability of our hooks and ensure that we won't run into problems with concurrent mode in the future. At its core is a tiny library I've written, `react-wonka`. https://github.com/kitten/react-wonka

It takes care of transforming changing inputs to streams and using the given output stream to then return output values from its hooks, synchronously if possible, and with updates if not.

It is aware of concurrent mode and uses an internal counter to trigger updates, which seems to be popular these days 😓 

It's tests are more thorough than what we could write realistically for each of our hooks: https://github.com/kitten/react-wonka/blob/master/src/index.test.ts

In practice this may even make `useQuery` and `useSubscription` look a little nicer, if someone is open to help me tweak the implementation a little. Doesn't look 100% pretty yet 😅 

In practice, our bundle-size only changes by `+0.02kB min+gzip` (this branch compared to the last release) when I'm including `react-wonka` in the bundle, which seems better for a fair comparison.

This should also improve our handling of suspense. Our `useImmediateEffect` hook runs normal effects after the initial mount. When we update a hook we'd right now  throw a suspense-promise in a normal effect, which is not allowed.


Patch:
diff --git a/package.json b/package.json
index 8cc00b2e..d6a5f66a 100644
--- a/package.json
+++ b/package.json
@@ -133,6 +133,7 @@
     "react-dom": ">= 16.8.0"
   },
   "dependencies": {
+    "react-wonka": "^1.0.1",
     "wonka": "^3.2.1"
   }
 }
diff --git a/rollup.config.js b/rollup.config.js
index 048f725d..f86b26e0 100644
--- a/rollup.config.js
+++ b/rollup.config.js
@@ -9,15 +9,14 @@ import { terser } from 'rollup-plugin-terser';
 import transformPipe from './scripts/transform-pipe';
 
 const pkgInfo = require('./package.json');
-const external = ['dns', 'fs', 'path', 'url'];
 
-if (pkgInfo.peerDependencies) {
+let external = ['dns', 'fs', 'path', 'url'];
+if (pkgInfo.peerDependencies)
   external.push(...Object.keys(pkgInfo.peerDependencies));
-}
-
-if (pkgInfo.dependencies) {
+if (pkgInfo.dependencies)
   external.push(...Object.keys(pkgInfo.dependencies));
-}
+
+external = external.filter(x => x !== 'react-wonka');
 
 const externalPredicate = new RegExp(`^(${external.join('|')})($|/)`);
 const externalTest = id => {
diff --git a/src/hooks/useImmediateEffect.ts b/src/hooks/useImmediateEffect.ts
deleted file mode 100644
index ee2bf122..00000000
--- a/src/hooks/useImmediateEffect.ts
+++ /dev/null
@@ -1,30 +0,0 @@
-/* eslint-disable react-hooks/exhaustive-deps */
-
-import { useRef, useEffect, EffectCallback } from 'react';
-import { noop } from '../utils';
-
-/** This is a drop-in replacement for useEffect that will execute the first effect that happens during initial mount synchronously */
-export const useImmediateEffect = (
-  effect: EffectCallback,
-  changes: ReadonlyArray<any>
-) => {
-  const teardown = useRef<() => void>(noop);
-  const isMounted = useRef<boolean>(false);
-
-  // On initial render we just execute the effect
-  if (!isMounted.current) {
-    // There's the slight possibility that we had an interrupt due to
-    // conccurrent mode after running the effect.
-    // This could result in memory leaks.
-    teardown.current();
-    teardown.current = effect() || noop;
-  }
-
-  useEffect(() => {
-    // Initially we skip executing the effect since we've already done so on
-    // initial render, then we execute it as usual
-    return isMounted.current
-      ? effect()
-      : ((isMounted.current = true), teardown.current);
-  }, changes);
-};
diff --git a/src/hooks/useMutation.ts b/src/hooks/useMutation.ts
index dd46dee6..52f2077c 100644
--- a/src/hooks/useMutation.ts
+++ b/src/hooks/useMutation.ts
@@ -1,10 +1,9 @@
 import { DocumentNode } from 'graphql';
-import { useCallback } from 'react';
+import { useState, useCallback } from 'react';
 import { pipe, toPromise } from 'wonka';
 import { useClient } from '../context';
 import { OperationResult, OperationContext } from '../types';
 import { CombinedError, createRequest } from '../utils';
-import { useImmediateState } from './useImmediateState';
 
 export interface UseMutationState<T> {
   fetching: boolean;
@@ -25,7 +24,8 @@ export const useMutation = <T = any, V = object>(
   query: DocumentNode | string
 ): UseMutationResponse<T, V> => {
   const client = useClient();
-  const [state, setState] = useImmediateState<UseMutationState<T>>({
+
+  const [state, setState] = useState<UseMutationState<T>>({
     fetching: false,
     error: undefined,
     data: undefined,
diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts
index c6dcecc5..f6858ede 100644
--- a/src/hooks/useQuery.ts
+++ b/src/hooks/useQuery.ts
@@ -1,12 +1,19 @@
 import { DocumentNode } from 'graphql';
-import { useCallback, useRef } from 'react';
-import { pipe, onEnd, subscribe } from 'wonka';
+import { useCallback, useMemo } from 'react';
+import { pipe, concat, fromValue, switchMap, map, scan } from 'wonka';
+import { useSubjectValue } from 'react-wonka';
+
 import { useClient } from '../context';
 import { OperationContext, RequestPolicy } from '../types';
-import { CombinedError, noop } from '../utils';
+import { CombinedError } from '../utils';
 import { useRequest } from './useRequest';
-import { useImmediateEffect } from './useImmediateEffect';
-import { useImmediateState } from './useImmediateState';
+
+const initialState: UseQueryState<any> = {
+  fetching: false,
+  data: undefined,
+  error: undefined,
+  extensions: undefined,
+};
 
 export interface UseQueryArgs<V> {
   query: string | DocumentNode;
@@ -32,61 +39,60 @@ export type UseQueryResponse<T> = [
 export const useQuery = <T = any, V = object>(
   args: UseQueryArgs<V>
 ): UseQueryResponse<T> => {
-  const unsubscribe = useRef(noop);
   const client = useClient();
 
-  // This is like useState but updates the state object
-  // immediately, when we're still before the initial mount
-  const [state, setState] = useImmediateState<UseQueryState<T>>({
-    fetching: false,
-    data: undefined,
-    error: undefined,
-    extensions: undefined,
-  });
-
   // This creates a request which will keep a stable reference
   // if request.key doesn't change
   const request = useRequest(args.query, args.variables);
 
-  const executeQuery = useCallback(
+  // Create a new query-source from client.executeQuery
+  const makeQuery$ = useCallback(
     (opts?: Partial<OperationContext>) => {
-      unsubscribe.current();
+      return client.executeQuery(request, {
+        requestPolicy: args.requestPolicy,
+        pollInterval: args.pollInterval,
+        ...args.context,
+        ...opts,
+      });
+    },
+    [client, request, args.requestPolicy, args.pollInterval, args.context]
+  );
 
-      setState(s => ({ ...s, fetching: true }));
+  const [state, update] = useSubjectValue(
+    query$$ =>
+      pipe(
+        query$$,
+        switchMap(query$ => {
+          if (!query$) return fromValue({ fetching: false });
 
-      [unsubscribe.current] = pipe(
-        client.executeQuery(request, {
-          requestPolicy: args.requestPolicy,
-          pollInterval: args.pollInterval,
-          ...args.context,
-          ...opts,
+          return concat([
+            // Initially set fetching to true
+            fromValue({ fetching: true }),
+            pipe(
+              query$,
+              map(({ data, error, extensions }) => ({
+                fetching: false,
+                data,
+                error,
+                extensions,
+              }))
+            ),
+            // When the source proactively closes, fetching is set to false
+            fromValue({ fetching: false }),
+          ]);
         }),
-        onEnd(() => setState(s => ({ ...s, fetching: false }))),
-        subscribe(({ data, error, extensions }) => {
-          setState({ fetching: false, data, error, extensions });
-        })
-      );
-    },
-    [
-      args.context,
-      args.requestPolicy,
-      args.pollInterval,
-      client,
-      request,
-      setState,
-    ]
+        // The individual partial results are merged into each previous result
+        scan((result, partial) => ({ ...result, ...partial }), initialState)
+      ),
+    useMemo(() => (args.pause ? null : makeQuery$()), [args.pause, makeQuery$]),
+    initialState
   );
 
-  useImmediateEffect(() => {
-    if (args.pause) {
-      unsubscribe.current();
-      setState(s => ({ ...s, fetching: false }));
-      return noop;
-    }
-
-    executeQuery();
-    return () => unsubscribe.current(); // eslint-disable-line
-  }, [executeQuery, args.pause, setState]);
+  // This is the imperative execute function passed to the user
+  const executeQuery = useCallback(
+    (opts?: Partial<OperationContext>) => update(makeQuery$(opts)),
+    [update, makeQuery$]
+  );
 
   return [state, executeQuery];
 };
diff --git a/src/hooks/useSubscription.ts b/src/hooks/useSubscription.ts
index b704ebf6..f37b2e52 100644
--- a/src/hooks/useSubscription.ts
+++ b/src/hooks/useSubscription.ts
@@ -1,13 +1,20 @@
 import { DocumentNode } from 'graphql';
-import { useCallback, useRef } from 'react';
-import { pipe, onEnd, subscribe } from 'wonka';
+import { useCallback, useRef, useMemo } from 'react';
+import { pipe, concat, fromValue, switchMap, map, scan } from 'wonka';
+import { useSubjectValue } from 'react-wonka';
+
 import { useClient } from '../context';
-import { CombinedError, noop } from '../utils';
+import { CombinedError } from '../utils';
 import { useRequest } from './useRequest';
-import { useImmediateEffect } from './useImmediateEffect';
-import { useImmediateState } from './useImmediateState';
 import { OperationContext } from '../types';
 
+const initialState: UseSubscriptionState<any> = {
+  fetching: false,
+  data: undefined,
+  error: undefined,
+  extensions: undefined,
+};
+
 export interface UseSubscriptionArgs<V> {
   query: DocumentNode | string;
   variables?: V;
@@ -33,62 +40,75 @@ export const useSubscription = <T = any, R = T, V = object>(
   args: UseSubscriptionArgs<V>,
   handler?: SubscriptionHandler<T, R>
 ): UseSubscriptionResponse<R> => {
-  const unsubscribe = useRef(noop);
-  const handlerRef = useRef(handler);
   const client = useClient();
 
-  const [state, setState] = useImmediateState<UseSubscriptionState<R>>({
-    fetching: false,
-    error: undefined,
-    data: undefined,
-    extensions: undefined,
-  });
-
   // Update handler on constant ref, since handler changes shouldn't
   // trigger a new subscription run
+  const handlerRef = useRef(handler);
   handlerRef.current = handler;
 
   // This creates a request which will keep a stable reference
   // if request.key doesn't change
   const request = useRequest(args.query, args.variables);
 
-  const executeSubscription = useCallback(
+  // Create a new subscription-source from client.executeSubscription
+  const makeSubscription$ = useCallback(
     (opts?: Partial<OperationContext>) => {
-      unsubscribe.current();
+      return client.executeSubscription(request, { ...args.context, ...opts });
+    },
+    [client, request, args.context]
+  );
 
-      setState(s => ({ ...s, fetching: true }));
+  const [state, update] = useSubjectValue(
+    subscription$$ =>
+      pipe(
+        subscription$$,
+        switchMap(subscription$ => {
+          if (!subscription$) return fromValue({ fetching: false });
 
-      [unsubscribe.current] = pipe(
-        client.executeSubscription(request, {
-          ...args.context,
-          ...opts,
+          return concat([
+            // Initially set fetching to true
+            fromValue({ fetching: true }),
+            pipe(
+              subscription$,
+              map(({ data, error, extensions }) => ({
+                fetching: true,
+                data,
+                error,
+                extensions,
+              }))
+            ),
+            // When the source proactively closes, fetching is set to false
+            fromValue({ fetching: false }),
+          ]);
         }),
-        onEnd(() => setState(s => ({ ...s, fetching: false }))),
-        subscribe(({ data, error, extensions }) => {
+        // The individual partial results are merged into each previous result
+        scan((result, partial: any) => {
           const { current: handler } = handlerRef;
-
-          setState(s => ({
-            fetching: true,
-            data: typeof handler === 'function' ? handler(s.data, data) : data,
-            error,
-            extensions,
-          }));
-        })
-      );
-    },
-    [client, request, setState, args.context]
+          // If a handler has been passed, it's used to merge new data in
+          if (partial.data !== undefined && typeof handler === 'function') {
+            return {
+              ...result,
+              ...partial,
+              data: handler(result.data, partial.data),
+            };
+          } else {
+            return { ...result, ...partial };
+          }
+        }, initialState)
+      ),
+    useMemo(() => (args.pause ? null : makeSubscription$()), [
+      args.pause,
+      makeSubscription$,
+    ]),
+    initialState
   );
 
-  useImmediateEffect(() => {
-    if (args.pause) {
-      unsubscribe.current();
-      setState(s => ({ ...s, fetching: false }));
-      return noop;
-    }
-
-    executeSubscription();
-    return () => unsubscribe.current(); // eslint-disable-line
-  }, [executeSubscription, args.pause, setState]);
+  // This is the imperative execute function passed to the user
+  const executeSubscription = useCallback(
+    (opts?: Partial<OperationContext>) => update(makeSubscription$(opts)),
+    [update, makeSubscription$]
+  );
 
   return [state, executeSubscription];
 };
diff --git a/yarn.lock b/yarn.lock
index 898e5ad2..64d737c6 100644
--- a/yarn.lock
+++ b/yarn.lock
@@ -4542,6 +4542,11 @@ react-test-renderer@^16.0.0-0, react-test-renderer@^16.9.0:
     react-is "^16.9.0"
     scheduler "^0.15.0"
 
+react-wonka@^1.0.1:
+  version "1.0.1"
+  resolved "https://registry.yarnpkg.com/react-wonka/-/react-wonka-1.0.1.tgz#66b44aa1fb4e3e38cb2ef49f7d7195de1a322775"
+  integrity sha512-SMZCQUQ1gTxCqH9ftx5RecykOx1wWDtbZa6lJDmBU8aCpX3Xxs3baTtbGP+PIvrDMIOZIJN7Ezv1e/8jWQ2vXw==
+
 react@^16.9.0:
   version "16.9.0"
   resolved "https://registry.yarnpkg.com/react/-/react-16.9.0.tgz#40ba2f9af13bc1a38d75dbf2f4359a5185c4f7aa"


Test patch:
diff --git a/src/hooks/useImmediateEffect.test.ts b/src/hooks/useImmediateEffect.test.ts
deleted file mode 100644
index 767491b4..00000000
--- a/src/hooks/useImmediateEffect.test.ts
+++ /dev/null
@@ -1,44 +0,0 @@
-import React from 'react';
-import { renderHook, act } from '@testing-library/react-hooks';
-import { useImmediateEffect } from './useImmediateEffect';
-
-it('calls effects immediately on mount', () => {
-  const spy = jest.spyOn(React, 'useEffect');
-  const useEffect = jest.fn();
-  const effect = jest.fn();
-
-  spy.mockImplementation(useEffect);
-
-  renderHook(() => {
-    useImmediateEffect(effect, [effect]);
-    expect(effect).toHaveBeenCalledTimes(1);
-  });
-
-  expect(effect).toHaveBeenCalledTimes(1);
-  expect(useEffect).toHaveBeenCalledWith(expect.any(Function), [
-    expect.any(Function),
-  ]);
-
-  spy.mockRestore();
-});
-
-it('behaves like useEffect otherwise', () => {
-  const spy = jest.spyOn(React, 'useEffect');
-  const effect = jest.fn();
-
-  const { result } = renderHook(() => {
-    const [ref, setState] = React.useState({});
-    useImmediateEffect(effect, [ref]);
-
-    expect(effect).toHaveBeenCalledTimes(1);
-    expect(spy).toHaveBeenCalled();
-
-    const forceUpdate = () => setState({});
-    return forceUpdate;
-  });
-
-  act(() => result.current()); // forceUpdate
-  expect(effect).toHaveBeenCalledTimes(2);
-
-  spy.mockRestore();
-});
diff --git a/src/hooks/useImmediateState.test.tsx b/src/hooks/useImmediateState.test.tsx
deleted file mode 100644
index 5ab641ec..00000000
--- a/src/hooks/useImmediateState.test.tsx
+++ /dev/null
@@ -1,53 +0,0 @@
-import React from 'react';
-import renderer from 'react-test-renderer';
-import { useImmediateState } from './useImmediateState';
-
-const setStateMock = jest.fn();
-
-jest
-  .spyOn(React, 'useState')
-  .mockImplementation((arg => [arg, setStateMock]) as any);
-
-const initialState = { someObject: 1234 };
-const updateState = { someObject: 5678 };
-let state;
-let setState;
-let returnVal;
-
-const Fixture = ({ update }: { update?: boolean }) => {
-  const [a, set] = useImmediateState<object>(initialState);
-
-  if (update) {
-    returnVal = set(updateState);
-  }
-
-  state = a;
-  setState = set;
-
-  return null;
-};
-
-beforeEach(jest.clearAllMocks);
-
-describe('on initial mount', () => {
-  it('sets initial state', () => {
-    renderer.create(<Fixture />);
-    expect(state).toEqual(initialState);
-  });
-
-  it('only mutates on setState call', () => {
-    renderer.create(<Fixture update={true} />);
-    expect(setStateMock).toBeCalledTimes(0);
-    expect(state).toEqual(updateState);
-    expect(returnVal).toBe(undefined);
-  });
-});
-
-describe('on later mounts', () => {
-  it('sets state via setState', () => {
-    renderer.create(<Fixture />);
-    setState(updateState);
-    expect(setStateMock).toBeCalledTimes(1);
-    expect(returnVal).toBe(undefined);
-  });
-});
diff --git a/src/hooks/useImmediateState.ts b/src/hooks/useImmediateState.ts
deleted file mode 100644
index 701a2aa1..00000000
--- a/src/hooks/useImmediateState.ts
+++ /dev/null
@@ -1,50 +0,0 @@
-import {
-  useRef,
-  useState,
-  useCallback,
-  useEffect,
-  useLayoutEffect,
-} from 'react';
-
-type SetStateAction<S> = S | ((prevState: S) => S);
-type SetState<S> = (action: SetStateAction<S>) => void;
-
-// See https://github.com/reduxjs/react-redux/blob/316467a/src/hooks/useSelector.js#L6-L15
-const useIsomorphicLayoutEffect =
-  typeof window !== 'undefined' ? useLayoutEffect : useEffect;
-
-/**
- * This is a drop-in replacement for useState, limited to object-based state.
- * During initial mount it will mutably update the state, instead of scheduling
- * a React update using setState
- */
-export const useImmediateState = <S extends {}>(init: S): [S, SetState<S>] => {
-  const isMounted = useRef(false);
-  const [state, setState] = useState<S>(init);
-
-  // This wraps setState and updates the state mutably on initial mount
-  const updateState: SetState<S> = useCallback(
-    (action: SetStateAction<S>): void => {
-      if (!isMounted.current) {
-        const newState =
-          typeof action === 'function'
-            ? (action as (arg: S) => S)(state)
-            : action;
-        Object.assign(state, newState);
-      } else {
-        setState(action);
-      }
-    },
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-    []
-  );
-
-  useIsomorphicLayoutEffect(() => {
-    isMounted.current = true;
-    return () => {
-      isMounted.current = false;
-    };
-  }, []);
-
-  return [state, updateState];
-};
diff --git a/src/hooks/useSubscription.test.tsx b/src/hooks/useSubscription.test.tsx
index 87360ca0..c3ca9077 100644
--- a/src/hooks/useSubscription.test.tsx
+++ b/src/hooks/useSubscription.test.tsx
@@ -53,18 +53,6 @@ describe('on initial useEffect', () => {
     renderer.create(<SubscriptionUser q={query} />);
     expect(client.executeSubscription).toBeCalledTimes(1);
   });
-
-  it('passes query to executeSubscription', () => {
-    renderer.create(<SubscriptionUser q={query} />);
-    expect(client.executeSubscription).toBeCalledWith(
-      {
-        key: expect.any(Number),
-        query: expect.any(Object),
-        variables: {},
-      },
-      expect.any(Object)
-    );
-  });
 });
 
 it('should support setting context in useSubscription params', () => {
@@ -96,25 +84,6 @@ describe('on subscription', () => {
   });
 });
 
-describe('on change', () => {
-  const qa = 'subscription NewSubA { exampleA }';
-  const qb = 'subscription NewSubB { exampleB }';
-
-  it('executes subscription', () => {
-    const wrapper = renderer.create(<SubscriptionUser q={query} />);
-
-    /**
-     * Have to call update twice for the change to be detected.
-     * Only a single change is detected (updating 5 times still only calls
-     * execute subscription twice).
-     */
-    wrapper.update(<SubscriptionUser q={qa} />);
-    wrapper.update(<SubscriptionUser q={qa} />);
-    wrapper.update(<SubscriptionUser q={qb} />);
-    expect(client.executeSubscription).toBeCalledTimes(2);
-  });
-});
-
 it('calls handler', () => {
   const handler = jest.fn();
   const wrapper = renderer.create(