From 2fc7d5ce2f7eadb28a0e497ac5fd6375a977eb73 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Thu, 4 Sep 2025 11:39:47 +0100 Subject: [PATCH 1/2] track call order using Set, to allow differentiating between addAsyncThunk and addMatcher --- packages/toolkit/src/mapBuilders.ts | 136 ++++++++++-------- .../toolkit/src/tests/createReducer.test.ts | 24 +++- 2 files changed, 95 insertions(+), 65 deletions(-) diff --git a/packages/toolkit/src/mapBuilders.ts b/packages/toolkit/src/mapBuilders.ts index 36ca6f5840..fe648de065 100644 --- a/packages/toolkit/src/mapBuilders.ts +++ b/packages/toolkit/src/mapBuilders.ts @@ -38,6 +38,17 @@ export type TypedActionCreator = { type: Type } +interface InternalMapBuilder { + addCase( + typeOrActionCreator: string | TypedActionCreator, + reducer: CaseReducer, + ): InternalMapBuilder + addMatcher( + matcher: TypeGuard, + reducer: CaseReducer, + ): InternalMapBuilder +} + /** * A builder for an action <-> reducer map. * @@ -47,7 +58,7 @@ export interface ActionReducerMapBuilder { /** * Adds a case reducer to handle a single exact action type. * @remarks - * All calls to `builder.addCase` must come before any calls to `builder.addMatcher` or `builder.addDefaultCase`. + * All calls to `builder.addCase` must come before any calls to `builder.addAsyncThunk`, `builder.addMatcher` or `builder.addDefaultCase`. * @param actionCreator - Either a plain action type string, or an action creator generated by [`createAction`](./createAction) that can be used to determine the action type. * @param reducer - The actual case reducer function. */ @@ -70,7 +81,7 @@ export interface ActionReducerMapBuilder { /** * Adds case reducers to handle actions based on a `AsyncThunk` action creator. * @remarks - * All calls to `builder.addAsyncThunk` must come before after any calls to `builder.addCase` and before any calls to `builder.addMatcher` or `builder.addDefaultCase`. + * All calls to `builder.addAsyncThunk` must come after any calls to `builder.addCase` and before any calls to `builder.addMatcher` or `builder.addDefaultCase`. * @param asyncThunk - The async thunk action creator itself. * @param reducers - A mapping from each of the `AsyncThunk` action types to the case reducer that should handle those actions. * @example @@ -191,6 +202,13 @@ const reducer = createReducer(initialState, builder => { addDefaultCase(reducer: CaseReducer): {} } +const callOrder: Array> = [ + 'addCase', + 'addAsyncThunk', + 'addMatcher', + 'addDefaultCase', +] + export function executeReducerBuilderCallback( builderCallback: (builder: ActionReducerMapBuilder) => void, ): [ @@ -201,94 +219,90 @@ export function executeReducerBuilderCallback( const actionsMap: CaseReducers = {} const actionMatchers: ActionMatcherDescriptionCollection = [] let defaultCaseReducer: CaseReducer | undefined - const builder = { + const called = new Set>() + function ensureCallOrder( + method: keyof ActionReducerMapBuilder, + allowCallTwice = true, + ) { + if (called.has(method) && !allowCallTwice) { + throw new Error(`\`builder.${method}\` can only be called once`) + } + for (const otherMethod of callOrder.slice(callOrder.indexOf(method) + 1)) { + if (called.has(otherMethod)) { + throw new Error( + `\`builder.${method}\` should only be called before calling \`builder.${otherMethod}\``, + ) + } + } + called.add(method) + } + + // builder methods without dev checks + const rawBuilder: InternalMapBuilder = { addCase( typeOrActionCreator: string | TypedActionCreator, - reducer: CaseReducer, + reducer: CaseReducer, ) { - if (process.env.NODE_ENV !== 'production') { - /* - to keep the definition by the user in line with actual behavior, - we enforce `addCase` to always be called before calling `addMatcher` - as matching cases take precedence over matchers - */ - if (actionMatchers.length > 0) { - throw new Error( - '`builder.addCase` should only be called before calling `builder.addMatcher`', - ) - } - if (defaultCaseReducer) { - throw new Error( - '`builder.addCase` should only be called before calling `builder.addDefaultCase`', - ) - } - } const type = typeof typeOrActionCreator === 'string' ? typeOrActionCreator : typeOrActionCreator.type if (!type) { - throw new Error( - '`builder.addCase` cannot be called with an empty action type', - ) + throw new Error('A reducer cannot be defined for an empty action type') } if (type in actionsMap) { throw new Error( - '`builder.addCase` cannot be called with two reducers for the same action type ' + - `'${type}'`, + `A reducer already exists for the action type '${type}'`, ) } actionsMap[type] = reducer - return builder + return rawBuilder + }, + addMatcher(matcher: TypeGuard, reducer) { + actionMatchers.push({ matcher, reducer }) + return rawBuilder }, - addAsyncThunk< - Returned, - ThunkArg, - ThunkApiConfig extends AsyncThunkConfig = {}, - >( - asyncThunk: AsyncThunk, - reducers: AsyncThunkReducers, + } + const builder: ActionReducerMapBuilder = { + addCase( + typeOrActionCreator: string | TypedActionCreator, + reducer: CaseReducer, ) { if (process.env.NODE_ENV !== 'production') { - // since this uses both action cases and matchers, we can't enforce the order in runtime other than checking for default case - if (defaultCaseReducer) { - throw new Error( - '`builder.addAsyncThunk` should only be called before calling `builder.addDefaultCase`', - ) - } + /* + to keep the definition by the user in line with actual behavior, + we enforce `addCase` to always be called before calling `addMatcher` + as matching cases take precedence over matchers + */ + ensureCallOrder('addCase') + } + rawBuilder.addCase(typeOrActionCreator, reducer) + return builder + }, + addAsyncThunk(asyncThunk, reducers) { + if (process.env.NODE_ENV !== 'production') { + ensureCallOrder('addAsyncThunk') } if (reducers.pending) - actionsMap[asyncThunk.pending.type] = reducers.pending + rawBuilder.addCase(asyncThunk.pending, reducers.pending) if (reducers.rejected) - actionsMap[asyncThunk.rejected.type] = reducers.rejected + rawBuilder.addCase(asyncThunk.rejected, reducers.rejected) if (reducers.fulfilled) - actionsMap[asyncThunk.fulfilled.type] = reducers.fulfilled + rawBuilder.addCase(asyncThunk.fulfilled, reducers.fulfilled) if (reducers.settled) - actionMatchers.push({ - matcher: asyncThunk.settled, - reducer: reducers.settled, - }) + rawBuilder.addMatcher(asyncThunk.settled, reducers.settled) return builder }, - addMatcher( - matcher: TypeGuard, - reducer: CaseReducer, - ) { + addMatcher(matcher: TypeGuard, reducer) { if (process.env.NODE_ENV !== 'production') { - if (defaultCaseReducer) { - throw new Error( - '`builder.addMatcher` should only be called before calling `builder.addDefaultCase`', - ) - } + ensureCallOrder('addMatcher') } - actionMatchers.push({ matcher, reducer }) + rawBuilder.addMatcher(matcher, reducer) return builder }, - addDefaultCase(reducer: CaseReducer) { + addDefaultCase(reducer) { if (process.env.NODE_ENV !== 'production') { - if (defaultCaseReducer) { - throw new Error('`builder.addDefaultCase` can only be called once') - } + ensureCallOrder('addDefaultCase', false) } defaultCaseReducer = reducer return builder diff --git a/packages/toolkit/src/tests/createReducer.test.ts b/packages/toolkit/src/tests/createReducer.test.ts index 114096c069..808bfbcdb3 100644 --- a/packages/toolkit/src/tests/createReducer.test.ts +++ b/packages/toolkit/src/tests/createReducer.test.ts @@ -353,7 +353,7 @@ describe('createReducer', () => { .addCase(decrement, (state, action) => state - action.payload) }) }).toThrowErrorMatchingInlineSnapshot( - `[Error: \`builder.addCase\` cannot be called with two reducers for the same action type 'increment']`, + `[Error: A reducer already exists for the action type 'increment']`, ) expect(() => { createReducer(0, (builder) => { @@ -363,7 +363,7 @@ describe('createReducer', () => { .addCase(decrement, (state, action) => state - action.payload) }) }).toThrowErrorMatchingInlineSnapshot( - `[Error: \`builder.addCase\` cannot be called with two reducers for the same action type 'increment']`, + `[Error: A reducer already exists for the action type 'increment']`, ) }) @@ -381,7 +381,7 @@ describe('createReducer', () => { ) }) }).toThrowErrorMatchingInlineSnapshot( - `[Error: \`builder.addCase\` cannot be called with an empty action type]`, + `[Error: A reducer cannot be defined for an empty action type]`, ) }) }) @@ -498,7 +498,23 @@ describe('createReducer', () => { stringActions: 0, }) }) - test('calling addCase, addMatcher and addDefaultCase in a nonsensical order should result in an error in development mode', () => { + test('calling addCase, addAsyncThunk, addMatcher and addDefaultCase in a nonsensical order should result in an error in development mode', () => { + expect(() => + createReducer(initialState, (builder: any) => + builder + .addAsyncThunk(addTodoThunk, {}) + .addCase(incrementBy, () => {}), + ), + ).toThrowErrorMatchingInlineSnapshot(`[Error: \`builder.addCase\` should only be called before calling \`builder.addAsyncThunk\`]`) + expect(() => + createReducer(initialState, (builder: any) => + builder + .addMatcher(numberActionMatcher, () => {}) + .addAsyncThunk(addTodoThunk, {}), + ), + ).toThrowErrorMatchingInlineSnapshot( + `[Error: \`builder.addAsyncThunk\` should only be called before calling \`builder.addMatcher\`]`, + ) expect(() => createReducer(initialState, (builder: any) => builder From d2792f3436593c04764190aacf50b0d500c2f073 Mon Sep 17 00:00:00 2001 From: Ben Durrant Date: Thu, 4 Sep 2025 16:29:00 +0100 Subject: [PATCH 2/2] update snapshot and add test for conflicting reducers --- .../toolkit/src/tests/createReducer.test.ts | 25 ++++++++++++++++++- .../toolkit/src/tests/createSlice.test.ts | 2 +- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/packages/toolkit/src/tests/createReducer.test.ts b/packages/toolkit/src/tests/createReducer.test.ts index 808bfbcdb3..e0f2f5cf05 100644 --- a/packages/toolkit/src/tests/createReducer.test.ts +++ b/packages/toolkit/src/tests/createReducer.test.ts @@ -505,7 +505,9 @@ describe('createReducer', () => { .addAsyncThunk(addTodoThunk, {}) .addCase(incrementBy, () => {}), ), - ).toThrowErrorMatchingInlineSnapshot(`[Error: \`builder.addCase\` should only be called before calling \`builder.addAsyncThunk\`]`) + ).toThrowErrorMatchingInlineSnapshot( + `[Error: \`builder.addCase\` should only be called before calling \`builder.addAsyncThunk\`]`, + ) expect(() => createReducer(initialState, (builder: any) => builder @@ -598,6 +600,27 @@ describe('createReducer', () => { `[Error: \`builder.addAsyncThunk\` should only be called before calling \`builder.addDefaultCase\`]`, ) }) + test('calling addAsyncThunk after addMatcher should result in an error in development mode', () => { + expect(() => + createReducer(initialState, (builder: any) => + builder + .addMatcher( + () => true, + () => {}, + ) + .addAsyncThunk(addTodoThunk, {}), + ), + ).toThrowErrorMatchingInlineSnapshot(`[Error: \`builder.addAsyncThunk\` should only be called before calling \`builder.addMatcher\`]`) + }) + test('calling addAsyncThunk for an action already covered by addCase should result in an error in development mode', () => { + expect(() => + createReducer(initialState, (builder: any) => + builder + .addCase(addTodoThunk.pending, () => {}) + .addAsyncThunk(addTodoThunk, { pending() {} }), + ), + ).toThrowErrorMatchingInlineSnapshot(`[Error: A reducer already exists for the action type 'todos/add/pending']`) + }) }) }) diff --git a/packages/toolkit/src/tests/createSlice.test.ts b/packages/toolkit/src/tests/createSlice.test.ts index 09f726c113..537f82d9df 100644 --- a/packages/toolkit/src/tests/createSlice.test.ts +++ b/packages/toolkit/src/tests/createSlice.test.ts @@ -262,7 +262,7 @@ describe('createSlice', () => { }) slice.reducer(undefined, { type: 'unrelated' }) }).toThrowErrorMatchingInlineSnapshot( - `[Error: \`builder.addCase\` cannot be called with two reducers for the same action type 'increment']`, + `[Error: A reducer already exists for the action type 'increment']`, ) })