Skip to content

Commit

Permalink
Only remove promise in query hook if the subscription was removed
Browse files Browse the repository at this point in the history
  • Loading branch information
markerikson committed Feb 20, 2023
1 parent 140ca1a commit 364ff51
Show file tree
Hide file tree
Showing 2 changed files with 99 additions and 4 deletions.
4 changes: 3 additions & 1 deletion packages/toolkit/src/query/react/buildHooks.ts
Original file line number Diff line number Diff line change
Expand Up @@ -740,7 +740,9 @@ export function buildHooks<Definitions extends EndpointDefinitions>({
})

usePossiblyImmediateEffect((): void | undefined => {
promiseRef.current = undefined
if (subscriptionRemoved) {
promiseRef.current = undefined
}
}, [subscriptionRemoved])

usePossiblyImmediateEffect((): void | undefined => {
Expand Down
99 changes: 96 additions & 3 deletions packages/toolkit/src/query/tests/buildHooks.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import util from 'util'
import * as React from 'react'
import type {
UseMutation,
Expand All @@ -10,7 +9,14 @@ import {
QueryStatus,
skipToken,
} from '@reduxjs/toolkit/query/react'
import { act, fireEvent, render, screen, waitFor } from '@testing-library/react'
import {
act,
fireEvent,
render,
screen,
waitFor,
renderHook,
} from '@testing-library/react'
import userEvent from '@testing-library/user-event'
import { rest } from 'msw'
import {
Expand All @@ -28,7 +34,6 @@ import type { AnyAction } from 'redux'
import type { SubscriptionOptions } from '@reduxjs/toolkit/dist/query/core/apiState'
import type { SerializedError } from '@reduxjs/toolkit'
import { createListenerMiddleware, configureStore } from '@reduxjs/toolkit'
import { renderHook } from '@testing-library/react'
import { delay } from '../../utils'

// Just setup a temporary in-memory counter for tests that `getIncrementedAmount`.
Expand Down Expand Up @@ -715,6 +720,94 @@ describe('hooks tests', () => {
expect(res.data!.amount).toBeGreaterThan(originalAmount)
})

// See https://github.com/reduxjs/redux-toolkit/issues/3182
test('Hook subscriptions are properly cleaned up when changing skip back and forth', async () => {
const pokemonApi = createApi({
baseQuery: fetchBaseQuery({ baseUrl: 'https://pokeapi.co/api/v2/' }),
endpoints: (builder) => ({
getPokemonByName: builder.query({
queryFn: (name: string) => ({ data: null }),
keepUnusedDataFor: 1,
}),
}),
})

const storeRef = setupApiStore(pokemonApi, undefined, {
withoutTestLifecycles: true,
})

const getSubscriptions = () => storeRef.store.getState().api.subscriptions

const checkNumSubscriptions = (arg: string, count: number) => {
const subscriptions = getSubscriptions()
const cacheKeyEntry = subscriptions[arg]

if (cacheKeyEntry) {
expect(Object.values(cacheKeyEntry).length).toBe(count)
}
}

// 1) Initial state: an active subscription
const { result, rerender, unmount } = renderHook(
([arg, options]: Parameters<
typeof pokemonApi.useGetPokemonByNameQuery
>) => pokemonApi.useGetPokemonByNameQuery(arg, options),
{
wrapper: storeRef.wrapper,
initialProps: ['a'],
}
)

await act(async () => {
await delay(1)
})

// 2) Set the current subscription to `{skip: true}
await act(async () => {
rerender(['a', { skip: true }])
})

// 3) Change _both_ the cache key _and_ `{skip: false}` at the same time.
// This causes the `subscriptionRemoved` check to be `true`.
await act(async () => {
rerender(['b'])
})

// There should only be one active subscription after changing the arg
checkNumSubscriptions('b', 1)

// 4) Re-render with the same arg.
// This causes the `subscriptionRemoved` check to be `false`.
// Correct behavior is this does _not_ clear the promise ref,
// so
await act(async () => {
rerender(['b'])
})

// There should only be one active subscription after changing the arg
checkNumSubscriptions('b', 1)

await act(async () => {
await delay(1)
})

unmount()

await act(async () => {
await delay(1)
})

// There should be no subscription entries left over after changing
// cache key args and swapping `skip` on and off
checkNumSubscriptions('b', 0)

const finalSubscriptions = getSubscriptions()

for (let cacheKeyEntry of Object.values(finalSubscriptions)) {
expect(Object.values(cacheKeyEntry!).length).toBe(0)
}
})

describe('Hook middleware requirements', () => {
let mock: jest.SpyInstance

Expand Down

0 comments on commit 364ff51

Please sign in to comment.