Skip to content
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.

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

typings for combineReducers infers overly action type from reducer map #2979

Closed
insidewhy opened this issue May 6, 2018 · 5 comments
Closed

Comments

@insidewhy
Copy link

insidewhy commented May 6, 2018

Do you want to request a feature or report a bug?

Bug

What is the current behavior?

The following minimal reproduction shows the reducer is incorrectly typed to accept AnyAction

interface A1 { type: 'A1' }
interface A2 { type: 'A2' }
type Action = A1 | A2
const reducerMap = {
  s1: (s: number, a: Action) => 1,
  s2: (s: number, a: Action) => 2,
}
const reducer = combineReducers(reducerMap)
// the reducer call works even though it should only accept actions A1 and A2.
reducer({ s1: 0, s2: 0 }, { type: 'blah' })

What is the expected behavior?

The action types should be inferred from the action. If the typings are changed to this it works:

type FunctionReturnType<T> = T extends (...args: any[]) => infer R ? R : never

type CombinedState<T extends object> = { [K in keyof T]: FunctionReturnType<T[K]> }

type ActionFromReducer<T extends object> = T[keyof T] extends (
  state: any,
  action: infer A,
  ...args: any[]
) => any
  ? A
  : never

type ReducerReturnValue<T extends object> = (
  state: CombinedState<T>,
  action: ActionFromReducer<T>,
) => CombinedState<T>

export function combineReducers<T extends object>(reducers: T): ReducerReturnValue<T>

But this relies on conditional types which were added in typescript 2.8.

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

Redux 4.0.0

@timdorr
Copy link
Member

timdorr commented May 6, 2018

We're using Typescript 2.8. Feel free to submit a PR.

@insidewhy
Copy link
Author

insidewhy commented May 6, 2018

It would prevent anyone using earlier releases of TypeScript from using the official redux typings. It would also break for those explicitly specifying the state/action type parameters. You could argue that with this level of inference there's no longer a need to use those parameters, but some might be using them to explicitly verify their reducer map functions. I like it this way at least ;)

@aikoven
Copy link
Collaborator

aikoven commented May 8, 2018

@ohjames Current typings support restricting action type, see the test.

@bloadvenro
Copy link

bloadvenro commented Nov 6, 2018

@ohjames thank you! Your solution was very helpful. I slightly modified it:

type ReducersMapObject = { [index: string]: Reducer<any, any> };

type CombinedState<T extends ReducersMapObject> = { [K in keyof T]: ReturnType<T[K]> };

type ObjectValues<T> = T[keyof T];

type CombinedActions<T extends ReducersMapObject> = ObjectValues<
  { [K in keyof T]: Parameters<T[K]>[1] /* <- type of action parameter */ }
>;

type CombinedReducer<T extends ReducersMapObject> = Reducer<
  CombinedState<T>,
  CombinedActions<T>
>;

CombinedReducer is the return type of combineReducers function. Quick testing shows that everything works fine.

@Shakeskeyboarde
Copy link
Contributor

I’ve opened a pull request for this issue, #3484.

timdorr pushed a commit that referenced this issue Aug 10, 2019
* Add type overload for combineReducers which strictly infers state shape and actions from the reducers object map.

* Fixed some typos.

* Please don't change version numbers in a PR

* Typescript 2.8 default type fixes.
@timdorr timdorr closed this as completed Dec 24, 2019
@reduxjs reduxjs deleted a comment from sajumani Jul 16, 2021
@reduxjs reduxjs locked as resolved and limited conversation to collaborators Jul 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants