Skip to content

Include case reducers in createSlice result #209

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 9 commits into from
Oct 11, 2019
Merged

Include case reducers in createSlice result #209

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
67d96f7
Fix missing name field in type test
markerikson Oct 7, 2019
a389eef
Include provided case reducers in createSlice output
markerikson Oct 7, 2019
36e966f
Add type tests for correct case reducer export inference
markerikson Oct 9, 2019
dd7f19b
Rewrite caseReducers types for correct inference of state + action
markerikson Oct 9, 2019
d9cf216
Add type test for reducers with prepare callback
markerikson Oct 10, 2019
731c744
Fix type inference for actions from reducers w/ prepare callbacks
markerikson Oct 10, 2019
184e5ab
Clean up type names and usages
markerikson Oct 10, 2019
a34bf93
Use a generic State type in ActionForReducer
markerikson Oct 10, 2019
8f6add0
Re-switch Slice generics order
markerikson Oct 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/createSlice.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ describe('createSlice', () => {
})

describe('when passing slice', () => {
const { actions, reducer } = createSlice({
const { actions, reducer, caseReducers } = createSlice({
reducers: {
increment: state => state + 1
},
Expand All @@ -57,6 +57,12 @@ describe('createSlice', () => {
it('should return the correct value from reducer', () => {
expect(reducer(undefined, actions.increment())).toEqual(1)
})

it('should include the generated case reducers', () => {
expect(caseReducers).toBeTruthy()
expect(caseReducers.increment).toBeTruthy()
expect(typeof caseReducers.increment).toBe('function')
})
})

describe('when mutating state object', () => {
Expand Down
119 changes: 77 additions & 42 deletions src/createSlice.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,9 @@ export type SliceActionCreator<P> = PayloadActionCreator<P>

export interface Slice<
State = any,
ActionCreators extends { [key: string]: any } = { [key: string]: any }
CaseReducers extends SliceCaseReducerDefinitions<State, PayloadActions> = {
[key: string]: any
}
> {
/**
* The slice name.
Expand All @@ -34,15 +36,20 @@ export interface Slice<
* Action creators for the types of actions that are handled by the slice
* reducer.
*/
actions: ActionCreators
actions: CaseReducerActions<CaseReducers>

caseReducers: SliceDefinedCaseReducers<CaseReducers, State>
}

/**
* Options for `createSlice()`.
*/
export interface CreateSliceOptions<
State = any,
CR extends SliceCaseReducers<State, any> = SliceCaseReducers<State, any>
CR extends SliceCaseReducerDefinitions<
State,
any
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use unknown instead of any (requires TypeScript 3)

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This one was there already, but yeah, we can try that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Adding unknown in the places you're suggesting just broke all the existing type tests.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the error? It's possible the type tests are running on an old version of TS, or that they need type guards.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

redux-starter-kit/src/createSlice.ts (49, 5) Type 'unknown' does not satisfy the constraint 'Record<string, WithPayload<any, Action<string>>>'.

And no, we're on TS 3.4.3 right now.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m not sure why that doesn’t match, but if the return type uses any anyway, maybe this can be left alone for now

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can pretty easily change all those parts in the extends statement from any to unknown without consequence.
But doing it for the default value will in some places be a breaking change for people who didn't specify a type manually and where the type could not be inferred.
Arguably, this is how they shouldn't be using TS in the first place if they want it for type-safety, but it definitely is used that way (see #165), so maybe that should be done with caution in a different PR.

> = SliceCaseReducerDefinitions<State, any>
> {
/**
* The slice's name. Used to namespace the generated action types.
Expand Down Expand Up @@ -74,23 +81,23 @@ type PayloadActions<Types extends keyof any = string> = Record<
PayloadAction
>

type EnhancedCaseReducer<State, Action extends PayloadAction> = {
type CaseReducerWithPrepare<State, Action extends PayloadAction> = {
reducer: CaseReducer<State, Action>
prepare: PrepareAction<Action['payload']>
}

type SliceCaseReducers<State, PA extends PayloadActions> = {
type SliceCaseReducerDefinitions<State, PA extends PayloadActions> = {
[ActionType in keyof PA]:
| CaseReducer<State, PA[ActionType]>
| EnhancedCaseReducer<State, PA[ActionType]>
| CaseReducerWithPrepare<State, PA[ActionType]>
}

type IfIsReducerFunctionWithoutAction<R, True, False = never> = R extends (
state: any
) => any
? True
: False
type IfIsEnhancedReducer<R, True, False = never> = R extends {
type IfIsCaseReducerWithPrepare<R, True, False = never> = R extends {
prepare: Function
}
? True
Expand All @@ -106,8 +113,21 @@ type PrepareActionForReducer<R> = R extends { prepare: infer Prepare }
? Prepare
: never

type CaseReducerActions<CaseReducers extends SliceCaseReducers<any, any>> = {
[Type in keyof CaseReducers]: IfIsEnhancedReducer<
type ActionForReducer<R, S> = R extends (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you don't really care for the State type here, I believe you can just leave it out.

type ActionForReducer<R> =
  R extends (  state: any,  action: PayloadAction<infer P> ) => any
  ? PayloadAction<P>
  : R extends {      reducer(state: any, action: PayloadAction<infer P>): any    }
  ? PayloadAction<P>
  : unknown

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, that's basically what I originally had. @nickmccurdy suggested I might as well use State instead of any for consistency, and it doesn't seem to actually hurt anything.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It doesn't hurt - it's just one more thing to carry around :)

Explanation why I think it's not necessary here:
What you are doing here is shape-matching. You're either looking for an object with a reducer method or for a method itself, both methods with at least two arguments. From that moment on you just care about the type of the second argument.
Sure, you can choose to only match if the first argument is State, but that means a second parameter to the generic, also caring about that second parameter when calling. So, essentially: more code, without any direct benefit.
So I'd just let it out. But if you choose to let it in there, don't forget to do the same check in line 120, too.

state: S,
action: PayloadAction<infer P>
) => S
? PayloadAction<P>
: R extends {
reducer(state: any, action: PayloadAction<infer P>): any
}
? PayloadAction<P>
: unknown

type CaseReducerActions<
CaseReducers extends SliceCaseReducerDefinitions<any, any>
> = {
[Type in keyof CaseReducers]: IfIsCaseReducerWithPrepare<
CaseReducers[Type],
ActionCreatorWithPreparedPayload<
PrepareActionForReducer<CaseReducers[Type]>
Expand All @@ -122,6 +142,16 @@ type CaseReducerActions<CaseReducers extends SliceCaseReducers<any, any>> = {
>
}

type SliceDefinedCaseReducers<
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as a consequence, no need for the State parameter here as well

CaseReducers extends SliceCaseReducerDefinitions<any, any>,
State = any
> = {
[Type in keyof CaseReducers]: CaseReducer<
State,
ActionForReducer<CaseReducers[Type], State>
>
}

type NoInfer<T> = [T][T extends any ? 0 : never]

type SliceCaseReducersCheck<S, ACR> = {
Expand All @@ -134,9 +164,9 @@ type SliceCaseReducersCheck<S, ACR> = {
: {}
}

type RestrictEnhancedReducersToMatchReducerAndPrepare<
type RestrictCaseReducerDefinitionsToMatchReducerAndPrepare<
S,
CR extends SliceCaseReducers<S, any>
CR extends SliceCaseReducerDefinitions<S, any>
> = { reducers: SliceCaseReducersCheck<S, NoInfer<CR>> }

function getType(slice: string, actionKey: string): string {
Expand All @@ -153,54 +183,59 @@ function getType(slice: string, actionKey: string): string {
*/
export function createSlice<
State,
CaseReducers extends SliceCaseReducers<State, any>
CaseReducers extends SliceCaseReducerDefinitions<State, any>
>(
options: CreateSliceOptions<State, CaseReducers> &
RestrictEnhancedReducersToMatchReducerAndPrepare<State, CaseReducers>
): Slice<State, CaseReducerActions<CaseReducers>>
RestrictCaseReducerDefinitionsToMatchReducerAndPrepare<State, CaseReducers>
): Slice<State, CaseReducers>

// internal definition is a little less restrictive
export function createSlice<
State,
CaseReducers extends SliceCaseReducers<State, any>
CaseReducers extends SliceCaseReducerDefinitions<State, any>
>(
options: CreateSliceOptions<State, CaseReducers>
): Slice<State, CaseReducerActions<CaseReducers>> {
): Slice<State, CaseReducers> {
const { name, initialState } = options
if (!name) {
throw new Error('`name` is a required option for createSlice')
}
const reducers = options.reducers || {}
const extraReducers = options.extraReducers || {}
const actionKeys = Object.keys(reducers)

const reducerMap = actionKeys.reduce((map, actionKey) => {
let maybeEnhancedReducer = reducers[actionKey]
map[getType(name, actionKey)] =
typeof maybeEnhancedReducer === 'function'
? maybeEnhancedReducer
: maybeEnhancedReducer.reducer
return map
}, extraReducers)

const reducer = createReducer(initialState, reducerMap)

const actionMap = actionKeys.reduce(
(map, action) => {
let maybeEnhancedReducer = reducers[action]
const type = getType(name, action)
map[action] =
typeof maybeEnhancedReducer === 'function'
? createAction(type)
: createAction(type, maybeEnhancedReducer.prepare)
return map
},
{} as any
)
const reducerNames = Object.keys(reducers)

const sliceCaseReducersByName: Record<string, CaseReducer> = {}
const sliceCaseReducersByType: Record<string, CaseReducer> = {}
const actionCreators: Record<string, PayloadActionCreator> = {}

reducerNames.forEach(reducerName => {
const maybeReducerWithPrepare = reducers[reducerName]
const type = getType(name, reducerName)

let caseReducer: CaseReducer<State, any>
let prepareCallback: PrepareAction<any> | undefined
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment


if (typeof maybeReducerWithPrepare === 'function') {
caseReducer = maybeReducerWithPrepare
} else {
caseReducer = maybeReducerWithPrepare.reducer
prepareCallback = maybeReducerWithPrepare.prepare
}

sliceCaseReducersByName[reducerName] = caseReducer
sliceCaseReducersByType[type] = caseReducer
actionCreators[reducerName] = prepareCallback
? createAction(type, prepareCallback)
: createAction(type)
})

const finalCaseReducers = { ...extraReducers, ...sliceCaseReducersByType }
const reducer = createReducer(initialState, finalCaseReducers)

return {
name,
reducer,
actions: actionMap
actions: actionCreators as any,
caseReducers: sliceCaseReducersByName as any
}
}
52 changes: 52 additions & 0 deletions type-tests/files/createSlice.typetest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,57 @@ function expectType<T>(t: T) {
expectType<string>(counter.actions.concatMetaStrLen('test').meta)
}

/*
* Test: returned case reducer has the correct type
*/
{
const counter = createSlice({
name: 'counter',
initialState: 0,
reducers: {
increment(state, action: PayloadAction<number>) {
return state + action.payload
},
decrement: {
reducer(state, action: PayloadAction<number>) {
return state - action.payload
},
prepare(amount: number) {
return { payload: amount }
}
}
}
})

// Should match positively
expectType<(state: number, action: PayloadAction<number>) => number | void>(
counter.caseReducers.increment
)

// Should match positively for reducers with prepare callback
expectType<(state: number, action: PayloadAction<number>) => number | void>(
counter.caseReducers.decrement
)

// Should not mismatch the payload if it's a simple reducer
// typings:expect-error
expectType<(state: number, action: PayloadAction<string>) => number | void>(
counter.caseReducers.increment
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these two additional tests should be failing, but currently don't

  // typings:expect-error
  expectType<(state: number, action: PayloadAction<string>) => number | void>(
    counter.caseReducers.increment
  )

  // typings:expect-error
  expectType<(state: number, action: PayloadAction<string>) => number | void>(
    counter.caseReducers.someThingNonExistant
  )

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! These were a huge help!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd add one non-error-check for decrement here, as currently you're just testing for expect-errors on that one.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

something like this one.

 // Should match positively for reducers with prepare callback
 expectType<(state: number, action: PayloadAction<number>) => number | void>(
   counter.caseReducers.decrement
 )


// Should not mismatch the payload if it's a reducer with a prepare callback
// typings:expect-error
expectType<(state: number, action: PayloadAction<string>) => number | void>(
counter.caseReducers.decrement
)

// Should not include entries that don't exist
// typings:expect-error
expectType<(state: number, action: PayloadAction<string>) => number | void>(
counter.caseReducers.someThingNonExistant
)
}

/*
* Test: prepared payload does not match action payload - should cause an error.
*/
Expand Down Expand Up @@ -180,6 +231,7 @@ function expectType<T>(t: T) {
}

const mySlice = createSlice({
name: 'name',
initialState,
reducers: {
setName: (state, action) => {
Expand Down