diff --git a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts new file mode 100644 index 0000000000..a002f68cb3 --- /dev/null +++ b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts @@ -0,0 +1,57 @@ +import type { QueryThunk, RejectedAction } from '../buildThunks' +import type { SubMiddlewareBuilder } from './types' + +// Copied from https://github.com/feross/queue-microtask +let promise: Promise +const queueMicrotaskShim = + typeof queueMicrotask === 'function' + ? queueMicrotask.bind(typeof window !== 'undefined' ? window : global) + : // reuse resolved promise, and allocate it lazily + (cb: () => void) => + (promise || (promise = Promise.resolve())).then(cb).catch((err: any) => + setTimeout(() => { + throw err + }, 0) + ) + +export const build: SubMiddlewareBuilder = ({ + api, + context: { apiUid }, + queryThunk, + reducerPath, +}) => { + return (mwApi) => { + let abortedQueryActionsQueue: RejectedAction[] = [] + let dispatchQueued = false + + return (next) => (action) => { + if (queryThunk.rejected.match(action)) { + const { condition, arg } = action.meta + + if (condition && arg.subscribe) { + // request was aborted due to condition (another query already running) + // _Don't_ dispatch right away - queue it for a debounced grouped dispatch + abortedQueryActionsQueue.push(action) + + if (!dispatchQueued) { + queueMicrotaskShim(() => { + mwApi.dispatch( + api.internalActions.subscriptionRequestsRejected( + abortedQueryActionsQueue + ) + ) + abortedQueryActionsQueue = [] + }) + dispatchQueued = true + } + // _Don't_ let the action reach the reducers now! + return + } + } + + const result = next(action) + + return result + } + } +} diff --git a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts index b7f8217bc8..825c2747c9 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts @@ -11,6 +11,16 @@ import type { export type ReferenceCacheCollection = never +function isObjectEmpty(obj: Record) { + // Apparently a for..in loop is faster than `Object.keys()` here: + // https://stackoverflow.com/a/59787784/62937 + for (let k in obj) { + // If there is at least one key, it's not empty + return false + } + return true +} + declare module '../../endpointDefinitions' { interface QueryExtraOptions< TagTypes extends string, @@ -38,6 +48,15 @@ export const THIRTY_TWO_BIT_MAX_TIMER_SECONDS = 2_147_483_647 / 1_000 - 1 export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => { const { removeQueryResult, unsubscribeQueryResult } = api.internalActions + function anySubscriptionsRemainingForKey( + queryCacheKey: string, + api: SubMiddlewareApi + ) { + const subscriptions = + api.getState()[reducerPath].subscriptions[queryCacheKey] + return !!subscriptions && !isObjectEmpty(subscriptions) + } + return (mwApi) => { const currentRemovalTimeouts: QueryStateMeta = {} @@ -94,6 +113,11 @@ export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => { ] as QueryDefinition const keepUnusedDataFor = endpointDefinition?.keepUnusedDataFor ?? config.keepUnusedDataFor + + if (keepUnusedDataFor === Infinity) { + // Hey, user said keep this forever! + return + } // Prevent `setTimeout` timers from overflowing a 32-bit internal int, by // clamping the max value to be at most 1000ms less than the 32-bit max. // Look, a 24.8-day keepalive ought to be enough for anybody, right? :) @@ -103,18 +127,18 @@ export const build: SubMiddlewareBuilder = ({ reducerPath, api, context }) => { Math.min(keepUnusedDataFor, THIRTY_TWO_BIT_MAX_TIMER_SECONDS) ) - const currentTimeout = currentRemovalTimeouts[queryCacheKey] - if (currentTimeout) { - clearTimeout(currentTimeout) - } - currentRemovalTimeouts[queryCacheKey] = setTimeout(() => { - const subscriptions = - api.getState()[reducerPath].subscriptions[queryCacheKey] - if (!subscriptions || Object.keys(subscriptions).length === 0) { - api.dispatch(removeQueryResult({ queryCacheKey })) + if (!anySubscriptionsRemainingForKey(queryCacheKey, api)) { + const currentTimeout = currentRemovalTimeouts[queryCacheKey] + if (currentTimeout) { + clearTimeout(currentTimeout) } - delete currentRemovalTimeouts![queryCacheKey] - }, finalKeepUnusedDataFor * 1000) + currentRemovalTimeouts[queryCacheKey] = setTimeout(() => { + if (!anySubscriptionsRemainingForKey(queryCacheKey, api)) { + api.dispatch(removeQueryResult({ queryCacheKey })) + } + delete currentRemovalTimeouts![queryCacheKey] + }, finalKeepUnusedDataFor * 1000) + } } } } diff --git a/packages/toolkit/src/query/core/buildMiddleware/index.ts b/packages/toolkit/src/query/core/buildMiddleware/index.ts index c7a9ed1330..7732daac61 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/index.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/index.ts @@ -17,6 +17,7 @@ import { build as buildWindowEventHandling } from './windowEventHandling' import { build as buildCacheLifecycle } from './cacheLifecycle' import { build as buildQueryLifecycle } from './queryLifecycle' import { build as buildDevMiddleware } from './devMiddleware' +import { build as buildBatchActions } from './batchActions' export function buildMiddleware< Definitions extends EndpointDefinitions, @@ -38,6 +39,7 @@ export function buildMiddleware< buildWindowEventHandling, buildCacheLifecycle, buildQueryLifecycle, + buildBatchActions, ].map((build) => build({ ...(input as any as BuildMiddlewareInput< diff --git a/packages/toolkit/src/query/core/buildSlice.ts b/packages/toolkit/src/query/core/buildSlice.ts index 07ba888188..cdfa6625f4 100644 --- a/packages/toolkit/src/query/core/buildSlice.ts +++ b/packages/toolkit/src/query/core/buildSlice.ts @@ -23,7 +23,7 @@ import type { ConfigState, } from './apiState' import { QueryStatus } from './apiState' -import type { MutationThunk, QueryThunk } from './buildThunks' +import type { MutationThunk, QueryThunk, RejectedAction } from './buildThunks' import { calculateProvidedByThunk } from './buildThunks' import type { AssertTagTypes, @@ -387,6 +387,26 @@ export function buildSlice({ delete draft[queryCacheKey]![requestId] } }, + subscriptionRequestsRejected( + draft, + action: PayloadAction[]> + ) { + // We need to process "rejected" actions caused by a component trying to start a subscription + // after there's already a cache entry. Since many components may mount at once and all want + // the same data, we use a middleware that intercepts those actions batches these together + // into a single larger action , and we'll process all of them at once. + for (let rejectedAction of action.payload) { + const { + meta: { condition, arg, requestId }, + } = rejectedAction + // request was aborted due to condition (another query already running) + if (condition && arg.subscribe) { + const substate = (draft[arg.queryCacheKey] ??= {}) + substate[requestId] = + arg.subscriptionOptions ?? substate[requestId] ?? {} + } + } + }, }, extraReducers: (builder) => { builder @@ -403,17 +423,6 @@ export function buildSlice({ arg.subscriptionOptions ?? substate[requestId] ?? {} } }) - .addCase( - queryThunk.rejected, - (draft, { meta: { condition, arg, requestId }, error, payload }) => { - // request was aborted due to condition (another query already running) - if (condition && arg.subscribe) { - const substate = (draft[arg.queryCacheKey] ??= {}) - substate[requestId] = - arg.subscriptionOptions ?? substate[requestId] ?? {} - } - } - ) // update the state to be a new object to be picked up as a "state change" // by redux-persist's `autoMergeLevel2` .addMatcher(hasRehydrationInfo, (draft) => ({ ...draft })) diff --git a/packages/toolkit/src/query/react/buildHooks.ts b/packages/toolkit/src/query/react/buildHooks.ts index 096a0fe04a..9e1aa9ce83 100644 --- a/packages/toolkit/src/query/react/buildHooks.ts +++ b/packages/toolkit/src/query/react/buildHooks.ts @@ -696,16 +696,25 @@ export function buildHooks({ pollingInterval, }) + const lastRenderHadSubscription = useRef(false) + const promiseRef = useRef>() let { queryCacheKey, requestId } = promiseRef.current || {} - const subscriptionRemoved = useSelector( + const currentRenderHasSubscription = useSelector( (state: RootState) => !!queryCacheKey && !!requestId && !state[api.reducerPath].subscriptions[queryCacheKey]?.[requestId] ) + const subscriptionRemoved = + !currentRenderHasSubscription && lastRenderHadSubscription.current + + usePossiblyImmediateEffect(() => { + lastRenderHadSubscription.current = currentRenderHasSubscription + }) + usePossiblyImmediateEffect((): void | undefined => { promiseRef.current = undefined }, [subscriptionRemoved]) @@ -736,6 +745,7 @@ export function buildHooks({ forceRefetch: refetchOnMountOrArgChange, }) ) + promiseRef.current = promise } else if (stableSubscriptionOptions !== lastSubscriptionOptions) { lastPromise.updateSubscriptionOptions(stableSubscriptionOptions) @@ -923,8 +933,9 @@ export function buildHooks({ ...options, }) - const { data, status, isLoading, isSuccess, isError, error } = queryStateResults; - useDebugValue({ data, status, isLoading, isSuccess, isError, error }); + const { data, status, isLoading, isSuccess, isError, error } = + queryStateResults + useDebugValue({ data, status, isLoading, isSuccess, isError, error }) return useMemo( () => ({ ...queryStateResults, ...querySubscriptionResults }), @@ -993,8 +1004,24 @@ export function buildHooks({ }) }, [dispatch, fixedCacheKey, promise, requestId]) - const { endpointName, data, status, isLoading, isSuccess, isError, error } = currentState; - useDebugValue({ endpointName, data, status, isLoading, isSuccess, isError, error }); + const { + endpointName, + data, + status, + isLoading, + isSuccess, + isError, + error, + } = currentState + useDebugValue({ + endpointName, + data, + status, + isLoading, + isSuccess, + isError, + error, + }) const finalState = useMemo( () => ({ ...currentState, originalArgs, reset }), diff --git a/packages/toolkit/src/query/tests/buildHooks.test.tsx b/packages/toolkit/src/query/tests/buildHooks.test.tsx index 8af0b0fb10..371711a263 100644 --- a/packages/toolkit/src/query/tests/buildHooks.test.tsx +++ b/packages/toolkit/src/query/tests/buildHooks.test.tsx @@ -1162,9 +1162,12 @@ describe('hooks tests', () => { }) test('useMutation return value contains originalArgs', async () => { - const { result } = renderHook(() => api.endpoints.updateUser.useMutation(), { - wrapper: storeRef.wrapper, - }) + const { result } = renderHook( + () => api.endpoints.updateUser.useMutation(), + { + wrapper: storeRef.wrapper, + } + ) const arg = { name: 'Foo' } const firstRenderResult = result.current @@ -1955,13 +1958,13 @@ describe('hooks with createApi defaults set', () => { const addBtn = screen.getByTestId('addPost') - await waitFor(() => expect(getRenderCount()).toBe(3)) + await waitFor(() => expect(getRenderCount()).toBe(4)) fireEvent.click(addBtn) - await waitFor(() => expect(getRenderCount()).toBe(5)) + await waitFor(() => expect(getRenderCount()).toBe(6)) fireEvent.click(addBtn) fireEvent.click(addBtn) - await waitFor(() => expect(getRenderCount()).toBe(7)) + await waitFor(() => expect(getRenderCount()).toBe(8)) }) test('useQuery with selectFromResult option serves a deeply memoized value and does not rerender unnecessarily', async () => { diff --git a/packages/toolkit/src/query/tests/cacheCollection.test.ts b/packages/toolkit/src/query/tests/cacheCollection.test.ts index 80eb3ac9eb..29ccce1fb4 100644 --- a/packages/toolkit/src/query/tests/cacheCollection.test.ts +++ b/packages/toolkit/src/query/tests/cacheCollection.test.ts @@ -101,6 +101,10 @@ describe(`query: await cleanup, keepUnusedDataFor set`, () => { query: () => '/success', keepUnusedDataFor: 0, }), + query4: build.query({ + query: () => '/success', + keepUnusedDataFor: Infinity, + }), }), keepUnusedDataFor: 29, }) @@ -126,9 +130,18 @@ describe(`query: await cleanup, keepUnusedDataFor set`, () => { expect(onCleanup).not.toHaveBeenCalled() store.dispatch(api.endpoints.query3.initiate('arg')).unsubscribe() expect(onCleanup).not.toHaveBeenCalled() - jest.advanceTimersByTime(1), await waitMs() + jest.advanceTimersByTime(1) + await waitMs() expect(onCleanup).toHaveBeenCalled() }) + + test('endpoint keepUnusedDataFor: Infinity', async () => { + expect(onCleanup).not.toHaveBeenCalled() + store.dispatch(api.endpoints.query4.initiate('arg')).unsubscribe() + expect(onCleanup).not.toHaveBeenCalled() + jest.advanceTimersByTime(THIRTY_TWO_BIT_MAX_INT) + expect(onCleanup).not.toHaveBeenCalled() + }) }) function storeForApi< diff --git a/packages/toolkit/src/query/tests/cleanup.test.tsx b/packages/toolkit/src/query/tests/cleanup.test.tsx index 81cb44d31d..5aebc49758 100644 --- a/packages/toolkit/src/query/tests/cleanup.test.tsx +++ b/packages/toolkit/src/query/tests/cleanup.test.tsx @@ -2,12 +2,13 @@ import React from 'react' +import { createListenerMiddleware } from '@reduxjs/toolkit' import { createApi, QueryStatus } from '@reduxjs/toolkit/query/react' -import { render, waitFor } from '@testing-library/react' +import { render, waitFor, act, screen } from '@testing-library/react' import { setupApiStore } from './helpers' const api = createApi({ - baseQuery: () => ({ data: null }), + baseQuery: () => ({ data: 42 }), endpoints: (build) => ({ a: build.query({ query: () => '' }), b: build.query({ query: () => '' }), @@ -19,18 +20,21 @@ let getSubStateA = () => storeRef.store.getState().api.queries['a(undefined)'] let getSubStateB = () => storeRef.store.getState().api.queries['b(undefined)'] function UsingA() { - api.endpoints.a.useQuery() - return <> + const { data } = api.endpoints.a.useQuery() + + return <>Result: {data} } function UsingB() { api.endpoints.b.useQuery() + return <> } function UsingAB() { api.endpoints.a.useQuery() api.endpoints.b.useQuery() + return <> } @@ -90,11 +94,15 @@ test('data stays in store when component stays rendered while data for another c const statusA = getSubStateA() - rerender( - <> - - - ) + await act(async () => { + rerender( + <> + + + ) + + jest.advanceTimersByTime(10) + }) jest.advanceTimersByTime(120000) @@ -108,8 +116,8 @@ test('data stays in store when one component requiring the data stays in the sto const { rerender } = render( <> - - + + , { wrapper: storeRef.wrapper } ) @@ -121,14 +129,81 @@ test('data stays in store when one component requiring the data stays in the sto const statusA = getSubStateA() const statusB = getSubStateB() - rerender( - <> - - - ) + await act(async () => { + rerender( + <> + + + ) + jest.advanceTimersByTime(10) + jest.runAllTimers() + }) - jest.advanceTimersByTime(120000) + await act(async () => { + jest.advanceTimersByTime(120000) + jest.runAllTimers() + }) expect(getSubStateA()).toEqual(statusA) expect(getSubStateB()).toEqual(statusB) }) + +test('Minimizes the number of subscription dispatches when multiple components ask for the same data', async () => { + const listenerMiddleware = createListenerMiddleware() + const storeRef = setupApiStore(api, undefined, { + middleware: [listenerMiddleware.middleware], + withoutTestLifecycles: true, + }) + + let getSubscriptionsA = () => + storeRef.store.getState().api.subscriptions['a(undefined)'] + + let actionTypes: string[] = [] + + listenerMiddleware.startListening({ + predicate: () => true, + effect: (action) => { + actionTypes.push(action.type) + }, + }) + + const NUM_LIST_ITEMS = 1000 + + function ParentComponent() { + const listItems = Array.from({ length: NUM_LIST_ITEMS }).map((_, i) => ( + + )) + + return <>{listItems} + } + + const start = Date.now() + + render(, { + wrapper: storeRef.wrapper, + }) + + jest.advanceTimersByTime(10) + + await waitFor(() => { + return screen.getAllByText(/42/).length > 0 + }) + + const end = Date.now() + + const timeElapsed = end - start + + const subscriptions = getSubscriptionsA() + + expect(Object.keys(subscriptions!).length).toBe(NUM_LIST_ITEMS) + // Expected: [ + // 'api/config/middlewareRegistered', + // 'api/executeQuery/pending', + // 'api/subscriptions/subscriptionRequestsRejected', + // 'api/executeQuery/fulfilled' + // ] + expect(actionTypes.length).toBe(4) + // Could be flaky in CI, but we'll see. + // Currently seeing 1000ms in local dev, 6300 without the batching fixes + expect(timeElapsed).toBeLessThan(2500) +}, 25000) diff --git a/packages/toolkit/src/query/tests/helpers.tsx b/packages/toolkit/src/query/tests/helpers.tsx index 0f05428b7a..61581e91df 100644 --- a/packages/toolkit/src/query/tests/helpers.tsx +++ b/packages/toolkit/src/query/tests/helpers.tsx @@ -176,14 +176,16 @@ export function setupApiStore< >( api: A, extraReducers?: R, - options: { withoutListeners?: boolean; withoutTestLifecycles?: boolean } = {} + options: { withoutListeners?: boolean; withoutTestLifecycles?: boolean, middleware?: Middleware[] } = {} ) { + const { middleware = [] } = options; const getStore = () => configureStore({ reducer: { api: api.reducer, ...extraReducers }, middleware: (gdm) => gdm({ serializableCheck: false, immutableCheck: false }).concat( - api.middleware + api.middleware, + ...middleware ), }) diff --git a/packages/toolkit/src/query/tests/polling.test.tsx b/packages/toolkit/src/query/tests/polling.test.tsx index af60e7c23e..15a9710ed9 100644 --- a/packages/toolkit/src/query/tests/polling.test.tsx +++ b/packages/toolkit/src/query/tests/polling.test.tsx @@ -21,6 +21,10 @@ const { getPosts } = api.endpoints const storeRef = setupApiStore(api) +function delay(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + const getSubscribersForQueryCacheKey = (queryCacheKey: string) => storeRef.store.getState()[api.reducerPath].subscriptions[queryCacheKey] || {} const createSubscriptionGetter = (queryCacheKey: string) => () => @@ -79,6 +83,8 @@ describe('polling tests', () => { }) ) + await delay(10) + const getSubs = createSubscriptionGetter(subscriptionOne.queryCacheKey) expect(Object.keys(getSubs())).toHaveLength(2) diff --git a/packages/toolkit/src/query/tests/useMutation-fixedCacheKey.test.tsx b/packages/toolkit/src/query/tests/useMutation-fixedCacheKey.test.tsx index 6ea36d764d..ded2e71b4e 100644 --- a/packages/toolkit/src/query/tests/useMutation-fixedCacheKey.test.tsx +++ b/packages/toolkit/src/query/tests/useMutation-fixedCacheKey.test.tsx @@ -271,14 +271,14 @@ describe('fixedCacheKey', () => { test('a component without `fixedCacheKey` has `originalArgs`', async () => { render(, { wrapper: storeRef.wrapper, - legacyRoot: true, }) let c1 = screen.getByTestId('C1') expect(getByTestId(c1, 'status').textContent).toBe('uninitialized') expect(getByTestId(c1, 'originalArgs').textContent).toBe('undefined') - act(() => { + await act(async () => { getByTestId(c1, 'trigger').click() + await Promise.resolve() }) expect(getByTestId(c1, 'originalArgs').textContent).toBe('C1') @@ -312,15 +312,16 @@ describe('fixedCacheKey', () => { , - { wrapper: storeRef.wrapper, legacyRoot: true } + { wrapper: storeRef.wrapper } ) const c1 = screen.getByTestId('C1') const c2 = screen.getByTestId('C2') expect(getByTestId(c1, 'status').textContent).toBe('uninitialized') expect(getByTestId(c2, 'status').textContent).toBe('uninitialized') - act(() => { + await act(async () => { getByTestId(c1, 'trigger').click() + await Promise.resolve() }) expect(getByTestId(c1, 'status').textContent).toBe('pending') @@ -333,8 +334,9 @@ describe('fixedCacheKey', () => { expect(getByTestId(c1, 'status').textContent).toBe('pending') expect(getByTestId(c1, 'data').textContent).toBe('') - act(() => { + await act(async () => { resolve1!('this should not show up any more') + await Promise.resolve() }) await waitMs() @@ -342,8 +344,9 @@ describe('fixedCacheKey', () => { expect(getByTestId(c1, 'status').textContent).toBe('pending') expect(getByTestId(c1, 'data').textContent).toBe('') - act(() => { + await act(async () => { resolve2!('this should be visible') + await Promise.resolve() }) await waitMs()