Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bugfixes for useQuery #243

Merged
merged 8 commits into from
Jun 3, 2019
+83 −33
Merged

Bugfixes for useQuery #243

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d3ca8ff
Bugfixes for useQuery
andyrichardson May 30, 2019
5ce4090
Apply fixes to useMutation and useSubscription
andyrichardson May 30, 2019
3d358f2
Merge branch 'master' of github.com:FormidableLabs/urql into fix-stal…
andyrichardson May 30, 2019
8c26b2d
Don't refetch on query change
andyrichardson Jun 1, 2019
1a6d38a
Fix tests waiting for update
andyrichardson Jun 1, 2019
a57a557
Add test for unsubscribe
andyrichardson Jun 1, 2019
8f65db3
Remove unneeded dependency
andyrichardson Jun 1, 2019
0533ad2
Isolate unmount handler
andyrichardson Jun 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 0 additions & 2 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@

# urql

<div align="center">
Expand All @@ -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
Expand Down
16 changes: 14 additions & 2 deletions src/hooks/useMutation.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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';
Expand All @@ -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 });

Expand All @@ -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;
});
};
Expand Down
4 changes: 0 additions & 4 deletions src/hooks/useQuery.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,6 @@ describe('useQuery', () => {
`;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Think I saw a comment in an email from you @andyrichardson, but yep, to explain the naming behind this file, I basically wanted a way to differentiate it from useQuery.test.tsx. I'd be happy to use a different name. Part of the reason for bringing in react-hooks-testing-library was to make us less dependent on enzyme, to avoid re-assignment of let bindings in tests, and to be able to get the direct values returned by our hooks so we can assert on them (which I think has some value). Perhaps we could specify these two files as useQuery.enzyme.test.tsx and useQuery.rhtl.test.tsx? I'm also fine with removing them if we don't think they have value or we don't like the API, I just find the it a bit easier to work w/ than enzyme.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cheers @parkerziegler - I initially asked but then found your PR with the explanation. No need to make changes 👍

I'm pretty sure react-hooks-testing-library is using react-test-renderer under the hood (we're not using enzyme). I agree the reassignment + added complexity is a pain 😑


rerender({ query: newQuery });
await waitForNextUpdate();
expect(client.executeQuery).toBeCalledTimes(2);
expect(client.executeQuery).toHaveBeenNthCalledWith(
2,
Expand Down Expand Up @@ -161,7 +160,6 @@ describe('useQuery', () => {
};

rerender({ query: mockQuery, variables: newVariables });
await waitForNextUpdate();
expect(client.executeQuery).toBeCalledTimes(2);
expect(client.executeQuery).toHaveBeenNthCalledWith(
2,
Expand All @@ -188,7 +186,6 @@ describe('useQuery', () => {
expect(client.executeQuery).toBeCalledTimes(1);

rerender({ query: mockQuery, variables: mockVariables });
await waitForNextUpdate();
expect(client.executeQuery).toBeCalledTimes(1);
});

Expand Down Expand Up @@ -224,7 +221,6 @@ describe('useQuery', () => {
variables: mockVariables,
requestPolicy: 'network-only',
});
await waitForNextUpdate();
expect(client.executeQuery).toBeCalledTimes(2);
expect(client.executeQuery).toHaveBeenNthCalledWith(
2,
Expand Down
21 changes: 21 additions & 0 deletions src/hooks/useQuery.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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';
Expand Down Expand Up @@ -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} />);
Expand Down
43 changes: 27 additions & 16 deletions src/hooks/useQuery.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the early return here is the reason for the test failures. Basically, waitForNextUpdate expects some asynchronous thing to happen (i.e. a Promise to be resolved when running the useEffect), otherwise it'll never get a signal that the effect completed. You should be able to switch the failing tests to sync tests (remove async keyword and await waitForNextUpdate) and I'd expect them to pass.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Parker, I'll investigate tonight 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking a little more closely, it appears that the failed tests are related to the effect not re-running when new variables, query, and requestPolicy are received 🤔 Not totally sure why that would be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@parkerziegler I've managed to fix the tests by removing the waitForUpdate call. Given tests themselves are checking that a re-fetch is triggered we can get away without waiting for state updates.

As for why those state updates aren't taking place in a test environment, I suspect it's related to the mock implementation of executeQuery.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, will dig into this more tomorrow to confirm. But agree, as long as we're sure that executeQuery is being called again when a new query, variables, or requestPolicy are received, then I'm not too worried about the state updates.

}

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]);

Expand Down
30 changes: 21 additions & 9 deletions src/hooks/useSubscription.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@ import {
useCallback,
useContext,
useEffect,
useState,
useRef,
useState,
useMemo,
} from 'react';
import { pipe, subscribe } from 'wonka';
Expand All @@ -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);
Expand All @@ -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]);

Expand Down