From d3ca8ff18bd5e247d80190bcb82bde8b35204206 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Thu, 30 May 2019 09:26:56 +0100 Subject: [PATCH 1/7] Bugfixes for useQuery --- src/hooks/useQuery.ts | 32 ++++++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 8 deletions(-) diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts index 3ebabcb28a..ea0daf605f 100644 --- a/src/hooks/useQuery.ts +++ b/src/hooks/useQuery.ts @@ -1,5 +1,12 @@ import { DocumentNode } from 'graphql'; -import { useCallback, useContext, useEffect, useState } from 'react'; +import { + useCallback, + useContext, + useEffect, + useMemo, + useRef, + useState, +} from 'react'; import { pipe, subscribe } from 'wonka'; import { Context } from '../context'; import { OperationContext, RequestPolicy } from '../types'; @@ -25,10 +32,14 @@ type UseQueryResponse = [ export const useQuery = ( args: UseQueryArgs ): UseQueryResponse => { - let unsubscribe = noop; + const isMounted = useRef(true); + const unsubscribe = useRef<() => void>(noop); const client = useContext(Context); - const request = createRequest(args.query, args.variables as any); + const request = useMemo( + () => createRequest(args.query, args.variables as any), + [args] + ); const [state, setState] = useState>({ fetching: false, @@ -38,7 +49,7 @@ export const useQuery = ( const executeQuery = useCallback( (opts?: Partial) => { - unsubscribe(); + unsubscribe.current(); setState(s => ({ ...s, fetching: true })); const [teardown] = pipe( @@ -46,19 +57,24 @@ export const useQuery = ( requestPolicy: args.requestPolicy, ...opts, }), - subscribe(({ data, error }) => - setState({ fetching: false, data, error }) + subscribe( + ({ data, error }) => + isMounted.current && setState({ fetching: false, data, error }) ) ); - unsubscribe = teardown; + unsubscribe.current = teardown; }, [request.key] ); useEffect(() => { executeQuery(); - return unsubscribe; + + return () => { + isMounted.current = false; + unsubscribe.current(); + }; }, [request.key]); return [state, executeQuery]; From 5ce4090a4a3e8963a481384178aa591e54671d44 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Thu, 30 May 2019 09:36:21 +0100 Subject: [PATCH 2/7] Apply fixes to useMutation and useSubscription --- src/hooks/useMutation.ts | 16 ++++++++++++++-- src/hooks/useSubscription.ts | 28 +++++++++++++++++----------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/src/hooks/useMutation.ts b/src/hooks/useMutation.ts index 3c7e707807..b5311f1baa 100644 --- 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,6 +19,7 @@ type UseMutationResponse = [ export const useMutation = ( query: DocumentNode | string ): UseMutationResponse => { + const isMounted = useRef(true); const client = useContext(Context); const [state, setState] = useState>({ fetching: false, @@ -26,6 +27,13 @@ export const useMutation = ( data: undefined, }); + useEffect( + () => () => { + isMounted.current = false; + }, + [] + ); + const executeMutation = (variables?: V) => { setState({ fetching: true, error: undefined, data: undefined }); @@ -36,7 +44,11 @@ export const useMutation = ( 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/useSubscription.ts b/src/hooks/useSubscription.ts index deb4825b46..0c5fc6136c 100644 --- a/src/hooks/useSubscription.ts +++ b/src/hooks/useSubscription.ts @@ -1,5 +1,5 @@ import { DocumentNode } from 'graphql'; -import { useCallback, useContext, useEffect, useState } from 'react'; +import { useCallback, useContext, useEffect, useRef, useState } from 'react'; import { pipe, subscribe } from 'wonka'; import { Context } from '../context'; import { CombinedError, createRequest, noop } from '../utils'; @@ -22,7 +22,8 @@ export const useSubscription = ( args: UseSubscriptionArgs, handler?: SubscriptionHandler ): UseSubscriptionResponse => { - let unsubscribe = noop; + const isMounted = useRef(true); + const unsubscribe = useRef(noop); const client = useContext(Context); const request = createRequest(args.query, args.variables as any); @@ -33,24 +34,29 @@ export const useSubscription = ( }); const executeSubscription = useCallback(() => { - unsubscribe(); + 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 = teardown; + unsubscribe.current = teardown; }, [request.key]); useEffect(() => { executeSubscription(); - return unsubscribe; + return () => { + unsubscribe.current(); + isMounted.current = false; + }; }, [request.key]); return [state]; From 8c26b2d01701480b87d98c079961eb6e179629b0 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Sat, 1 Jun 2019 14:53:02 +0100 Subject: [PATCH 3/7] Don't refetch on query change --- src/hooks/useQuery.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts index b91d697bc8..e761f75092 100644 --- a/src/hooks/useQuery.ts +++ b/src/hooks/useQuery.ts @@ -83,7 +83,7 @@ export const useQuery = ( isMounted.current = false; unsubscribe.current(); }; - }, [executeQuery, request.key]); + }, [executeQuery]); return [state, executeQuery]; }; From 1a6d38a00e49a54ae54460aca32c438a114aa7d7 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Sat, 1 Jun 2019 15:27:59 +0100 Subject: [PATCH 4/7] Fix tests waiting for update --- src/hooks/useQuery.spec.ts | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/hooks/useQuery.spec.ts b/src/hooks/useQuery.spec.ts index a5fa1e4e83..e0db51e4b4 100644 --- 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, From a57a5574e70404852df90884c9caae77311d31e1 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Sat, 1 Jun 2019 15:42:41 +0100 Subject: [PATCH 5/7] Add test for unsubscribe --- src/hooks/useQuery.test.tsx | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/src/hooks/useQuery.test.tsx b/src/hooks/useQuery.test.tsx index d83a09b4ca..7fe075dd51 100644 --- 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(); + wrapper.unmount(); + + expect(unsubscribe).toBeCalledTimes(1); + }); +}); + describe('execute query', () => { it('triggers query execution', () => { renderer.create(); From 8f65db35312efd8597104aaaeaed113fd9fce804 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Sat, 1 Jun 2019 15:44:09 +0100 Subject: [PATCH 6/7] Remove unneeded dependency --- src/hooks/useSubscription.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hooks/useSubscription.ts b/src/hooks/useSubscription.ts index 287b2dbc2a..fc89b916f9 100644 --- a/src/hooks/useSubscription.ts +++ b/src/hooks/useSubscription.ts @@ -63,11 +63,12 @@ export const useSubscription = ( useEffect(() => { executeSubscription(); + return () => { unsubscribe.current(); isMounted.current = false; }; - }, [executeSubscription, request.key]); + }, [executeSubscription]); return [state]; }; From 0533ad2b5ecbf200ab478d57c81ff780f45d8591 Mon Sep 17 00:00:00 2001 From: Andy Richardson Date: Sat, 1 Jun 2019 16:02:21 +0100 Subject: [PATCH 7/7] Isolate unmount handler --- src/hooks/useQuery.ts | 14 ++++++++++---- src/hooks/useSubscription.ts | 14 ++++++++++---- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/hooks/useQuery.ts b/src/hooks/useQuery.ts index e761f75092..2cb1c10ce8 100644 --- a/src/hooks/useQuery.ts +++ b/src/hooks/useQuery.ts @@ -48,6 +48,14 @@ export const useQuery = ( data: undefined, }); + /** Unmount handler */ + useEffect( + () => () => { + isMounted.current = false; + }, + [] + ); + const executeQuery = useCallback( (opts?: Partial) => { unsubscribe.current(); @@ -76,13 +84,11 @@ export const useQuery = ( [request.key, client, args.pause, args.requestPolicy] ); + /** Trigger query on arg change. */ useEffect(() => { executeQuery(); - return () => { - isMounted.current = false; - unsubscribe.current(); - }; + return unsubscribe.current; }, [executeQuery]); return [state, executeQuery]; diff --git a/src/hooks/useSubscription.ts b/src/hooks/useSubscription.ts index fc89b916f9..d12f1a303f 100644 --- a/src/hooks/useSubscription.ts +++ b/src/hooks/useSubscription.ts @@ -43,6 +43,14 @@ export const useSubscription = ( data: undefined, }); + /** Unmount handler */ + useEffect( + () => () => { + isMounted.current = false; + }, + [] + ); + const executeSubscription = useCallback(() => { unsubscribe.current(); @@ -61,13 +69,11 @@ export const useSubscription = ( unsubscribe.current = teardown; }, [client, handler, request]); + /** Trigger subscription on query change. */ useEffect(() => { executeSubscription(); - return () => { - unsubscribe.current(); - isMounted.current = false; - }; + return unsubscribe.current; }, [executeSubscription]); return [state];