From 6975282e87ce98f9a548043c15a82aacf3c384fc Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Sat, 8 Oct 2022 15:46:46 -0400 Subject: [PATCH] Speed up subscription behavior by tracking state in middleware --- .../core/buildMiddleware/batchActions.ts | 96 ++++++++++---- .../core/buildMiddleware/cacheCollection.ts | 19 ++- .../core/buildMiddleware/cacheLifecycle.ts | 1 + .../src/query/core/buildMiddleware/index.ts | 33 ++++- .../src/query/core/buildMiddleware/polling.ts | 33 +++-- .../src/query/core/buildMiddleware/types.ts | 15 ++- .../buildMiddleware/windowEventHandling.ts | 16 ++- packages/toolkit/src/query/core/buildSlice.ts | 125 +++++++++++------- .../toolkit/src/query/core/buildThunks.ts | 2 + .../toolkit/src/query/react/buildHooks.ts | 21 ++- .../src/query/tests/buildHooks.test.tsx | 101 +++++++++----- .../src/query/tests/buildMiddleware.test.tsx | 3 + .../src/query/tests/buildSlice.test.ts | 29 ++-- .../toolkit/src/query/tests/cleanup.test.tsx | 84 ++++++++++-- packages/toolkit/src/query/tests/helpers.tsx | 26 +++- .../toolkit/src/query/tests/matchers.test.tsx | 9 ++ .../query/tests/optimisticUpserts.test.tsx | 5 +- .../toolkit/src/query/tests/polling.test.tsx | 8 +- .../query/tests/refetchingBehaviors.test.tsx | 17 ++- packages/toolkit/src/utils.ts | 4 + 20 files changed, 456 insertions(+), 191 deletions(-) diff --git a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts index 60091d5075..8c7ef9763c 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts @@ -1,5 +1,7 @@ import type { QueryThunk, RejectedAction } from '../buildThunks' import type { InternalHandlerBuilder } from './types' +import type { SubscriptionState } from '../apiState' +import { produceWithPatches } from 'immer' // Copied from https://github.com/feross/queue-microtask let promise: Promise @@ -14,39 +16,75 @@ const queueMicrotaskShim = }, 0) ) -export const buildBatchedActionsHandler: InternalHandlerBuilder = ({ - api, - queryThunk, -}) => { - let abortedQueryActionsQueue: RejectedAction[] = [] +export const buildBatchedActionsHandler: InternalHandlerBuilder< + [actionShouldContinue: boolean, subscriptionExists: boolean] +> = ({ api, queryThunk }) => { + const { actuallyMutateSubscriptions } = api.internalActions + const subscriptionsPrefix = `${api.reducerPath}/subscriptions` + + let previousSubscriptions: SubscriptionState = + null as unknown as SubscriptionState + let dispatchQueued = false - return (action, mwApi) => { - 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 = false - }) - dispatchQueued = true - } - // _Don't_ let the action reach the reducers now! - return false + return (action, mwApi, internalState) => { + if (!previousSubscriptions) { + // Initialize it the first time this handler runs + previousSubscriptions = JSON.parse( + JSON.stringify(internalState.currentSubscriptions) + ) + } + + // Intercept requests by hooks to see if they're subscribed + // Necessary because we delay updating store state to the end of the tick + if (api.internalActions.internal_probeSubscription.match(action)) { + const { queryCacheKey, requestId } = action.payload + const hasSubscription = + !!internalState.currentSubscriptions[queryCacheKey]?.[requestId] + return [false, hasSubscription] + } + + // Update subscription data based on this action + const didMutate = actuallyMutateSubscriptions( + internalState.currentSubscriptions, + action + ) + + if (didMutate) { + if (!dispatchQueued) { + queueMicrotaskShim(() => { + // Deep clone the current subscription data + const newSubscriptions: SubscriptionState = JSON.parse( + JSON.stringify(internalState.currentSubscriptions) + ) + // Figure out a smaller diff between original and current + const [, patches] = produceWithPatches( + previousSubscriptions, + () => newSubscriptions + ) + + // Sync the store state for visibility + mwApi.next(api.internalActions.subscriptionsUpdated(patches)) + // Save the cloned state for later reference + previousSubscriptions = newSubscriptions + dispatchQueued = false + }) + dispatchQueued = true } + + const isSubscriptionSliceAction = + !!action.type?.startsWith(subscriptionsPrefix) + const isAdditionalSubscriptionAction = + queryThunk.rejected.match(action) && + action.meta.condition && + !!action.meta.arg.subscribe + + const actionShouldContinue = + !isSubscriptionSliceAction && !isAdditionalSubscriptionAction + + return [actionShouldContinue, false] } - return true + return [true, false] } } diff --git a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts index 4f17d37e75..41548be8b1 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts @@ -7,6 +7,7 @@ import type { TimeoutId, InternalHandlerBuilder, ApiMiddlewareInternalHandler, + InternalMiddlewareState, } from './types' export type ReferenceCacheCollection = never @@ -54,16 +55,19 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ function anySubscriptionsRemainingForKey( queryCacheKey: string, - api: SubMiddlewareApi + internalState: InternalMiddlewareState ) { - const subscriptions = - api.getState()[reducerPath].subscriptions[queryCacheKey] + const subscriptions = internalState.currentSubscriptions[queryCacheKey] return !!subscriptions && !isObjectEmpty(subscriptions) } const currentRemovalTimeouts: QueryStateMeta = {} - const handler: ApiMiddlewareInternalHandler = (action, mwApi) => { + const handler: ApiMiddlewareInternalHandler = ( + action, + mwApi, + internalState + ) => { if (unsubscribeQueryResult.match(action)) { const state = mwApi.getState()[reducerPath] const { queryCacheKey } = action.payload @@ -72,6 +76,7 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey, state.queries[queryCacheKey]?.endpointName, mwApi, + internalState, state.config ) } @@ -94,6 +99,7 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey as QueryCacheKey, queryState?.endpointName, mwApi, + internalState, state.config ) } @@ -104,6 +110,7 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey: QueryCacheKey, endpointName: string | undefined, api: SubMiddlewareApi, + internalState: InternalMiddlewareState, config: ConfigState ) { const endpointDefinition = context.endpointDefinitions[ @@ -125,13 +132,13 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ Math.min(keepUnusedDataFor, THIRTY_TWO_BIT_MAX_TIMER_SECONDS) ) - if (!anySubscriptionsRemainingForKey(queryCacheKey, api)) { + if (!anySubscriptionsRemainingForKey(queryCacheKey, internalState)) { const currentTimeout = currentRemovalTimeouts[queryCacheKey] if (currentTimeout) { clearTimeout(currentTimeout) } currentRemovalTimeouts[queryCacheKey] = setTimeout(() => { - if (!anySubscriptionsRemainingForKey(queryCacheKey, api)) { + if (!anySubscriptionsRemainingForKey(queryCacheKey, internalState)) { api.dispatch(removeQueryResult({ queryCacheKey })) } delete currentRemovalTimeouts![queryCacheKey] diff --git a/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts b/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts index 21a0e4af77..24ed38e300 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts @@ -197,6 +197,7 @@ export const buildCacheLifecycleHandler: InternalHandlerBuilder = ({ const handler: ApiMiddlewareInternalHandler = ( action, mwApi, + internalState, stateBefore ) => { const cacheKey = getCacheKey(action) diff --git a/packages/toolkit/src/query/core/buildMiddleware/index.ts b/packages/toolkit/src/query/core/buildMiddleware/index.ts index 09f2412e12..577b21ca41 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/index.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/index.ts @@ -10,7 +10,11 @@ import type { QueryThunkArg } from '../buildThunks' import { buildCacheCollectionHandler } from './cacheCollection' import { buildInvalidationByTagsHandler } from './invalidationByTags' import { buildPollingHandler } from './polling' -import type { BuildMiddlewareInput, InternalHandlerBuilder } from './types' +import type { + BuildMiddlewareInput, + InternalHandlerBuilder, + InternalMiddlewareState, +} from './types' import { buildWindowEventHandler } from './windowEventHandling' import { buildCacheLifecycleHandler } from './cacheLifecycle' import { buildQueryLifecycleHandler } from './queryLifecycle' @@ -69,6 +73,10 @@ export function buildMiddleware< const batchedActionsHandler = buildBatchedActionsHandler(builderArgs) const windowEventsHandler = buildWindowEventHandler(builderArgs) + let internalState: InternalMiddlewareState = { + currentSubscriptions: {}, + } + return (next) => { return (action) => { if (!initialized) { @@ -77,19 +85,30 @@ export function buildMiddleware< mwApi.dispatch(api.internalActions.middlewareRegistered(apiUid)) } + const mwApiWithNext = { ...mwApi, next } + const stateBefore = mwApi.getState() - if (!batchedActionsHandler(action, mwApi, stateBefore)) { - return - } + const [actionShouldContinue, hasSubscription] = batchedActionsHandler( + action, + mwApiWithNext, + internalState, + stateBefore + ) + + let res: any - const res = next(action) + if (actionShouldContinue) { + res = next(action) + } else { + res = hasSubscription + } if (!!mwApi.getState()[reducerPath]) { // Only run these checks if the middleware is registered okay // This looks for actions that aren't specific to the API slice - windowEventsHandler(action, mwApi, stateBefore) + windowEventsHandler(action, mwApiWithNext, internalState, stateBefore) if ( isThisApiSliceAction(action) || @@ -98,7 +117,7 @@ export function buildMiddleware< // Only run these additional checks if the actions are part of the API slice, // or the action has hydration-related data for (let handler of handlers) { - handler(action, mwApi, stateBefore) + handler(action, mwApiWithNext, internalState, stateBefore) } } } diff --git a/packages/toolkit/src/query/core/buildMiddleware/polling.ts b/packages/toolkit/src/query/core/buildMiddleware/polling.ts index 8a5ded5271..df7865901c 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/polling.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/polling.ts @@ -6,6 +6,7 @@ import type { TimeoutId, InternalHandlerBuilder, ApiMiddlewareInternalHandler, + InternalMiddlewareState, } from './types' export const buildPollingHandler: InternalHandlerBuilder = ({ @@ -20,26 +21,30 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ pollingInterval: number }> = {} - const handler: ApiMiddlewareInternalHandler = (action, mwApi) => { + const handler: ApiMiddlewareInternalHandler = ( + action, + mwApi, + internalState + ) => { if ( api.internalActions.updateSubscriptionOptions.match(action) || api.internalActions.unsubscribeQueryResult.match(action) ) { - updatePollingInterval(action.payload, mwApi) + updatePollingInterval(action.payload, mwApi, internalState) } if ( queryThunk.pending.match(action) || (queryThunk.rejected.match(action) && action.meta.condition) ) { - updatePollingInterval(action.meta.arg, mwApi) + updatePollingInterval(action.meta.arg, mwApi, internalState) } if ( queryThunk.fulfilled.match(action) || (queryThunk.rejected.match(action) && !action.meta.condition) ) { - startNextPoll(action.meta.arg, mwApi) + startNextPoll(action.meta.arg, mwApi, internalState) } if (api.util.resetApiState.match(action)) { @@ -49,11 +54,12 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ function startNextPoll( { queryCacheKey }: QuerySubstateIdentifier, - api: SubMiddlewareApi + api: SubMiddlewareApi, + internalState: InternalMiddlewareState ) { const state = api.getState()[reducerPath] const querySubState = state.queries[queryCacheKey] - const subscriptions = state.subscriptions[queryCacheKey] + const subscriptions = internalState.currentSubscriptions[queryCacheKey] if (!querySubState || querySubState.status === QueryStatus.uninitialized) return @@ -84,11 +90,12 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ function updatePollingInterval( { queryCacheKey }: QuerySubstateIdentifier, - api: SubMiddlewareApi + api: SubMiddlewareApi, + internalState: InternalMiddlewareState ) { const state = api.getState()[reducerPath] const querySubState = state.queries[queryCacheKey] - const subscriptions = state.subscriptions[queryCacheKey] + const subscriptions = internalState.currentSubscriptions[queryCacheKey] if (!querySubState || querySubState.status === QueryStatus.uninitialized) { return @@ -105,7 +112,7 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ const nextPollTimestamp = Date.now() + lowestPollingInterval if (!currentPoll || nextPollTimestamp < currentPoll.nextPollTimestamp) { - startNextPoll({ queryCacheKey }, api) + startNextPoll({ queryCacheKey }, api, internalState) } } @@ -125,13 +132,15 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ function findLowestPollingInterval(subscribers: Subscribers = {}) { let lowestPollingInterval = Number.POSITIVE_INFINITY - for (const subscription of Object.values(subscribers)) { - if (!!subscription.pollingInterval) + for (let key in subscribers) { + if (!!subscribers[key].pollingInterval) { lowestPollingInterval = Math.min( - subscription.pollingInterval, + subscribers[key].pollingInterval!, lowestPollingInterval ) + } } + return lowestPollingInterval } return handler diff --git a/packages/toolkit/src/query/core/buildMiddleware/types.ts b/packages/toolkit/src/query/core/buildMiddleware/types.ts index 5d79bd9b29..6262caa421 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/types.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/types.ts @@ -1,6 +1,7 @@ import type { AnyAction, AsyncThunkAction, + Dispatch, Middleware, MiddlewareAPI, ThunkDispatch, @@ -11,7 +12,12 @@ import type { AssertTagTypes, EndpointDefinitions, } from '../../endpointDefinitions' -import type { QueryStatus, QuerySubState, RootState } from '../apiState' +import type { + QueryStatus, + QuerySubState, + RootState, + SubscriptionState, +} from '../apiState' import type { MutationThunk, QueryThunk, @@ -22,6 +28,10 @@ import type { export type QueryStateMeta = Record export type TimeoutId = ReturnType +export interface InternalMiddlewareState { + currentSubscriptions: SubscriptionState +} + export interface BuildMiddlewareInput< Definitions extends EndpointDefinitions, ReducerPath extends string, @@ -62,7 +72,8 @@ export type SubMiddlewareBuilder = ( export type ApiMiddlewareInternalHandler = ( action: AnyAction, - mwApi: SubMiddlewareApi, + mwApi: SubMiddlewareApi & { next: Dispatch }, + internalState: InternalMiddlewareState, prevState: RootState ) => ReturnType diff --git a/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts b/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts index 0345c2e14c..a0d9941bc3 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts @@ -4,6 +4,7 @@ import { onFocus, onOnline } from '../setupListeners' import type { ApiMiddlewareInternalHandler, InternalHandlerBuilder, + InternalMiddlewareState, SubMiddlewareApi, } from './types' @@ -15,22 +16,27 @@ export const buildWindowEventHandler: InternalHandlerBuilder = ({ }) => { const { removeQueryResult } = api.internalActions - const handler: ApiMiddlewareInternalHandler = (action, mwApi) => { + const handler: ApiMiddlewareInternalHandler = ( + action, + mwApi, + internalState + ) => { if (onFocus.match(action)) { - refetchValidQueries(mwApi, 'refetchOnFocus') + refetchValidQueries(mwApi, 'refetchOnFocus', internalState) } if (onOnline.match(action)) { - refetchValidQueries(mwApi, 'refetchOnReconnect') + refetchValidQueries(mwApi, 'refetchOnReconnect', internalState) } } function refetchValidQueries( api: SubMiddlewareApi, - type: 'refetchOnFocus' | 'refetchOnReconnect' + type: 'refetchOnFocus' | 'refetchOnReconnect', + internalState: InternalMiddlewareState ) { const state = api.getState()[reducerPath] const queries = state.queries - const subscriptions = state.subscriptions + const subscriptions = internalState.currentSubscriptions context.batch(() => { for (const queryCacheKey of Object.keys(subscriptions)) { diff --git a/packages/toolkit/src/query/core/buildSlice.ts b/packages/toolkit/src/query/core/buildSlice.ts index c9fe6e6406..2b91606132 100644 --- a/packages/toolkit/src/query/core/buildSlice.ts +++ b/packages/toolkit/src/query/core/buildSlice.ts @@ -380,15 +380,14 @@ export function buildSlice({ }, }) + // Dummy slice to generate actions const subscriptionSlice = createSlice({ name: `${reducerPath}/subscriptions`, initialState: initialState as SubscriptionState, reducers: { updateSubscriptionOptions( - draft, - { - payload: { queryCacheKey, requestId, options }, - }: PayloadAction< + d, + a: PayloadAction< { endpointName: string requestId: string @@ -396,59 +395,81 @@ export function buildSlice({ } & QuerySubstateIdentifier > ) { - if (draft?.[queryCacheKey]?.[requestId]) { - draft[queryCacheKey]![requestId] = options - } + // Dummy }, unsubscribeQueryResult( - draft, - { - payload: { queryCacheKey, requestId }, - }: PayloadAction<{ requestId: string } & QuerySubstateIdentifier> + d, + a: PayloadAction<{ requestId: string } & QuerySubstateIdentifier> ) { - if (draft[queryCacheKey]) { - delete draft[queryCacheKey]![requestId] - } + // Dummy }, - subscriptionRequestsRejected( - draft, - action: PayloadAction[]> + internal_probeSubscription( + d, + a: PayloadAction<{ queryCacheKey: string; requestId: string }> ) { - // 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] ?? {} - } - } + // dummy }, }, - extraReducers: (builder) => { - builder - .addCase( - querySlice.actions.removeQueryResult, - (draft, { payload: { queryCacheKey } }) => { - delete draft[queryCacheKey] - } - ) - .addCase(queryThunk.pending, (draft, { meta: { arg, requestId } }) => { - if (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 })) + }) + + const { updateSubscriptionOptions, unsubscribeQueryResult } = + subscriptionSlice.actions + + // Actually intentionally mutate the subscriptions state used in the middleware + // This is done to speed up perf when loading many components + const actuallyMutateSubscriptions = ( + draft: SubscriptionState, + action: AnyAction + ) => { + if (updateSubscriptionOptions.match(action)) { + const { queryCacheKey, requestId, options } = action.payload + + if (draft?.[queryCacheKey]?.[requestId]) { + draft[queryCacheKey]![requestId] = options + } + return true + } else if (unsubscribeQueryResult.match(action)) { + const { queryCacheKey, requestId } = action.payload + if (draft[queryCacheKey]) { + delete draft[queryCacheKey]![requestId] + } + return true + } else if (querySlice.actions.removeQueryResult.match(action)) { + delete draft[action.payload.queryCacheKey] + } else if (queryThunk.pending.match(action)) { + const { + meta: { arg, requestId }, + } = action + if (arg.subscribe) { + const substate = (draft[arg.queryCacheKey] ??= {}) + substate[requestId] = + arg.subscriptionOptions ?? substate[requestId] ?? {} + + return true + } + } else if (queryThunk.rejected.match(action)) { + const { + meta: { condition, arg, requestId }, + } = action + if (condition && arg.subscribe) { + const substate = (draft[arg.queryCacheKey] ??= {}) + substate[requestId] = + arg.subscriptionOptions ?? substate[requestId] ?? {} + + return true + } + } + + return false + } + + const internalSubscriptionsSlice = createSlice({ + name: `${reducerPath}/internalSubscriptions`, + initialState: initialState as SubscriptionState, + reducers: { + subscriptionsUpdated(state, action: PayloadAction) { + return applyPatches(state, action.payload) + }, }, }) @@ -494,7 +515,7 @@ export function buildSlice({ queries: querySlice.reducer, mutations: mutationSlice.reducer, provided: invalidationSlice.reducer, - subscriptions: subscriptionSlice.reducer, + subscriptions: internalSubscriptionsSlice.reducer, config: configSlice.reducer, }) @@ -505,10 +526,12 @@ export function buildSlice({ ...configSlice.actions, ...querySlice.actions, ...subscriptionSlice.actions, + ...internalSubscriptionsSlice.actions, ...mutationSlice.actions, /** @deprecated has been renamed to `removeMutationResult` */ unsubscribeMutationResult: mutationSlice.actions.removeMutationResult, resetApiState, + actuallyMutateSubscriptions, } return { reducer, actions } diff --git a/packages/toolkit/src/query/core/buildThunks.ts b/packages/toolkit/src/query/core/buildThunks.ts index 67291aad07..895a9ae3c7 100644 --- a/packages/toolkit/src/query/core/buildThunks.ts +++ b/packages/toolkit/src/query/core/buildThunks.ts @@ -486,6 +486,8 @@ In the case of an unhandled error, no tags will be "provided" or "invalidated".` const endpointDefinition = endpointDefinitions[queryThunkArgs.endpointName] + // console.trace('Query thunk: ', currentArg) + // Order of these checks matters. // In order for `upsertQueryData` to successfully run while an existing request is in flight, /// we have to check for that first, otherwise `queryThunk` will bail out and not run at all. diff --git a/packages/toolkit/src/query/react/buildHooks.ts b/packages/toolkit/src/query/react/buildHooks.ts index a6b518766f..a6ee6b5811 100644 --- a/packages/toolkit/src/query/react/buildHooks.ts +++ b/packages/toolkit/src/query/react/buildHooks.ts @@ -701,12 +701,21 @@ export function buildHooks({ const promiseRef = useRef>() let { queryCacheKey, requestId } = promiseRef.current || {} - const currentRenderHasSubscription = useSelector( - (state: RootState) => - !!queryCacheKey && - !!requestId && - !state[api.reducerPath].subscriptions[queryCacheKey]?.[requestId] - ) + + // HACK Because the latest state is in the middleware, we actually + // dispatch an action that will be intercepted and returned. + let currentRenderHasSubscription = false + if (queryCacheKey && requestId) { + // This _should_ return a boolean, even if the types don't line up + const returnedValue = dispatch( + api.internalActions.internal_probeSubscription({ + queryCacheKey, + requestId, + }) + ) + // console.log('Probe subscription check: ', returnedValue) + currentRenderHasSubscription = !!returnedValue + } const subscriptionRemoved = !currentRenderHasSubscription && lastRenderHadSubscription.current diff --git a/packages/toolkit/src/query/tests/buildHooks.test.tsx b/packages/toolkit/src/query/tests/buildHooks.test.tsx index 5806252c75..187895b12c 100644 --- a/packages/toolkit/src/query/tests/buildHooks.test.tsx +++ b/packages/toolkit/src/query/tests/buildHooks.test.tsx @@ -24,8 +24,9 @@ import { import { server } from './mocks/server' import type { AnyAction } from 'redux' import type { SubscriptionOptions } from '@reduxjs/toolkit/dist/query/core/apiState' -import type { SerializedError } from '@reduxjs/toolkit' +import { createListenerMiddleware, SerializedError } from '@reduxjs/toolkit' import { renderHook } from '@testing-library/react' +import { delay } from '../../utils' // Just setup a temporary in-memory counter for tests that `getIncrementedAmount`. // This can be used to test how many renders happen due to data changes or @@ -82,14 +83,37 @@ const api = createApi({ }), }) -const storeRef = setupApiStore(api, { - actions(state: AnyAction[] = [], action: AnyAction) { - return [...state, action] +const listenerMiddleware = createListenerMiddleware() + +let actions: AnyAction[] = [] + +const storeRef = setupApiStore( + api, + { + // actions(state: AnyAction[] = [], action: AnyAction) { + // return [...state, action] + // }, }, + { + middleware: { + prepend: [listenerMiddleware.middleware], + }, + } +) + +beforeEach(() => { + actions = [] + listenerMiddleware.startListening({ + predicate: () => true, + effect: (action) => { + actions.push(action) + }, + }) }) afterEach(() => { amount = 0 + listenerMiddleware.clearListeners() }) let getRenderCount: () => number = () => 0 @@ -765,9 +789,7 @@ describe('hooks tests', () => { expect(getRenderCount()).toBe(9) expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.updateSubscriptionOptions.match) + actions.filter(api.internalActions.updateSubscriptionOptions.match) ).toHaveLength(1) }) @@ -811,9 +833,7 @@ describe('hooks tests', () => { // Being that there is only the initial query, no unsubscribe should be dispatched expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.unsubscribeQueryResult.match) + actions.filter(api.internalActions.unsubscribeQueryResult.match) ).toHaveLength(0) fireEvent.click(screen.getByTestId('fetchUser2')) @@ -826,34 +846,26 @@ describe('hooks tests', () => { ) expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.unsubscribeQueryResult.match) + actions.filter(api.internalActions.unsubscribeQueryResult.match) ).toHaveLength(1) fireEvent.click(screen.getByTestId('fetchUser1')) expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.unsubscribeQueryResult.match) + actions.filter(api.internalActions.unsubscribeQueryResult.match) ).toHaveLength(2) // we always unsubscribe the original promise and create a new one fireEvent.click(screen.getByTestId('fetchUser1')) expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.unsubscribeQueryResult.match) + actions.filter(api.internalActions.unsubscribeQueryResult.match) ).toHaveLength(3) unmount() // We unsubscribe after the component unmounts expect( - storeRef.store - .getState() - .actions.filter(api.internalActions.unsubscribeQueryResult.match) + actions.filter(api.internalActions.unsubscribeQueryResult.match) ).toHaveLength(4) }) @@ -1618,6 +1630,7 @@ describe('hooks tests', () => { expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, checkSession.matchPending, + api.internalActions.subscriptionsUpdated.match, checkSession.matchRejected, login.matchPending, login.matchFulfilled, @@ -1984,13 +1997,13 @@ describe('hooks with createApi defaults set', () => { const addBtn = screen.getByTestId('addPost') - await waitFor(() => expect(getRenderCount()).toBe(4)) + await waitFor(() => expect(getRenderCount()).toBe(3)) fireEvent.click(addBtn) - await waitFor(() => expect(getRenderCount()).toBe(6)) + await waitFor(() => expect(getRenderCount()).toBe(5)) fireEvent.click(addBtn) fireEvent.click(addBtn) - await waitFor(() => expect(getRenderCount()).toBe(8)) + await waitFor(() => expect(getRenderCount()).toBe(7)) }) test('useQuery with selectFromResult option serves a deeply memoized value and does not rerender unnecessarily', async () => { @@ -2353,14 +2366,25 @@ describe('skip behaviour', () => { ) expect(result.current).toEqual(uninitialized) + await delay(1) expect(subscriptionCount('getUser(1)')).toBe(0) - rerender([1]) - expect(result.current).toMatchObject({ status: QueryStatus.pending }) + await act(async () => { + rerender([1]) + }) + expect(result.current).toMatchObject({ status: QueryStatus.fulfilled }) + await delay(1) expect(subscriptionCount('getUser(1)')).toBe(1) - rerender([1, { skip: true }]) - expect(result.current).toEqual(uninitialized) + await act(async () => { + rerender([1, { skip: true }]) + }) + expect(result.current).toEqual({ + ...uninitialized, + currentData: undefined, + data: { name: 'Timmy' }, + }) + await delay(1) expect(subscriptionCount('getUser(1)')).toBe(0) }) @@ -2375,17 +2399,28 @@ 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({}) - rerender([1]) - expect(result.current).toMatchObject({ status: QueryStatus.pending }) + await act(async () => { + rerender([1]) + }) + expect(result.current).toMatchObject({ status: QueryStatus.fulfilled }) + await delay(1) expect(subscriptionCount('getUser(1)')).toBe(1) expect(storeRef.store.getState().api.subscriptions).not.toEqual({}) - rerender([skipToken]) - expect(result.current).toEqual(uninitialized) + await act(async () => { + rerender([skipToken]) + }) + expect(result.current).toEqual({ + ...uninitialized, + currentData: undefined, + data: { name: 'Timmy' }, + }) + await delay(1) expect(subscriptionCount('getUser(1)')).toBe(0) }) }) diff --git a/packages/toolkit/src/query/tests/buildMiddleware.test.tsx b/packages/toolkit/src/query/tests/buildMiddleware.test.tsx index f6154ea848..cd7ac0fb95 100644 --- a/packages/toolkit/src/query/tests/buildMiddleware.test.tsx +++ b/packages/toolkit/src/query/tests/buildMiddleware.test.tsx @@ -37,6 +37,7 @@ it('invalidates the specified tags', async () => { expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, getBanana.matchPending, + api.internalActions.subscriptionsUpdated.match, getBanana.matchFulfilled ) @@ -48,6 +49,7 @@ it('invalidates the specified tags', async () => { const firstSequence = [ api.internalActions.middlewareRegistered.match, getBanana.matchPending, + api.internalActions.subscriptionsUpdated.match, getBanana.matchFulfilled, api.util.invalidateTags.match, getBanana.matchPending, @@ -63,6 +65,7 @@ it('invalidates the specified tags', async () => { expect(storeRef.store.getState().actions).toMatchSequence( ...firstSequence, getBread.matchPending, + api.internalActions.subscriptionsUpdated.match, getBread.matchFulfilled, api.util.invalidateTags.match, getBread.matchPending, diff --git a/packages/toolkit/src/query/tests/buildSlice.test.ts b/packages/toolkit/src/query/tests/buildSlice.test.ts index 7d487b5542..71d16def35 100644 --- a/packages/toolkit/src/query/tests/buildSlice.test.ts +++ b/packages/toolkit/src/query/tests/buildSlice.test.ts @@ -1,13 +1,10 @@ import { createSlice } from '@reduxjs/toolkit' import { createApi } from '@reduxjs/toolkit/query' import { setupApiStore } from './helpers' +import { delay } from '../../utils' let shouldApiResponseSuccess = true -function delay(ms: number) { - return new Promise((resolve) => setTimeout(resolve, ms)) -} - const baseQuery = (args?: any) => ({ data: args }) const api = createApi({ baseQuery, @@ -50,7 +47,7 @@ describe('buildSlice', () => { getUser.initiate(1, { subscriptionOptions: { pollingInterval: 10 } }) ) - expect(storeRef.store.getState()).toEqual({ + const initialQueryState = { api: { config: { focused: true, @@ -63,10 +60,11 @@ describe('buildSlice', () => { refetchOnReconnect: false, }, mutations: {}, - provided: {}, + provided: expect.any(Object), queries: { 'getUser(1)': { data: { + success: true, url: 'user/1', }, endpointName: 'getUser', @@ -77,13 +75,26 @@ describe('buildSlice', () => { status: 'fulfilled', }, }, - subscriptions: { - 'getUser(1)': expect.any(Object), - }, + // Filled in a tick later + subscriptions: expect.any(Object), }, auth: { token: '1234', }, + } + + expect(storeRef.store.getState()).toEqual(initialQueryState) + + await delay(1) + + expect(storeRef.store.getState()).toEqual({ + ...initialQueryState, + api: { + ...initialQueryState.api, + subscriptions: { + 'getUser(1)': expect.any(Object), + }, + }, }) storeRef.store.dispatch(api.util.resetApiState()) diff --git a/packages/toolkit/src/query/tests/cleanup.test.tsx b/packages/toolkit/src/query/tests/cleanup.test.tsx index e9b9b95992..0c3e412a66 100644 --- a/packages/toolkit/src/query/tests/cleanup.test.tsx +++ b/packages/toolkit/src/query/tests/cleanup.test.tsx @@ -1,11 +1,16 @@ // tests for "cleanup-after-unsubscribe" behaviour -import React from 'react' +import React, { Profiler, ProfilerOnRenderCallback } from 'react' import { createListenerMiddleware } from '@reduxjs/toolkit' import { createApi, QueryStatus } from '@reduxjs/toolkit/query/react' import { render, waitFor, act, screen } from '@testing-library/react' import { setupApiStore } from './helpers' +import { delay } from '../../utils' + +const tick = () => new Promise((res) => setImmediate(res)) + +export const runAllTimers = async () => jest.runAllTimers() && (await tick()) const api = createApi({ baseQuery: () => ({ data: 42 }), @@ -151,10 +156,27 @@ test('data stays in store when one component requiring the data stays in the sto 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], + middleware: { + concat: [listenerMiddleware.middleware], + }, withoutTestLifecycles: true, }) + const onProfile: ProfilerOnRenderCallback = ( + id, + phase, + actualDuration, + baseDuration, + startTime, + commitTime + ) => { + console.table({ + phase, + actualDuration, + baseDuration, + }) + } + let getSubscriptionsA = () => storeRef.store.getState().api.subscriptions['a(undefined)'] @@ -177,24 +199,62 @@ test('Minimizes the number of subscription dispatches when multiple components a return <>{listItems} } - render(, { - wrapper: storeRef.wrapper, - }) + const start = Date.now() + + render( + // + , + // , + { + wrapper: storeRef.wrapper, + } + ) + + const afterRender = Date.now() jest.advanceTimersByTime(10) + const afterTimers1 = Date.now() + await waitFor(() => { return screen.getAllByText(/42/).length > 0 }) + const afterScreen = Date.now() + + await runAllTimers() + + const end = Date.now() + + const timeElapsed = end - start + // const renderTime = afterRender - start + // const timer1Time = afterTimers1 - afterRender + // const screenTime = afterScreen - afterTimers1 + // const timer2Time = end - afterScreen + + // console.table({ + // timeElapsed, + // renderTime, + // timer1Time, + // screenTime, + // timer2Time, + // }) + + // console.log('Getting final subscriptions') const subscriptions = getSubscriptionsA() + // console.log(actionTypes) 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) + + expect(actionTypes).toEqual([ + 'api/config/middlewareRegistered', + 'api/executeQuery/pending', + 'api/internalSubscriptions/subscriptionsUpdated', + 'api/executeQuery/fulfilled', + ]) + + // Could be flaky in CI, but we'll see. + // Currently seeing 800ms in local dev, 6300 without the batching fixes + // console.log('Elapsed subscription time: ', timeElapsed) + expect(timeElapsed).toBeLessThan(1500) }, 25000) diff --git a/packages/toolkit/src/query/tests/helpers.tsx b/packages/toolkit/src/query/tests/helpers.tsx index 61581e91df..faad21e41b 100644 --- a/packages/toolkit/src/query/tests/helpers.tsx +++ b/packages/toolkit/src/query/tests/helpers.tsx @@ -176,17 +176,29 @@ export function setupApiStore< >( api: A, extraReducers?: R, - options: { withoutListeners?: boolean; withoutTestLifecycles?: boolean, middleware?: Middleware[] } = {} + options: { + withoutListeners?: boolean + withoutTestLifecycles?: boolean + middleware?: { + prepend?: Middleware[] + concat?: Middleware[] + } + } = {} ) { - const { middleware = [] } = options; + const { middleware } = options const getStore = () => configureStore({ reducer: { api: api.reducer, ...extraReducers }, - middleware: (gdm) => - gdm({ serializableCheck: false, immutableCheck: false }).concat( - api.middleware, - ...middleware - ), + middleware: (gdm) => { + const tempMiddleware = gdm({ + serializableCheck: false, + immutableCheck: false, + }).concat(api.middleware) + + return tempMiddleware + .concat(...(middleware?.concat ?? [])) + .prepend(...(middleware?.prepend ?? [])) as typeof tempMiddleware + }, }) type StoreType = EnhancedStore< diff --git a/packages/toolkit/src/query/tests/matchers.test.tsx b/packages/toolkit/src/query/tests/matchers.test.tsx index 8d01490b9b..1a7f5847b0 100644 --- a/packages/toolkit/src/query/tests/matchers.test.tsx +++ b/packages/toolkit/src/query/tests/matchers.test.tsx @@ -65,21 +65,25 @@ test('matches query pending & fulfilled actions for the given endpoint', async ( expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchFulfilled ) expect(storeRef.store.getState().actions).not.toMatchSequence( api.internalActions.middlewareRegistered.match, otherEndpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, otherEndpoint.matchFulfilled ) expect(storeRef.store.getState().actions).not.toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchFulfilled, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) expect(storeRef.store.getState().actions).not.toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) }) @@ -92,6 +96,7 @@ test('matches query pending & rejected actions for the given endpoint', async () expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) expect(storeRef.store.getState().actions).not.toMatchSequence( @@ -117,17 +122,20 @@ test('matches lazy query pending & fulfilled actions for given endpoint', async expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchFulfilled ) expect(storeRef.store.getState().actions).not.toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchFulfilled, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) expect(storeRef.store.getState().actions).not.toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) }) @@ -143,6 +151,7 @@ test('matches lazy query pending & rejected actions for given endpoint', async ( expect(storeRef.store.getState().actions).toMatchSequence( api.internalActions.middlewareRegistered.match, endpoint.matchPending, + api.internalActions.subscriptionsUpdated.match, endpoint.matchRejected ) expect(storeRef.store.getState().actions).not.toMatchSequence( diff --git a/packages/toolkit/src/query/tests/optimisticUpserts.test.tsx b/packages/toolkit/src/query/tests/optimisticUpserts.test.tsx index 5ca0fd050e..0781665538 100644 --- a/packages/toolkit/src/query/tests/optimisticUpserts.test.tsx +++ b/packages/toolkit/src/query/tests/optimisticUpserts.test.tsx @@ -2,6 +2,7 @@ import { createApi } from '@reduxjs/toolkit/query/react' import { actionsReducer, hookWaitFor, setupApiStore, waitMs } from './helpers' import { skipToken } from '../core/buildSelectors' import { renderHook, act, waitFor } from '@testing-library/react' +import { delay } from '../../utils' interface Post { id: string @@ -12,10 +13,6 @@ interface Post { const baseQuery = jest.fn() beforeEach(() => baseQuery.mockReset()) -function delay(ms: number) { - return new Promise((resolve) => setTimeout(resolve, ms)) -} - const api = createApi({ baseQuery: (...args: any[]) => { const result = baseQuery(...args) diff --git a/packages/toolkit/src/query/tests/polling.test.tsx b/packages/toolkit/src/query/tests/polling.test.tsx index 15a9710ed9..357575b0da 100644 --- a/packages/toolkit/src/query/tests/polling.test.tsx +++ b/packages/toolkit/src/query/tests/polling.test.tsx @@ -1,5 +1,6 @@ import { createApi } from '@reduxjs/toolkit/query' import { setupApiStore, waitMs } from './helpers' +import { delay } from '../../utils' const mockBaseQuery = jest .fn() @@ -21,10 +22,6 @@ 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) => () => @@ -59,11 +56,13 @@ describe('polling tests', () => { const getSubs = createSubscriptionGetter(queryCacheKey) + await delay(1) expect(Object.keys(getSubs())).toHaveLength(1) expect(getSubs()[requestId].pollingInterval).toBe(10) subscription.updateSubscriptionOptions({ pollingInterval: 20 }) + await delay(1) expect(Object.keys(getSubs())).toHaveLength(1) expect(getSubs()[requestId].pollingInterval).toBe(20) }) @@ -91,6 +90,7 @@ describe('polling tests', () => { subscriptionOne.unsubscribe() + await delay(1) expect(Object.keys(getSubs())).toHaveLength(1) }) diff --git a/packages/toolkit/src/query/tests/refetchingBehaviors.test.tsx b/packages/toolkit/src/query/tests/refetchingBehaviors.test.tsx index 2e75cf95be..d531ae7b4e 100644 --- a/packages/toolkit/src/query/tests/refetchingBehaviors.test.tsx +++ b/packages/toolkit/src/query/tests/refetchingBehaviors.test.tsx @@ -2,7 +2,7 @@ import * as React from 'react' import { createApi, setupListeners } from '@reduxjs/toolkit/query/react' import { act, fireEvent, render, waitFor, screen } from '@testing-library/react' import { setupApiStore, waitMs } from './helpers' -import { AnyAction } from '@reduxjs/toolkit' +import { delay } from '../../utils' // Just setup a temporary in-memory counter for tests that `getIncrementedAmount`. // This can be used to test how many renders happen due to data changes or @@ -214,10 +214,11 @@ describe('refetchOnFocus tests', () => { expect(getIncrementedAmountState()).not.toBeUndefined() - act(() => { + await act(async () => { fireEvent.focus(window) }) + await delay(1) expect(getIncrementedAmountState()).toBeUndefined() }) }) @@ -372,7 +373,9 @@ describe('customListenersHandler', () => { test('setupListeners accepts a custom callback and executes it', async () => { const consoleSpy = jest.spyOn(console, 'log') - consoleSpy.mockImplementation(() => {}) + consoleSpy.mockImplementation((...args) => { + // console.info(...args) + }) const dispatchSpy = jest.spyOn(storeRef.store, 'dispatch') let unsubscribe = () => {} @@ -427,9 +430,15 @@ describe('customListenersHandler', () => { window.dispatchEvent(new Event('online')) }) expect(dispatchSpy).toHaveBeenCalled() + + // Ignore RTKQ middleware `internal_probeSubscription` calls + const mockCallsWithoutInternals = dispatchSpy.mock.calls.filter( + (call) => !(call[0] as any)?.type?.includes('internal') + ) + expect( defaultApi.internalActions.onOnline.match( - dispatchSpy.mock.calls[1][0] as any + mockCallsWithoutInternals[1][0] as any ) ).toBe(true) diff --git a/packages/toolkit/src/utils.ts b/packages/toolkit/src/utils.ts index c16eb8355a..40957c2788 100644 --- a/packages/toolkit/src/utils.ts +++ b/packages/toolkit/src/utils.ts @@ -23,6 +23,10 @@ It is disabled in production builds, so you don't need to worry about that.`) } } +export function delay(ms: number) { + return new Promise((resolve) => setTimeout(resolve, ms)) +} + /** * @public */