From d887ae764b68594756959ca0ddab2ae2980f708a Mon Sep 17 00:00:00 2001 From: Joe Warren Date: Wed, 6 Mar 2019 10:17:24 +0000 Subject: [PATCH 1/3] Don't update state after a request responds if the component has unmounted. --- .../graphql-hooks/src/useClientRequest.js | 25 ++++++++++++++----- .../test/unit/useClientRequest.test.js | 24 ++++++++++++++++++ 2 files changed, 43 insertions(+), 6 deletions(-) diff --git a/packages/graphql-hooks/src/useClientRequest.js b/packages/graphql-hooks/src/useClientRequest.js index 71120dca..fcf7eb9d 100644 --- a/packages/graphql-hooks/src/useClientRequest.js +++ b/packages/graphql-hooks/src/useClientRequest.js @@ -52,6 +52,7 @@ function reducer(state, action) { */ function useClientRequest(query, initialOpts = {}) { const client = React.useContext(ClientContext) + const isMounted = React.useRef(true) const operation = { query, variables: initialOpts.variables, @@ -73,12 +74,22 @@ function useClientRequest(query, initialOpts = {}) { // if so the state would be invalid, this effect ensures we reset it back const stringifiedCacheKey = JSON.stringify(cacheKey) React.useEffect(() => { - if (initialOpts.updateData) return // if using updateData we can assume that the consumer cares about the previous data - dispatch({ type: actionTypes.RESET_STATE, initialState }) + if (!initialOpts.updateData) { + // if using updateData we can assume that the consumer cares about the previous data + dispatch({ type: actionTypes.RESET_STATE, initialState }) + } }, [stringifiedCacheKey]) // eslint-disable-line react-hooks/exhaustive-deps + React.useEffect(() => { + isMounted.current = true + return () => { + isMounted.current = false + } + }, []) + // arguments to fetchData override the useClientRequest arguments function fetchData(newOpts) { + if (!isMounted.current) return Promise.resolve() const revisedOpts = { ...initialOpts, ...newOpts @@ -118,10 +129,12 @@ function useClientRequest(query, initialOpts = {}) { client.cache.set(revisedCacheKey, result) } - dispatch({ - type: actionTypes.REQUEST_RESULT, - result - }) + if (isMounted.current) { + dispatch({ + type: actionTypes.REQUEST_RESULT, + result + }) + } return result }) diff --git a/packages/graphql-hooks/test/unit/useClientRequest.test.js b/packages/graphql-hooks/test/unit/useClientRequest.test.js index 8b0e9570..665edd3c 100644 --- a/packages/graphql-hooks/test/unit/useClientRequest.test.js +++ b/packages/graphql-hooks/test/unit/useClientRequest.test.js @@ -205,6 +205,30 @@ describe('useClientRequest', () => { expect(state).toEqual({ cacheHit: false, loading: false, data: 'data' }) }) + it('calls request with options & doesnt update state if component unmounts whilst request is in flight', async () => { + let fetchData, state; + const { unmount } = renderHook( + () => + ([fetchData, state] = useClientRequest(TEST_QUERY, { + variables: { limit: 2 }, + operationName: 'test' + })), + { + wrapper: Wrapper + } + ); + + const fetchDataPromise = fetchData(); + unmount(); + await fetchDataPromise; + + expect(mockClient.request).toHaveBeenCalledWith( + { operationName: 'test', variables: { limit: 2 }, query: TEST_QUERY }, + { operationName: 'test', variables: { limit: 2 } } + ); + expect(state).toEqual({ cacheHit: false, loading: true }); + }); + it('calls request with revised options', async () => { let fetchData renderHook( From 0c6998baef17915818ec7c3304fd804d08edd647 Mon Sep 17 00:00:00 2001 From: Joe Warren Date: Wed, 6 Mar 2019 10:28:47 +0000 Subject: [PATCH 2/3] Semi colons --- .../test/unit/useClientRequest.test.js | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/packages/graphql-hooks/test/unit/useClientRequest.test.js b/packages/graphql-hooks/test/unit/useClientRequest.test.js index 665edd3c..a89e4fdc 100644 --- a/packages/graphql-hooks/test/unit/useClientRequest.test.js +++ b/packages/graphql-hooks/test/unit/useClientRequest.test.js @@ -206,7 +206,7 @@ describe('useClientRequest', () => { }) it('calls request with options & doesnt update state if component unmounts whilst request is in flight', async () => { - let fetchData, state; + let fetchData, state const { unmount } = renderHook( () => ([fetchData, state] = useClientRequest(TEST_QUERY, { @@ -216,18 +216,18 @@ describe('useClientRequest', () => { { wrapper: Wrapper } - ); + ) - const fetchDataPromise = fetchData(); - unmount(); - await fetchDataPromise; + const fetchDataPromise = fetchData() + unmount() + await fetchDataPromise expect(mockClient.request).toHaveBeenCalledWith( { operationName: 'test', variables: { limit: 2 }, query: TEST_QUERY }, { operationName: 'test', variables: { limit: 2 } } - ); - expect(state).toEqual({ cacheHit: false, loading: true }); - }); + ) + expect(state).toEqual({ cacheHit: false, loading: true }) + }) it('calls request with revised options', async () => { let fetchData From 8d5b440735244e73d32f029301bfad687271361c Mon Sep 17 00:00:00 2001 From: Joe Warren Date: Wed, 6 Mar 2019 13:00:24 +0000 Subject: [PATCH 3/3] Add test to ensure request is not sent if the component is unmounted --- .../test/unit/useClientRequest.test.js | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/packages/graphql-hooks/test/unit/useClientRequest.test.js b/packages/graphql-hooks/test/unit/useClientRequest.test.js index a89e4fdc..4a349487 100644 --- a/packages/graphql-hooks/test/unit/useClientRequest.test.js +++ b/packages/graphql-hooks/test/unit/useClientRequest.test.js @@ -229,6 +229,26 @@ describe('useClientRequest', () => { expect(state).toEqual({ cacheHit: false, loading: true }) }) + it('returns undefined instantly if not mounted', async () => { + let fetchData, state + const { unmount } = renderHook( + () => + ([fetchData, state] = useClientRequest(TEST_QUERY, { + variables: { limit: 2 }, + operationName: 'test' + })), + { + wrapper: Wrapper + } + ) + + unmount() + const result = await fetchData() + expect(result).toBe(undefined) + expect(mockClient.request).not.toHaveBeenCalled() + expect(state).toEqual({ cacheHit: false, loading: true }) + }) + it('calls request with revised options', async () => { let fetchData renderHook(