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.

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

Revamp TypeScript typing with more type safety #2563

Merged
merged 5 commits into from
Oct 6, 2017
Merged

Revamp TypeScript typing with more type safety #2563

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0dd93d1
Revamp TypeScript typing with more type safety
pelotom Sep 11, 2017
299b0b7
Provide a default action type for combineReducers
pelotom Sep 12, 2017
a76e708
Change state default types to any
pelotom Sep 12, 2017
813fbff
Don't parameterize Dispatch with a state type
pelotom Sep 12, 2017
4f5c320
Remove docstring about excised type parameter
pelotom Sep 12, 2017
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
75 changes: 44 additions & 31 deletions index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@

/**
* An *action* is a plain object that represents an intention to change the
* state. Actions are the only way to get data into the store. Any data,
Expand All @@ -12,12 +13,13 @@
* Other than `type`, the structure of an action object is really up to you.
* If you're interested, check out Flux Standard Action for recommendations on
* how actions should be constructed.
*
* @template T the type of the action's `type` tag.
*/
export interface Action {
type: any;
export interface Action<T = any> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Am I right that the intended use case for the type argument is using string literal types?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes

type: T;
}


/* reducers */

/**
Expand All @@ -41,15 +43,18 @@ export interface Action {
*
* *Do not put API calls into reducers.*
*
* @template S State object type.
* @template S The type of state consumed and produced by this reducer.
* @template A The type of actions the reducer can potentially respond to.
*/
export type Reducer<S> = <A extends Action>(state: S | undefined, action: A) => S;
export type Reducer<S = {}, A extends Action = Action> = (state: S | undefined, action: A) => S;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Still, do we really want to have S default to {}? Why?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was trying to keep this PR as backwards-compatible as possible, but if that's not important I'm more than happy to remove it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sorry, I don't get how it aids backward compatibility. Previous Reducer type did have S as a required parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm, I appear to have been confused when I made this comment. I was thinking of the state parameter to the Dispatch type, which is completely unnecessary but I was keeping around for backwards compatibility.

The argument for defaulting S here is the same as for ReducersMapObject and Store below... so that we can declare type constraints like R extends Reducer when we don't care about the type arguments.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Thanks, this makes sense. The only thing that bothers me: if the state type of reducer includes null, that reducer won't be assignable to Reducer (without parameters).

A very simplified example:

type Reducer<S = {}> = (state: S | undefined) => S;

const r: Reducer = (state: null | undefined = null): null => {
  return state;
}

Gives

Type '(state?: null | undefined) => null' is not assignable to type 'Reducer<{}>'.
  Types of parameters 'state' and 'state' are incompatible.
    Type '{} | undefined' is not assignable to type 'null | undefined'.
      Type '{}' is not assignable to type 'null | undefined'.
        Type '{}' is not assignable to type 'null'.

Works fine after setting S = {} | null.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're right, except that undefined for the state is prohibited by Redux. Yet I'm not sure if we gain any type safety by excluding it.

As far as I know, {} | null | undefined is the same as any.

Copy link

Choose a reason for hiding this comment

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

Remember though, undefined is prohibited to return from a reducer but not as argument, which is passed with the initial call.

Copy link
Contributor Author

@pelotom pelotom Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

It's not quite the same, because for example

declare x: {} | null | undefined
x.foo // this is a type error
declare y: any
y.foo // this is fine

any isn't really a type, it's more like a directive not to perform type checking. I'd say {} | null is the most "correct" thing to use here, but a lot of people tend to use any in situations like this because it's easy on the eyes. I could go either way.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remember though, undefined is prohibited to return from a reducer but not as argument, which is passed with the initial call.

The definition covers this since the state argument is of type S | undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's not quite the same

You are right, I used the wrong word. They are equivalent in terms of assignability.


/**
* Object whose values correspond to different reducer functions.
*
* @template A The type of actions the reducers can potentially respond to.
*/
export type ReducersMapObject<S> = {
[K in keyof S]: Reducer<S[K]>;
export type ReducersMapObject<S = {}, A extends Action = Action> = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Default for S here is unclear for me too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Having defaults means you can just reference ReducersMapObject if you don't care about the types involved. That tends to happen if you have a generic type parameter T extends ReducersMapObject for example.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes some sense. But wouldn't ReducersMapObject<{}> be the same as just {}? Therefore T extends {} only means that T is not null or undefined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You raise a good point and I think it means I chose the wrong default... instead it should be { [k: string]: any }. That way ReducersMapObject should be equivalent to { [k: string]: Reducer<any> }?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I think that should work.

[K in keyof S]: Reducer<S[K], A>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already done on the next branch: https://github.com/reactjs/redux/blob/next/index.d.ts#L52

This PR should probably target the next.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll do that.

}

/**
Expand All @@ -70,7 +75,7 @@ export type ReducersMapObject<S> = {
* @returns A reducer function that invokes every reducer inside the passed
* object, and builds a state object with the same shape.
*/
export function combineReducers<S>(reducers: ReducersMapObject<S>): Reducer<S>;
export function combineReducers<S, A extends Action>(reducers: ReducersMapObject<S, A>): Reducer<S, A>;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add a default type for A

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍



/* store */
Expand All @@ -92,9 +97,12 @@ export function combineReducers<S>(reducers: ReducersMapObject<S>): Reducer<S>;
* function to handle async actions in addition to actions. Middleware may
* transform, delay, ignore, or otherwise interpret actions or async actions
* before passing them to the next middleware.
*
* @template S unused, here only for backwards compatibility.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should put more thought into this. These changes are already breaking, so why should we bother about compatibility here?

The S type parameter brought much more misconception and inconvenience than benefit. It was initially added to allow us to strongly type-check dispatching thunks: as you can see here, we augment the Dispatch interface to add a new signature for thunks that uses S.

But in real-world it appeared that we rarely use dispatch as a method of the Store, where it has a fixed state type, but rather as an injected function (like in react-redux), where we have to declare its type manually:

const mapDispatchToProps = (dispatch: Dispatch<MyState>) => ...

For users who don't use redux-thunk this led to annoying need to write Dispatch<any> instead of just Dispatch.

We could just remove S altogether to simplify things; redux-thunk users would still be able to get partial type safety by declaring their state type in ThunkAction type parameters. The one major drawback of this is that it would make it much harder to migrate to the new typings.

Otherwise we should make this docstring point to the use case of S.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I said I'm happy to remove the state parameter and I definitely think this should be done at some point... If you think this PR is the right place for it I'll remove it.

* @template D the type of things (actions or otherwise) which may be dispatched.
*/
export interface Dispatch<S> {
<A extends Action>(action: A): A;
export interface Dispatch<S = any, D = Action> {
<A extends D>(action: A): A;
}

/**
Expand All @@ -109,9 +117,11 @@ export interface Unsubscribe {
* There should only be a single store in a Redux app, as the composition
* happens on the reducer level.
*
* @template S State object type.
* @template S The type of state held by this store.
* @template A the type of actions which may be dispatched by this store.
* @template N The type of non-actions which may be dispatched by this store.
*/
export interface Store<S> {
export interface Store<S = {}, A extends Action = Action, N = never> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Again, the default for S is unclear.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See previous comment... if that doesn't sway you, I'll remove the default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Dispatching non-actions is handled by augmenting the Dispatch interface. There's a difference between that and adding another type to the union: for example, when dispatching a thunk, dispatch returns the return value of the thunk, not the thunk itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So are you advocating removal of the N parameter?

Copy link

Choose a reason for hiding this comment

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

Please don't. Augmenting Dispatch for things like redux-thunk is broken, see reduxjs/redux-thunk#82. Still no one gave a solution how typing middlewares can work when redux-thunk has augmented Dispatch.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree that module augmentation is more of a workaround rather than an ideal solution.

Still, it gives us something that we can't achieve with just extending the type of dispatch argument: it allows us to set up a mapping from input type to output type of dispatch calls, which is different from one middleware to another.
Having just Dispatch<Action | ThunkAction> is incorrect, because it will work as if dispatch(thunk) returned the thunk itself.

Also, from what I see in the issue, the error comes from the definition of Middleware, and not from the module augmentation.

Considering the ideal solution: adding dispatch signatures happens during store enhancement by applyMiddleware(...). We could try to come up with smarter typings for StoreEnhancer and Middleware to get signature augmentation without module augmentation. Back then we didn't succeed with it, but now TS is much more powerful.

#1526
#1648

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the Dispatch type should still take 2 type parameters, but one is the input ("action") type, and one is the output type?

Copy link

Choose a reason for hiding this comment

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

Maybe the Dispatch type should still take 2 type parameters, but one is the input ("action") type, and one is the output type?

Sounds good to me.

Copy link

@unstubbable unstubbable Sep 12, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

After thinking a bit more about it, I don't think that improves it. Using a union type for the Action and a union type for the return value does not really help here (e.g. with redux-thunk). What you need instead is two different signatures (overloaded functions). I'm thinking the Action shouldn't be the type param (for Store and Middleware) but instead the whole Dispatch should be.

/**
* Dispatches an action. It is the only way to trigger a state change.
*
Expand All @@ -138,7 +148,7 @@ export interface Store<S> {
* Note that, if you use a custom middleware, it may wrap `dispatch()` to
* return something else (for example, a Promise you can await).
*/
dispatch: Dispatch<S>;
dispatch: Dispatch<any, A | N>;

/**
* Reads the state tree managed by the store.
Expand Down Expand Up @@ -182,7 +192,7 @@ export interface Store<S> {
*
* @param nextReducer The reducer for the store to use instead.
*/
replaceReducer(nextReducer: Reducer<S>): void;
replaceReducer(nextReducer: Reducer<S, A>): void;
}

/**
Expand All @@ -191,11 +201,13 @@ export interface Store<S> {
* `createStore(reducer, preloadedState)` exported from the Redux package, from
* store creators that are returned from the store enhancers.
*
* @template S State object type.
* @template S The type of state to be held by the store.
* @template A The type of actions which may be dispatched.
* @template D The type of all things which may be dispatched.
*/
export interface StoreCreator {
<S>(reducer: Reducer<S>, enhancer?: StoreEnhancer<S>): Store<S>;
<S>(reducer: Reducer<S>, preloadedState: S, enhancer?: StoreEnhancer<S>): Store<S>;
<S, A extends Action, N>(reducer: Reducer<S, A>, enhancer?: StoreEnhancer<N>): Store<S, A, N>;
<S, A extends Action, N>(reducer: Reducer<S, A>, preloadedState: S, enhancer?: StoreEnhancer<N>): Store<S, A, N>;
}

/**
Expand All @@ -215,10 +227,11 @@ export interface StoreCreator {
* provided by the developer tools. It is what makes time travel possible
* without the app being aware it is happening. Amusingly, the Redux
* middleware implementation is itself a store enhancer.
*
*/
export type StoreEnhancer<S> = (next: StoreEnhancerStoreCreator<S>) => StoreEnhancerStoreCreator<S>;
export type GenericStoreEnhancer = <S>(next: StoreEnhancerStoreCreator<S>) => StoreEnhancerStoreCreator<S>;
export type StoreEnhancerStoreCreator<S> = (reducer: Reducer<S>, preloadedState?: S) => Store<S>;
export type StoreEnhancer<N = never> = (next: StoreEnhancerStoreCreator<N>) => StoreEnhancerStoreCreator<N>;
export type GenericStoreEnhancer<N = never> = StoreEnhancer<N>;
export type StoreEnhancerStoreCreator<N = never> = <S = any, A extends Action = Action>(reducer: Reducer<S, A>, preloadedState?: S) => Store<S, A, N>;

/**
* Creates a Redux store that holds the state tree.
Expand Down Expand Up @@ -253,8 +266,8 @@ export const createStore: StoreCreator;

/* middleware */

export interface MiddlewareAPI<S> {
dispatch: Dispatch<S>;
export interface MiddlewareAPI<S = any, D = Action> {
dispatch: Dispatch<any, D>;
getState(): S;
}

Expand All @@ -268,7 +281,7 @@ export interface MiddlewareAPI<S> {
* asynchronous API call into a series of synchronous actions.
*/
export interface Middleware {
<S>(api: MiddlewareAPI<S>): (next: Dispatch<S>) => Dispatch<S>;
<S = any, D = Action>(api: MiddlewareAPI<S, D>): (next: Dispatch<any, D>) => Dispatch<any, D>;
}

/**
Expand Down Expand Up @@ -317,8 +330,8 @@ export interface ActionCreator<A> {
/**
* Object whose values are action creator functions.
*/
export interface ActionCreatorsMapObject {
[key: string]: ActionCreator<any>;
export interface ActionCreatorsMapObject<A = any> {
[key: string]: ActionCreator<A>;
}

/**
Expand All @@ -340,19 +353,19 @@ export interface ActionCreatorsMapObject {
* creator wrapped into the `dispatch` call. If you passed a function as
* `actionCreator`, the return value will also be a single function.
*/
export function bindActionCreators<A extends ActionCreator<any>>(actionCreator: A, dispatch: Dispatch<any>): A;
export function bindActionCreators<A, C extends ActionCreator<A>>(actionCreator: C, dispatch: Dispatch<any, A>): C;

export function bindActionCreators<
A extends ActionCreator<any>,
B extends ActionCreator<any>
>(actionCreator: A, dispatch: Dispatch<any>): B;
>(actionCreator: A, dispatch: Dispatch<any, any>): B;

export function bindActionCreators<M extends ActionCreatorsMapObject>(actionCreators: M, dispatch: Dispatch<any>): M;
export function bindActionCreators<A, M extends ActionCreatorsMapObject<A>>(actionCreators: M, dispatch: Dispatch<any, A>): M;

export function bindActionCreators<
M extends ActionCreatorsMapObject,
N extends ActionCreatorsMapObject
>(actionCreators: M, dispatch: Dispatch<any>): N;
M extends ActionCreatorsMapObject<any>,
N extends ActionCreatorsMapObject<any>
>(actionCreators: M, dispatch: Dispatch<any, any>): N;


/* compose */
Expand Down
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@
"rollup-plugin-replace": "^1.1.1",
"rollup-plugin-uglify": "^1.0.1",
"rxjs": "^5.0.0-beta.6",
"typescript": "^2.1.0",
"typescript": "^2.4.2",
"typescript-definition-tester": "0.0.5"
},
"npmName": "redux",
Expand Down
2 changes: 1 addition & 1 deletion test/typescript/compose.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,4 +36,4 @@ const t11: number = compose(stringToNumber, numberToString, stringToNumber,


const funcs = [stringToNumber, numberToString, stringToNumber];
const t12 = compose(...funcs)('bar', 42, true);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This appears to have just been a bug in the old test case, which the newer version of TypeScript caught.

const t12 = compose(...funcs)('bar');
19 changes: 10 additions & 9 deletions test/typescript/reducers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,15 @@ interface AddTodoAction extends Action {
}


const todosReducer: Reducer<TodosState> = (state: TodosState,
action: Action): TodosState => {
switch (action.type) {
case 'ADD_TODO':
return [...state, (<AddTodoAction>action).text]
default:
return state
const todosReducer: Reducer<TodosState, AddTodoAction> =
(state = [], action) => {
switch (action.type) {
case 'ADD_TODO':
return [...state, action.text]
default:
return state
}
}
}

const todosState: TodosState = todosReducer([], {
type: 'ADD_TODO',
Expand Down Expand Up @@ -47,7 +47,8 @@ type RootState = {
counter: CounterState;
}

const rootReducer: Reducer<RootState> = combineReducers({

const rootReducer = combineReducers<RootState, Action | AddTodoAction>({
todos: todosReducer,
counter: counterReducer,
})
Expand Down
2 changes: 1 addition & 1 deletion test/typescript/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ const reducer: Reducer<State> = (state: State, action: Action): State => {

/* createStore */

const store: Store<State> = createStore<State>(reducer);
const store: Store<State> = createStore(reducer);

const storeWithPreloadedState: Store<State> = createStore(reducer, {
todos: []
Expand Down