Bugfixes for useQuery (#243)

Bugfixes for useQuery
urql-graphql · Jun 3, 2019 · c8da383 · c8da383
2 parents 322c4fb + 0533ad2
commit c8da383
Show file tree

Hide file tree

Showing 6 changed files with 83 additions and 33 deletions.
diff --git a/README.md b/README.md
@@ -1,4 +1,3 @@
-
 # urql
 
 <div align="center">
@@ -25,7 +24,6 @@
 
 <img width="965" alt="Steve Urkel" src="https://user-images.githubusercontent.com/1457604/52959744-ee6cef80-338e-11e9-96fe-cf5231b8eab7.png">
 
-
 ## ✨ Features
 
 - 📦 **One package** to get a working GraphQL client in React

diff --git a/src/hooks/useMutation.ts b/src/hooks/useMutation.ts
@@ -1,5 +1,5 @@
 import { DocumentNode } from 'graphql';
-import { useContext, useState } from 'react';
+import { useContext, useEffect, useRef, useState } from 'react';
 import { pipe, toPromise } from 'wonka';
 import { Context } from '../context';
 import { OperationResult } from '../types';
@@ -19,13 +19,21 @@ export type UseMutationResponse<T, V> = [
 export const useMutation = <T = any, V = object>(
   query: DocumentNode | string
 ): UseMutationResponse<T, V> => {
+  const isMounted = useRef(true);
   const client = useContext(Context);
   const [state, setState] = useState<UseMutationState<T>>({
     fetching: false,
     error: undefined,
     data: undefined,
   });
 
+  useEffect(
+    () => () => {
+      isMounted.current = false;
+    },
+    []
+  );
+
   const executeMutation = (variables?: V) => {
     setState({ fetching: true, error: undefined, data: undefined });
 
@@ -36,7 +44,11 @@ export const useMutation = <T = any, V = object>(
       toPromise
     ).then(result => {
       const { data, error } = result;
-      setState({ fetching: false, data, error });
+
+      if (isMounted.current) {
+        setState({ fetching: false, data, error });
+      }
+
       return result;
     });
   };

diff --git a/src/hooks/useQuery.spec.ts b/src/hooks/useQuery.spec.ts
@@ -130,7 +130,6 @@ describe('useQuery', () => {
     `;
 
     rerender({ query: newQuery });
-    await waitForNextUpdate();
     expect(client.executeQuery).toBeCalledTimes(2);
     expect(client.executeQuery).toHaveBeenNthCalledWith(
       2,
@@ -161,7 +160,6 @@ describe('useQuery', () => {
     };
 
     rerender({ query: mockQuery, variables: newVariables });
-    await waitForNextUpdate();
     expect(client.executeQuery).toBeCalledTimes(2);
     expect(client.executeQuery).toHaveBeenNthCalledWith(
       2,
@@ -188,7 +186,6 @@ describe('useQuery', () => {
     expect(client.executeQuery).toBeCalledTimes(1);
 
     rerender({ query: mockQuery, variables: mockVariables });
-    await waitForNextUpdate();
     expect(client.executeQuery).toBeCalledTimes(1);
   });
 
@@ -224,7 +221,6 @@ describe('useQuery', () => {
       variables: mockVariables,
       requestPolicy: 'network-only',
     });
-    await waitForNextUpdate();
     expect(client.executeQuery).toBeCalledTimes(2);
     expect(client.executeQuery).toHaveBeenNthCalledWith(
       2,

diff --git a/src/hooks/useQuery.test.tsx b/src/hooks/useQuery.test.tsx
@@ -20,6 +20,7 @@ jest.mock('../client', () => {
 
 import React, { FC } from 'react';
 import renderer, { act } from 'react-test-renderer';
+import { pipe, onEnd, interval } from 'wonka';
 import { createClient } from '../client';
 import { OperationContext } from '../types';
 import { useQuery, UseQueryArgs, UseQueryState } from './useQuery';
@@ -162,6 +163,26 @@ describe('on change', () => {
   });
 });
 
+describe('on unmount', () => {
+  const unsubscribe = jest.fn();
+
+  beforeEach(() => {
+    client.executeQuery.mockReturnValueOnce(
+      pipe(
+        interval(400),
+        onEnd(unsubscribe)
+      )
+    );
+  });
+
+  it('unsubscribe is called', () => {
+    const wrapper = renderer.create(<QueryUser {...props} />);
+    wrapper.unmount();
+
+    expect(unsubscribe).toBeCalledTimes(1);
+  });
+});
+
 describe('execute query', () => {
   it('triggers query execution', () => {
     renderer.create(<QueryUser {...props} />);

diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts
@@ -33,7 +33,8 @@ export type UseQueryResponse<T> = [
 export const useQuery = <T = any, V = object>(
   args: UseQueryArgs<V>
 ): UseQueryResponse<T> => {
-  const unsubscribe = useRef(noop);
+  const isMounted = useRef(true);
+  const unsubscribe = useRef<() => void>(noop);
 
   const client = useContext(Context);
   const request = useMemo(
@@ -47,36 +48,46 @@ export const useQuery = <T = any, V = object>(
     data: undefined,
   });
 
+  /** Unmount handler */
+  useEffect(
+    () => () => {
+      isMounted.current = false;
+    },
+    []
+  );
+
   const executeQuery = useCallback(
     (opts?: Partial<OperationContext>) => {
       unsubscribe.current();
 
-      setState(s => ({ ...s, fetching: true }));
+      if (args.pause) {
+        unsubscribe.current = noop;
+        return;
+      }
 
-      let teardown = noop;
+      setState(s => ({ ...s, fetching: true }));
 
-      if (!args.pause) {
-        [teardown] = pipe(
-          client.executeQuery(request, {
-            requestPolicy: args.requestPolicy,
-            ...opts,
-          }),
-          subscribe(({ data, error }) => {
-            setState({ fetching: false, data, error });
-          })
-        );
-      } else {
-        setState(s => ({ ...s, fetching: false }));
-      }
+      const [teardown] = pipe(
+        client.executeQuery(request, {
+          requestPolicy: args.requestPolicy,
+          ...opts,
+        }),
+        subscribe(
+          ({ data, error }) =>
+            isMounted.current && setState({ fetching: false, data, error })
+        )
+      );
 
       unsubscribe.current = teardown;
     },
     // eslint-disable-next-line react-hooks/exhaustive-deps
     [request.key, client, args.pause, args.requestPolicy]
   );
 
+  /** Trigger query on arg change. */
   useEffect(() => {
     executeQuery();
+
     return unsubscribe.current;
   }, [executeQuery]);
 

diff --git a/src/hooks/useSubscription.ts b/src/hooks/useSubscription.ts
@@ -3,8 +3,8 @@ import {
   useCallback,
   useContext,
   useEffect,
-  useState,
   useRef,
+  useState,
   useMemo,
 } from 'react';
 import { pipe, subscribe } from 'wonka';
@@ -29,6 +29,7 @@ export const useSubscription = <T = any, R = T, V = object>(
   args: UseSubscriptionArgs<V>,
   handler?: SubscriptionHandler<T, R>
 ): UseSubscriptionResponse<R> => {
+  const isMounted = useRef(true);
   const unsubscribe = useRef(noop);
 
   const client = useContext(Context);
@@ -42,25 +43,36 @@ export const useSubscription = <T = any, R = T, V = object>(
     data: undefined,
   });
 
+  /** Unmount handler */
+  useEffect(
+    () => () => {
+      isMounted.current = false;
+    },
+    []
+  );
+
   const executeSubscription = useCallback(() => {
     unsubscribe.current();
 
     const [teardown] = pipe(
       client.executeSubscription(request),
-      subscribe(({ data, error }) => {
-        setState(s => ({
-          data: handler !== undefined ? handler(s.data, data) : data,
-          error,
-        }));
-      })
+      subscribe(
+        ({ data, error }) =>
+          isMounted.current &&
+          setState(s => ({
+            data: handler !== undefined ? handler(s.data, data) : data,
+            error,
+          }))
+      )
     );
 
     unsubscribe.current = teardown;
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [request.key, handler, client]);
+  }, [client, handler, request]);
 
+  /** Trigger subscription on query change. */
   useEffect(() => {
     executeSubscription();
+
     return unsubscribe.current;
   }, [executeSubscription]);