diff --git a/packages/toolkit/src/query/react/buildHooks.ts b/packages/toolkit/src/query/react/buildHooks.ts index b6822a7478..9d89188514 100644 --- a/packages/toolkit/src/query/react/buildHooks.ts +++ b/packages/toolkit/src/query/react/buildHooks.ts @@ -626,9 +626,7 @@ export function buildHooks({ ) lastResult = undefined } - if (queryArgs === skipToken) { - lastResult = undefined - } + // data is the last known good request result we have tracked - or if none has been tracked yet the last good result for the current args let data = currentState.isSuccess ? currentState.data : lastResult?.data if (data === undefined) data = currentState.data @@ -742,7 +740,9 @@ export function buildHooks({ }) usePossiblyImmediateEffect((): void | undefined => { - promiseRef.current = undefined + if (subscriptionRemoved) { + promiseRef.current = undefined + } }, [subscriptionRemoved]) usePossiblyImmediateEffect((): void | undefined => { diff --git a/packages/toolkit/src/query/tests/buildHooks.test.tsx b/packages/toolkit/src/query/tests/buildHooks.test.tsx index 15998503d0..2ddc91537b 100644 --- a/packages/toolkit/src/query/tests/buildHooks.test.tsx +++ b/packages/toolkit/src/query/tests/buildHooks.test.tsx @@ -9,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 { @@ -27,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`. @@ -714,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 @@ -2472,7 +2566,11 @@ describe('skip behaviour', () => { await act(async () => { rerender([1, { skip: true }]) }) - expect(result.current).toEqual(uninitialized) + expect(result.current).toEqual({ + ...uninitialized, + currentData: undefined, + data: { name: 'Timmy' }, + }) await delay(1) expect(subscriptionCount('getUser(1)')).toBe(0) }) @@ -2489,6 +2587,7 @@ describe('skip behaviour', () => { expect(result.current).toEqual(uninitialized) await delay(1) + expect(subscriptionCount('getUser(1)')).toBe(0) // also no subscription on `getUser(skipToken)` or similar: expect(storeRef.store.getState().api.subscriptions).toEqual({}) @@ -2504,10 +2603,51 @@ describe('skip behaviour', () => { await act(async () => { rerender([skipToken]) }) - expect(result.current).toEqual(uninitialized) + expect(result.current).toEqual({ + ...uninitialized, + currentData: undefined, + data: { name: 'Timmy' }, + }) await delay(1) expect(subscriptionCount('getUser(1)')).toBe(0) }) + + test('skipping a previously fetched query retains the existing value as `data`, but clears `currentData`', async () => { + const { result, rerender } = renderHook( + ([arg, options]: Parameters) => + api.endpoints.getUser.useQuery(arg, options), + { + wrapper: storeRef.wrapper, + initialProps: [1], + } + ) + + await act(async () => { + await delay(1) + }) + + // Normal fulfilled result, with both `data` and `currentData` + expect(result.current).toMatchObject({ + status: QueryStatus.fulfilled, + isSuccess: true, + data: { name: 'Timmy' }, + currentData: { name: 'Timmy' }, + }) + + await act(async () => { + rerender([1, { skip: true }]) + await delay(1) + }) + + // After skipping, the query is "uninitialized", but still retains the last fetched `data` + // even though it's skipped. `currentData` is undefined, since that matches the current arg. + expect(result.current).toMatchObject({ + status: QueryStatus.uninitialized, + isSuccess: false, + data: { name: 'Timmy' }, + currentData: undefined, + }) + }) }) // type tests: