From 2b864edbad642f6e6adede6a2a9287a07e59d095 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 17 Oct 2022 22:24:28 -0400 Subject: [PATCH 1/4] Pass internal middleware state in on setup for simplicity --- .../core/buildMiddleware/batchActions.ts | 4 ++-- .../core/buildMiddleware/cacheCollection.ts | 13 ++++-------- .../core/buildMiddleware/cacheLifecycle.ts | 2 +- .../src/query/core/buildMiddleware/index.ts | 14 ++++++------- .../src/query/core/buildMiddleware/polling.ts | 21 +++++++------------ .../src/query/core/buildMiddleware/types.ts | 2 +- .../buildMiddleware/windowEventHandling.ts | 15 +++++-------- 7 files changed, 28 insertions(+), 43 deletions(-) diff --git a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts index 8c7ef9763c..72e08ee289 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts @@ -18,7 +18,7 @@ const queueMicrotaskShim = export const buildBatchedActionsHandler: InternalHandlerBuilder< [actionShouldContinue: boolean, subscriptionExists: boolean] -> = ({ api, queryThunk }) => { +> = ({ api, queryThunk, internalState }) => { const { actuallyMutateSubscriptions } = api.internalActions const subscriptionsPrefix = `${api.reducerPath}/subscriptions` @@ -27,7 +27,7 @@ export const buildBatchedActionsHandler: InternalHandlerBuilder< let dispatchQueued = false - return (action, mwApi, internalState) => { + return (action, mwApi) => { if (!previousSubscriptions) { // Initialize it the first time this handler runs previousSubscriptions = JSON.parse( diff --git a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts index 41548be8b1..becf6b8d1f 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/cacheCollection.ts @@ -50,13 +50,11 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ reducerPath, api, context, + internalState, }) => { const { removeQueryResult, unsubscribeQueryResult } = api.internalActions - function anySubscriptionsRemainingForKey( - queryCacheKey: string, - internalState: InternalMiddlewareState - ) { + function anySubscriptionsRemainingForKey(queryCacheKey: string) { const subscriptions = internalState.currentSubscriptions[queryCacheKey] return !!subscriptions && !isObjectEmpty(subscriptions) } @@ -76,7 +74,6 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey, state.queries[queryCacheKey]?.endpointName, mwApi, - internalState, state.config ) } @@ -99,7 +96,6 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey as QueryCacheKey, queryState?.endpointName, mwApi, - internalState, state.config ) } @@ -110,7 +106,6 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ queryCacheKey: QueryCacheKey, endpointName: string | undefined, api: SubMiddlewareApi, - internalState: InternalMiddlewareState, config: ConfigState ) { const endpointDefinition = context.endpointDefinitions[ @@ -132,13 +127,13 @@ export const buildCacheCollectionHandler: InternalHandlerBuilder = ({ Math.min(keepUnusedDataFor, THIRTY_TWO_BIT_MAX_TIMER_SECONDS) ) - if (!anySubscriptionsRemainingForKey(queryCacheKey, internalState)) { + if (!anySubscriptionsRemainingForKey(queryCacheKey)) { const currentTimeout = currentRemovalTimeouts[queryCacheKey] if (currentTimeout) { clearTimeout(currentTimeout) } currentRemovalTimeouts[queryCacheKey] = setTimeout(() => { - if (!anySubscriptionsRemainingForKey(queryCacheKey, internalState)) { + if (!anySubscriptionsRemainingForKey(queryCacheKey)) { 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 24ed38e300..923eb79d56 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/cacheLifecycle.ts @@ -183,6 +183,7 @@ export const buildCacheLifecycleHandler: InternalHandlerBuilder = ({ context, queryThunk, mutationThunk, + internalState, }) => { const isQueryThunk = isAsyncThunkAction(queryThunk) const isMutationThunk = isAsyncThunkAction(mutationThunk) @@ -197,7 +198,6 @@ 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 577b21ca41..810839e333 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/index.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/index.ts @@ -59,12 +59,17 @@ export function buildMiddleware< > = (mwApi) => { let initialized = false + let internalState: InternalMiddlewareState = { + currentSubscriptions: {}, + } + const builderArgs = { ...(input as any as BuildMiddlewareInput< EndpointDefinitions, string, string >), + internalState, refetchQuery, } @@ -73,10 +78,6 @@ export function buildMiddleware< const batchedActionsHandler = buildBatchedActionsHandler(builderArgs) const windowEventsHandler = buildWindowEventHandler(builderArgs) - let internalState: InternalMiddlewareState = { - currentSubscriptions: {}, - } - return (next) => { return (action) => { if (!initialized) { @@ -92,7 +93,6 @@ export function buildMiddleware< const [actionShouldContinue, hasSubscription] = batchedActionsHandler( action, mwApiWithNext, - internalState, stateBefore ) @@ -108,7 +108,7 @@ export function buildMiddleware< // Only run these checks if the middleware is registered okay // This looks for actions that aren't specific to the API slice - windowEventsHandler(action, mwApiWithNext, internalState, stateBefore) + windowEventsHandler(action, mwApiWithNext, stateBefore) if ( isThisApiSliceAction(action) || @@ -117,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, mwApiWithNext, internalState, stateBefore) + handler(action, mwApiWithNext, stateBefore) } } } diff --git a/packages/toolkit/src/query/core/buildMiddleware/polling.ts b/packages/toolkit/src/query/core/buildMiddleware/polling.ts index df7865901c..839a3a5243 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/polling.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/polling.ts @@ -14,6 +14,7 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ queryThunk, api, refetchQuery, + internalState, }) => { const currentPolls: QueryStateMeta<{ nextPollTimestamp: number @@ -21,30 +22,26 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ pollingInterval: number }> = {} - const handler: ApiMiddlewareInternalHandler = ( - action, - mwApi, - internalState - ) => { + const handler: ApiMiddlewareInternalHandler = (action, mwApi) => { if ( api.internalActions.updateSubscriptionOptions.match(action) || api.internalActions.unsubscribeQueryResult.match(action) ) { - updatePollingInterval(action.payload, mwApi, internalState) + updatePollingInterval(action.payload, mwApi) } if ( queryThunk.pending.match(action) || (queryThunk.rejected.match(action) && action.meta.condition) ) { - updatePollingInterval(action.meta.arg, mwApi, internalState) + updatePollingInterval(action.meta.arg, mwApi) } if ( queryThunk.fulfilled.match(action) || (queryThunk.rejected.match(action) && !action.meta.condition) ) { - startNextPoll(action.meta.arg, mwApi, internalState) + startNextPoll(action.meta.arg, mwApi) } if (api.util.resetApiState.match(action)) { @@ -54,8 +51,7 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ function startNextPoll( { queryCacheKey }: QuerySubstateIdentifier, - api: SubMiddlewareApi, - internalState: InternalMiddlewareState + api: SubMiddlewareApi ) { const state = api.getState()[reducerPath] const querySubState = state.queries[queryCacheKey] @@ -90,8 +86,7 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ function updatePollingInterval( { queryCacheKey }: QuerySubstateIdentifier, - api: SubMiddlewareApi, - internalState: InternalMiddlewareState + api: SubMiddlewareApi ) { const state = api.getState()[reducerPath] const querySubState = state.queries[queryCacheKey] @@ -112,7 +107,7 @@ export const buildPollingHandler: InternalHandlerBuilder = ({ const nextPollTimestamp = Date.now() + lowestPollingInterval if (!currentPoll || nextPollTimestamp < currentPoll.nextPollTimestamp) { - startNextPoll({ queryCacheKey }, api, internalState) + startNextPoll({ queryCacheKey }, api) } } diff --git a/packages/toolkit/src/query/core/buildMiddleware/types.ts b/packages/toolkit/src/query/core/buildMiddleware/types.ts index 6262caa421..20e23a4ac8 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/types.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/types.ts @@ -52,6 +52,7 @@ export type SubMiddlewareApi = MiddlewareAPI< export interface BuildSubMiddlewareInput extends BuildMiddlewareInput { + internalState: InternalMiddlewareState refetchQuery( querySubState: Exclude< QuerySubState, @@ -73,7 +74,6 @@ export type SubMiddlewareBuilder = ( export type ApiMiddlewareInternalHandler = ( action: AnyAction, 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 a0d9941bc3..4caa97c77d 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/windowEventHandling.ts @@ -4,7 +4,6 @@ import { onFocus, onOnline } from '../setupListeners' import type { ApiMiddlewareInternalHandler, InternalHandlerBuilder, - InternalMiddlewareState, SubMiddlewareApi, } from './types' @@ -13,26 +12,22 @@ export const buildWindowEventHandler: InternalHandlerBuilder = ({ context, api, refetchQuery, + internalState, }) => { const { removeQueryResult } = api.internalActions - const handler: ApiMiddlewareInternalHandler = ( - action, - mwApi, - internalState - ) => { + const handler: ApiMiddlewareInternalHandler = (action, mwApi) => { if (onFocus.match(action)) { - refetchValidQueries(mwApi, 'refetchOnFocus', internalState) + refetchValidQueries(mwApi, 'refetchOnFocus') } if (onOnline.match(action)) { - refetchValidQueries(mwApi, 'refetchOnReconnect', internalState) + refetchValidQueries(mwApi, 'refetchOnReconnect') } } function refetchValidQueries( api: SubMiddlewareApi, - type: 'refetchOnFocus' | 'refetchOnReconnect', - internalState: InternalMiddlewareState + type: 'refetchOnFocus' | 'refetchOnReconnect' ) { const state = api.getState()[reducerPath] const queries = state.queries From 8d506bad5a9c795ced04c64774c8cb58c955acfc Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 17 Oct 2022 22:24:40 -0400 Subject: [PATCH 2/4] Remove leftover middleware perf debugging code --- .../toolkit/src/query/tests/cleanup.test.tsx | 57 +------------------ 1 file changed, 3 insertions(+), 54 deletions(-) diff --git a/packages/toolkit/src/query/tests/cleanup.test.tsx b/packages/toolkit/src/query/tests/cleanup.test.tsx index 0c3e412a66..a9cf2b94c2 100644 --- a/packages/toolkit/src/query/tests/cleanup.test.tsx +++ b/packages/toolkit/src/query/tests/cleanup.test.tsx @@ -162,21 +162,6 @@ test('Minimizes the number of subscription dispatches when multiple components a 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)'] @@ -199,50 +184,19 @@ test('Minimizes the number of subscription dispatches when multiple components a return <>{listItems} } - const start = Date.now() - - render( - // - , - // , - { - wrapper: storeRef.wrapper, - } - ) - - const afterRender = Date.now() + render(, { + wrapper: storeRef.wrapper, + }) 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) @@ -252,9 +206,4 @@ test('Minimizes the number of subscription dispatches when multiple components a '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) From 1696f7a05dbf4fd5264c92f73f6d1f968d9675ee Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 17 Oct 2022 22:36:08 -0400 Subject: [PATCH 3/4] Move subscription tracking logic over to batchActions file --- .../core/buildMiddleware/batchActions.ts | 64 ++++++++++++++++++- packages/toolkit/src/query/core/buildSlice.ts | 57 ----------------- 2 files changed, 62 insertions(+), 59 deletions(-) diff --git a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts index 72e08ee289..1eb2f40e12 100644 --- a/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts +++ b/packages/toolkit/src/query/core/buildMiddleware/batchActions.ts @@ -1,7 +1,12 @@ import type { QueryThunk, RejectedAction } from '../buildThunks' import type { InternalHandlerBuilder } from './types' -import type { SubscriptionState } from '../apiState' +import type { + SubscriptionState, + QuerySubstateIdentifier, + Subscribers, +} from '../apiState' import { produceWithPatches } from 'immer' +import { createSlice, PayloadAction, AnyAction } from '@reduxjs/toolkit' // Copied from https://github.com/feross/queue-microtask let promise: Promise @@ -19,7 +24,6 @@ const queueMicrotaskShim = export const buildBatchedActionsHandler: InternalHandlerBuilder< [actionShouldContinue: boolean, subscriptionExists: boolean] > = ({ api, queryThunk, internalState }) => { - const { actuallyMutateSubscriptions } = api.internalActions const subscriptionsPrefix = `${api.reducerPath}/subscriptions` let previousSubscriptions: SubscriptionState = @@ -27,6 +31,62 @@ export const buildBatchedActionsHandler: InternalHandlerBuilder< let dispatchQueued = false + const { updateSubscriptionOptions, unsubscribeQueryResult } = + api.internalActions + + // Actually intentionally mutate the subscriptions state used in the middleware + // This is done to speed up perf when loading many components + const actuallyMutateSubscriptions = ( + mutableState: SubscriptionState, + action: AnyAction + ) => { + if (updateSubscriptionOptions.match(action)) { + const { queryCacheKey, requestId, options } = action.payload + + if (mutableState?.[queryCacheKey]?.[requestId]) { + mutableState[queryCacheKey]![requestId] = options + } + return true + } + if (unsubscribeQueryResult.match(action)) { + const { queryCacheKey, requestId } = action.payload + if (mutableState[queryCacheKey]) { + delete mutableState[queryCacheKey]![requestId] + } + return true + } + if (api.internalActions.removeQueryResult.match(action)) { + delete mutableState[action.payload.queryCacheKey] + return true + } + if (queryThunk.pending.match(action)) { + const { + meta: { arg, requestId }, + } = action + if (arg.subscribe) { + const substate = (mutableState[arg.queryCacheKey] ??= {}) + substate[requestId] = + arg.subscriptionOptions ?? substate[requestId] ?? {} + + return true + } + } + if (queryThunk.rejected.match(action)) { + const { + meta: { condition, arg, requestId }, + } = action + if (condition && arg.subscribe) { + const substate = (mutableState[arg.queryCacheKey] ??= {}) + substate[requestId] = + arg.subscriptionOptions ?? substate[requestId] ?? {} + + return true + } + } + + return false + } + return (action, mwApi) => { if (!previousSubscriptions) { // Initialize it the first time this handler runs diff --git a/packages/toolkit/src/query/core/buildSlice.ts b/packages/toolkit/src/query/core/buildSlice.ts index d4cb0f5da9..1e50081814 100644 --- a/packages/toolkit/src/query/core/buildSlice.ts +++ b/packages/toolkit/src/query/core/buildSlice.ts @@ -412,62 +412,6 @@ export function buildSlice({ }, }) - 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 - } - if (unsubscribeQueryResult.match(action)) { - const { queryCacheKey, requestId } = action.payload - if (draft[queryCacheKey]) { - delete draft[queryCacheKey]![requestId] - } - return true - } - if (querySlice.actions.removeQueryResult.match(action)) { - delete draft[action.payload.queryCacheKey] - return true - } - 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 - } - } - 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, @@ -536,7 +480,6 @@ export function buildSlice({ /** @deprecated has been renamed to `removeMutationResult` */ unsubscribeMutationResult: mutationSlice.actions.removeMutationResult, resetApiState, - actuallyMutateSubscriptions, } return { reducer, actions } From feccd87eb5c84de75125f2fa74f2856a02ad1444 Mon Sep 17 00:00:00 2001 From: Mark Erikson Date: Mon, 17 Oct 2022 22:55:45 -0400 Subject: [PATCH 4/4] Assorted other cleanup --- packages/toolkit/src/query/core/buildThunks.ts | 2 -- packages/toolkit/src/query/react/buildHooks.ts | 2 +- packages/toolkit/src/query/tests/buildHooks.test.tsx | 10 ++++------ 3 files changed, 5 insertions(+), 9 deletions(-) diff --git a/packages/toolkit/src/query/core/buildThunks.ts b/packages/toolkit/src/query/core/buildThunks.ts index 895a9ae3c7..67291aad07 100644 --- a/packages/toolkit/src/query/core/buildThunks.ts +++ b/packages/toolkit/src/query/core/buildThunks.ts @@ -486,8 +486,6 @@ 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 d89d5f21bc..8df2eaace4 100644 --- a/packages/toolkit/src/query/react/buildHooks.ts +++ b/packages/toolkit/src/query/react/buildHooks.ts @@ -715,7 +715,7 @@ export function buildHooks({ requestId, }) ) - // console.log('Probe subscription check: ', returnedValue) + currentRenderHasSubscription = !!returnedValue } diff --git a/packages/toolkit/src/query/tests/buildHooks.test.tsx b/packages/toolkit/src/query/tests/buildHooks.test.tsx index a8122eb891..929363f4a2 100644 --- a/packages/toolkit/src/query/tests/buildHooks.test.tsx +++ b/packages/toolkit/src/query/tests/buildHooks.test.tsx @@ -89,11 +89,7 @@ let actions: AnyAction[] = [] const storeRef = setupApiStore( api, - { - // actions(state: AnyAction[] = [], action: AnyAction) { - // return [...state, action] - // }, - }, + {}, { middleware: { prepend: [listenerMiddleware.middleware], @@ -133,7 +129,9 @@ describe('hooks tests', () => { } render(, { wrapper: storeRef.wrapper }) - expect(getRenderCount()).toBe(2) // By the time this runs, the initial render will happen, and the query will start immediately running by the time we can expect this + // By the time this runs, the initial render will happen, and the query + // will start immediately running by the time we can expect this + expect(getRenderCount()).toBe(2) await waitFor(() => expect(screen.getByTestId('isFetching').textContent).toBe('false')