Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
25 changes: 19 additions & 6 deletions packages/graphql-hooks/src/useClientRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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
Expand Down Expand Up @@ -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
})
Expand Down
24 changes: 24 additions & 0 deletions packages/graphql-hooks/test/unit/useClientRequest.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor

Choose a reason for hiding this comment

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

nice

Copy link
Contributor

Choose a reason for hiding this comment

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

In this test it should resolve the result, right? We can then add a test that after unmount it resolves undefined:

const fetchDataPromise = fetchData()
unmount()
const firstResult = await fetchDataPromise // <- expected actual result
const secondResult = await fetchData() // <- expected undefined

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added a test for that 👍


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(
Expand Down