Repository: FormidableLabs/urql
Pull request #237 (2019-05-11 16:54:54)

Add support for skip to Query and useQuery.
This PR picks up on the changes in #229 and would close issue #228. @kitten I read through your comments in #229 and attempted to address them in this PR. I also added a few tests to ensure things are behaving as expected, but definitely would appreciate both of your 👀 in verifying that this behaves as expected.

parkerziegler: @kitten @andyrichardson refactored this just now per @kitten's suggestions, let me know what the next steps you think we should take are 😄 Thanks for the review thus far 💯  
parkerziegler: @kitten @andyrichardson how do you feel about merging this? I have some follow up changes that 1) integrate `typescript-eslint` _just_ for the React hooks rules, 2) fix some dependency array issues flagged by the ESLint plugin, and 3) show what a test suite for `useQuery` would look like with [`react-hooks-testing-library`](https://github.com/mpeyper/react-hooks-testing-library). Would love to get those changes up for review soon!

Patch:
diff --git a/docs/api.md b/docs/api.md
index cf4bd3c4..e86a16c5 100644
--- a/docs/api.md
+++ b/docs/api.md
@@ -11,6 +11,7 @@ interface UseQueryArgs {
   query: string;
   variables?: any;
   requestPolicy?: RequestPolicy;
+  skip?: boolean;
 }
 ```
 
@@ -100,6 +101,7 @@ interface UseSubscriptionState<T> {
 | query         | `string`                   | The GraphQL request's query                                                                           |
 | variables     | `object`                   | The GraphQL request's variables                                                                       |
 | requestPolicy | `?RequestPolicy`           | An optional request policy that should be used                                                        |
+| skip          | `?boolean`                 | A boolean flag instructing `Query` to skip executing the subsequent query operation                   |
 | children      | `RenderProps => ReactNode` | A function that follows the typical render props pattern. The shape of the render props is as follows |
 
 #### Render Props
diff --git a/src/components/Query.tsx b/src/components/Query.tsx
index 0f5e0a85..66c3dc75 100644
--- a/src/components/Query.tsx
+++ b/src/components/Query.tsx
@@ -11,6 +11,7 @@ interface QueryHandlerProps {
   variables?: object;
   client: Client;
   requestPolicy?: RequestPolicy;
+  skip?: boolean;
   children: (arg: QueryHandlerState) => ReactNode;
 }
 
@@ -30,19 +31,27 @@ class QueryHandler extends Component<QueryHandlerProps, QueryHandlerState> {
 
     this.setState({ fetching: true });
 
-    const [teardown] = pipe(
-      this.props.client.executeQuery(this.request, {
-        requestPolicy: this.props.requestPolicy,
-        ...opts,
-      }),
-      subscribe(({ data, error }) => {
-        this.setState({
-          fetching: false,
-          data,
-          error,
-        });
-      })
-    );
+    let teardown = noop;
+
+    if (!this.props.skip) {
+      [teardown] = pipe(
+        this.props.client.executeQuery(this.request, {
+          requestPolicy: this.props.requestPolicy,
+          ...opts,
+        }),
+        subscribe(({ data, error }) => {
+          this.setState({
+            fetching: false,
+            data,
+            error,
+          });
+        })
+      );
+    } else {
+      this.setState({
+        fetching: false,
+      });
+    }
 
     this.unsubscribe = teardown;
   };
diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts
index 51b9d717..17936d40 100644
--- a/src/hooks/useQuery.ts
+++ b/src/hooks/useQuery.ts
@@ -9,9 +9,10 @@ export interface UseQueryArgs<V> {
   query: string | DocumentNode;
   variables?: V;
   requestPolicy?: RequestPolicy;
+  skip?: boolean;
 }
 
-interface UseQueryState<T> {
+export interface UseQueryState<T> {
   fetching: boolean;
   data?: T;
   error?: CombinedError;
@@ -39,27 +40,34 @@ export const useQuery = <T = any, V = object>(
   const executeQuery = useCallback(
     (opts?: Partial<OperationContext>) => {
       unsubscribe();
+
       setState(s => ({ ...s, fetching: true }));
 
-      const [teardown] = pipe(
-        client.executeQuery(request, {
-          requestPolicy: args.requestPolicy,
-          ...opts,
-        }),
-        subscribe(({ data, error }) =>
-          setState({ fetching: false, data, error })
-        )
-      );
+      let teardown = noop;
+
+      if (!args.skip) {
+        [teardown] = pipe(
+          client.executeQuery(request, {
+            requestPolicy: args.requestPolicy,
+            ...opts,
+          }),
+          subscribe(({ data, error }) =>
+            setState({ fetching: false, data, error })
+          )
+        );
+      } else {
+        setState(s => ({ ...s, fetching: false }));
+      }
 
       unsubscribe = teardown;
     },
-    [request.key]
+    [request.key, args.skip, args.requestPolicy]
   );
 
   useEffect(() => {
     executeQuery();
     return unsubscribe;
-  }, [request.key]);
+  }, [request.key, args.skip]);
 
   return [state, executeQuery];
 };


Test patch:
diff --git a/src/components/Query.test.tsx b/src/components/Query.test.tsx
index 1cf6bf41..05c85afe 100644
--- a/src/components/Query.test.tsx
+++ b/src/components/Query.test.tsx
@@ -110,3 +110,23 @@ describe('on error', () => {
     expect(childProps).toHaveProperty('error', error);
   });
 });
+
+describe('skip', () => {
+  beforeEach(() => {
+    client.executeQuery.mockReturnValue(fromValue({ data: 1234 }));
+  });
+
+  it('should skip executing the query if skip is true', () => {
+    mountWrapper({ ...props, skip: true });
+    expect(client.executeQuery).not.toHaveBeenCalled();
+  });
+
+  it('should not call executeQuery if skip changes to true', () => {
+    const wrapper = mountWrapper(props);
+    expect(client.executeQuery).toHaveBeenCalledTimes(1);
+
+    // @ts-ignore
+    wrapper.setProps({ ...props, query: '{ newQuery }', skip: true });
+    expect(client.executeQuery).toHaveBeenCalledTimes(1);
+  });
+});
diff --git a/src/hooks/useQuery.test.tsx b/src/hooks/useQuery.test.tsx
index e78f39fc..e2bedc5a 100644
--- a/src/hooks/useQuery.test.tsx
+++ b/src/hooks/useQuery.test.tsx
@@ -19,30 +19,36 @@ jest.mock('../client', () => {
 
 import React, { FC } from 'react';
 import renderer, { act } from 'react-test-renderer';
-// @ts-ignore - data is imported from mock only
 import { createClient } from '../client';
-import { useQuery } from './useQuery';
+import { OperationContext } from '../types';
+import { useQuery, UseQueryArgs, UseQueryState } from './useQuery';
 
 // @ts-ignore
 const client = createClient() as { executeQuery: jest.Mock };
-const props = {
+const props: UseQueryArgs<{ myVar: number }> = {
   query: '{ example }',
   variables: {
     myVar: 1234,
   },
+  skip: false,
 };
-let state: any;
-let execute: any;
 
-const QueryUser: FC<typeof props> = ({ query, variables }) => {
-  const [s, e] = useQuery({ query, variables });
+let state: UseQueryState<any> | undefined;
+let execute: ((opts?: Partial<OperationContext>) => void) | undefined;
+
+const QueryUser: FC<UseQueryArgs<{ myVar: number }>> = ({
+  query,
+  variables,
+  skip,
+}) => {
+  const [s, e] = useQuery({ query, variables, skip });
   state = s;
   execute = e;
   return <p>{s.data}</p>;
 };
 
 beforeAll(() => {
-  // tslint:disable-next-line
+  // tslint:disable-next-line no-console
   console.log(
     'supressing console.error output due to react-test-renderer spam (hooks related)'
   );
@@ -158,7 +164,25 @@ describe('on change', () => {
 describe('execute query', () => {
   it('triggers query execution', () => {
     renderer.create(<QueryUser {...props} />);
-    act(() => execute());
+    act(() => execute!());
     expect(client.executeQuery).toBeCalledTimes(2);
   });
 });
+
+describe('skip', () => {
+  it('skips executing the query if skip is true', () => {
+    renderer.create(<QueryUser {...props} skip={true} />);
+    expect(client.executeQuery).not.toBeCalled();
+  });
+
+  it('skips executing queries if skip updates to true', () => {
+    const wrapper = renderer.create(<QueryUser {...props} />);
+
+    /**
+     * Call update twice for the change to be detected.
+     */
+    wrapper.update(<QueryUser {...props} skip={true} />);
+    wrapper.update(<QueryUser {...props} skip={true} />);
+    expect(client.executeQuery).toBeCalledTimes(1);
+  });
+});