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

Proposal: Modifying Typescript typing to make combineReducers match the state object better #2238

Closed
MichaelTontchev opened this issue Feb 12, 2017 · 5 comments

Comments

@MichaelTontchev
Copy link

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

Feature

What is the current behavior?

Right now, if I have a state defined by

interface IState {
	navBarSelection: NavBarSelection,
	potato: number,
}

export enum NavBarSelection {
	Home,
	About
}

and define my reducers like this:

class ActionTypes {
	public static SET_SELECTED_NAV = 'SET_SELECTED_NAV';
}

const navBarReducer = (state: NavBarSelection = NavBarSelection.Home, action: any): NavBarSelection => {
	switch(action.type) {
		case ActionTypes.SET_SELECTED_NAV:
			return action.payload.selectedNav;
	}

	return state;
}

let statePropertyToReducerMap = {
	navBarSelection: navBarReducer,
	extraProperty: () => {},
};

let combinedReducers = combineReducers<IState>(statePropertyToReducerMap);

Typescript will fail to catch two potential errors:

  1. The statePropertyToReducerMap contains "extraProperty", which does not exist on the state.
  2. The statePropertyToReducerMap does not contain "potato", which does exist on the state.

Note the definitions that currently exist regarding combineReducers:

export interface ReducersMapObject {
  [key: string]: Reducer<any>;
}

and

export function combineReducers<S>(reducers: ReducersMapObject): Reducer<S>;

What is the proposed behavior?

If we modify the definition of ReducersMapObject to

export type ReducersMapObject<S> = {
	[P in keyof S]: Reducer<S[P]>;
};

Then this automatically catches our errors and we get the following two errors:

First, we get

Type '{ navBarSelection: (state: NavBarSelection, action: any) => NavBarSelection; extraProperty: () => void; }' is not assignable to type 'StatePropertyNameAndTypeAwareReducer'.
Object literal may only specify known properties, and 'extraProperty' does not exist in type 'StatePropertyNameAndTypeAwareReducer'.

and when we fix that by removing extraProperty, we get

Type '{ navBarSelection: (state: NavBarSelection, action: any) => NavBarSelection; }' is not assignable to type 'StatePropertyNameAndTypeAwareReducer'.
Property 'potato' is missing in type '{ navBarSelection: (state: NavBarSelection, action: any) => NavBarSelection; }'.

which is everything we've wanted.

Note that this change is a breaking change, since we are converting ReducersMapObject from an interface to just a type. I am not sure whether we can get the same effect with an interface, though if we want to keep the current definition of ReducersMapObject we could just add the type right underneath it like

export type StateAwareReducersMapObject<S> = {
  [P in keyof S]: Reducer<S[P]>;
}

and then modify combineReducers to be

export function combineReducers<S>(reducers: StateAwareReducersMapObject<S>): Reducer<S>;

This way, people who relied on the old ReducersMapObject can keep using it, but the TypeScript compiler will start showing them errors when their reducers do not add up to create the state.

I haven't thought through all the edge cases (what if the object passed to combineReducers breaks up its responsibilities to delegated method calls that spread their result into the object), but it seems doing something along the lines of the proposal above would go a long way to making combineReducers more type safe. Sorry if I'm discussing a well-known approach or I'm overstepping my bounds here :)

At the very least we can maybe just add the StateAwareReducersMapObject type to the type definition for people to optionally use. Could still provide a lot of benefits. (note: keyof is only available in TypeScript >= 2.1)

What are your thoughts?

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

Latest Redux

@markerikson
Copy link
Contributor

To be honest, none of us three primary maintainers have any real interaction with either of the Typescript or Flow typings. Those definitions have been worked on by others. I particularly know nothing about the nuances of those type systems, so I've avoided the discussions.

I know there's been some active issues that have involved changes. If you've got ideas for improving things, probably best to ping some of the people who have worked on them previously, collaborate, and put together changes. If there's agreement that the changes are improvements, we can merge them in.

@MichaelTontchev
Copy link
Author

Got it :)

Do you happen to know some of the relevant contributors off the top of your head? If not, I'll go through some of the commits.

@MichaelTontchev
Copy link
Author

@aikoven , @timdorr , @raineorshine , @Igorbek , @use-strict - you've helped maintain the TypeScript typings for Redux. Could you comment on the above proposal if you've got a few minutes? I'm hoping this can make Redux a bit more type safe for future users :)

@aikoven
Copy link
Collaborator

aikoven commented Feb 13, 2017

Already merged into the next branch: #2182

@timdorr timdorr closed this as completed Feb 13, 2017
@MichaelTontchev
Copy link
Author

Can't beat the experts, I see :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants