-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Revamp TypeScript typing with more type safety #2563
Conversation
@@ -36,4 +36,4 @@ const t11: number = compose(stringToNumber, numberToString, stringToNumber, | |||
|
|||
|
|||
const funcs = [stringToNumber, numberToString, stringToNumber]; | |||
const t12 = compose(...funcs)('bar', 42, true); |
There was a problem hiding this comment.
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.
index.d.ts
Outdated
export interface AnyAction extends Action { | ||
// Allows any extra properties to be defined in an action. | ||
[extraProps: string]: any; | ||
export interface Action<T = any> { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes
index.d.ts
Outdated
*/ | ||
export type Reducer<S> = (state: S, action: AnyAction) => S; | ||
export type Reducer<S = {}, A extends Action = Action> = (state: S, action: A) => S; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason for S
to default to {}
?
Also, I don't think that it is correct to constraint the type of action
. A reducer must be able to accept every possible action, which is any object with type
property. It should just ignore all actions that don't affect its state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a particular reason for
S
to default to{}
?
Not really, I was just on a defaulting rampage.
Also, I don't think that it is correct to constraint the type of action. A reducer must be able to accept every possible action, which is any object with type property. It should just ignore all actions that don't affect its state.
It's perfectly valid to constrain the type of actions that a reducer handles. It must be able to accept any action in your app, but not any arbitrary object with a type
field. This is critical to making the library nice to use in TypeScript because it allows the type system to automatically refine the types for you as you learn information by inspecting the type
field. For example:
// The universe of all actions used in your app
export type AnyAction =
| SetGreeting
| IncrementCounter
| SetCompletionStatus
export interface SetGreeting {
type: 'SET_GREETING'
greeting: string
}
export interface IncrementCounter {
type: 'INCREMENT_COUNTER'
counterIndex: number
}
export interface SetCompletionStatus {
type: 'SET_COMPLETION_STATUS'
completed: boolean
}
const reducer: Reducer<State, AnyAction> = (state = initialState, action) => {
switch (action.type) {
case: 'SET_GREETING':
return { ...state, greeting: action.greeting }
case: 'INCREMENT_COUNTER':
return { ...state, counters: state.counters[action.counterIndex] + 1 }
case: 'SET_COMPLETION_STATUS':
return { ...state, completed: action.completed }
}
}
What's super cool about this is
- It's completely type safe. Inside each
case
, the type ofaction
has been refined such that it knows what fields should be available on it and what types they have. If I tried to accessaction.counterIndex
in a branch where I hadn't checked thataction.type === 'INCREMENT_COUNTER'
it would be a type error. - We get exhaustiveness checking, so that no
default
case is required because the type system has proven that you've handled all possible action types. - You can perform an app-wide refactoring on the string literals being used for the action
type
fields and they will also change these switch cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's always an init
action which is internal to Redux whose type
can potentially be made random.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but your app can pretend that doesn't exist for all intents and purposes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To me making types that don't agree with the runtime is an abuse of type system.
However, I can see that this approach gives really nice DX, so I tend to agree with you. It should be up to developer to decide the level of strictness they want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could certainly opt to include the init action in your top-level union of action types if you want to capture absolutely all actions that might be passing through your reducer. The point is to constrain the types to some known, closed universe so that you can have type safety and semantic awareness for refactoring.
index.d.ts
Outdated
export interface ReducersMapObject { | ||
[key: string]: Reducer<any>; | ||
export type ReducersMapObject<S = {}, A extends Action = Action> = { | ||
[K in keyof S]: Reducer<S[K], A>; |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
index.d.ts
Outdated
* This is not part of `Action` itself to prevent users who are extending `Action. | ||
* @private | ||
*/ | ||
export interface AnyAction extends Action { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you sure AnyAction
can be safely removed? Its absence broke stuff on TS 2.4
See #2467
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, since Reducer
s are parameterized by action type now it should no longer be necessary.
Looking forward to see any updates on this PR |
Rebased this onto |
index.d.ts
Outdated
*/ | ||
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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
index.d.ts
Outdated
*/ | ||
export type ReducersMapObject<S> = { | ||
[K in keyof S]: Reducer<S[K]>; | ||
export type ReducersMapObject<S = {}, A extends Action = Action> = { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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> }
?
There was a problem hiding this comment.
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.
index.d.ts
Outdated
@@ -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>; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
index.d.ts
Outdated
@@ -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. |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
index.d.ts
Outdated
*/ | ||
export interface Store<S> { | ||
export interface Store<S = {}, A extends Action = Action, N = never> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
index.d.ts
Outdated
*/ | ||
export interface Store<S> { | ||
export interface Store<S = {}, A extends Action = Action, N = never> { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Guys, let's continue our discussion here instead of code comments because they get hidden after code updates.
I think that's the way to go. The type for Middleware would be something like interface Middleware<NewDispatch> {
(api: MiddlewareAPI): (next: Dispatch) => Dispatch | NewDispatch;
} But there's one problem: |
This is tricky. If you think this further, than the export interface MiddlewareAPI<S = any, D = Dispatch> {
dispatch: D;
getState(): S;
} But then in the EDIT: Or you could just say |
@aikoven To answer your question, wouldn't this be sufficient? export interface MiddlewareAPI<S = any, D = Dispatch> {
dispatch: D;
getState(): S;
}
export interface Middleware<NewDispatch = Dispatch, D = Dispatch> {
<S = any>(api: MiddlewareAPI<S, NewDispatch | D>): (
next: D
) => NewDispatch | D;
} And in import {Middleware, Dispatch as ReduxDispatch} from 'redux';
export type ThunkAction<A, R, S, E> = (
dispatch: Dispatch<A>,
getState: () => S,
extraArgument: E
) => R;
export interface Dispatch<A> extends ReduxDispatch<A> {
<R, S, E>(asyncAction: ThunkAction<A, R, S, E>): R;
}
declare const thunk: Middleware<Dispatch<any>> & {
withExtraArgument(extraArgument: any): Middleware<Dispatch<any>>;
};
export default thunk; The |
That does not compile though in my current project (using the import {
LocationChangeAction,
RouterAction,
RouterState,
} from 'react-router-redux';
import {ThunkAction as ReduxThunkAction} from 'redux-thunk';
import {Action} from '../src/actions';
export interface AppState {
routing: RouterState;
...
}
export type ThunkAction<Result> = ReduxThunkAction<
Action,
Result,
AppState,
undefined
>;
export interface Dispatch {
<Result>(action: ThunkAction<Result>): Result;
(action: LocationChangeAction | RouterAction):
| LocationChangeAction
| RouterAction;
(action: Action): Action;
} This |
It seems like fixing the type of |
@pelotom The only thing that bothers me is the I am pretty sure there exists a good solution for Otherwise, the changes look very well done to me. |
OK, I'm merging it in! |
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
Because we didn't have
@pelotom @aikoven @KingHenne or anyone else want to provide a fix? I'd like to push forward on Redux 4.0 and this is a blocker. |
@timdorr hmm, what am I doing wrong?
|
Never mind, I reproduced it now. |
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
So glad that this got merged. Is there anything I can help to get this released to npm? |
It's in 4.0.0-beta.1. |
Ah great! Thank you so much. |
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
* Revamp TypeScript typing with more type safety * Provide a default action type for combineReducers * Change state default types to any * Don't parameterize Dispatch with a state type * Remove docstring about excised type parameter
This fixes many issues with the current TypeScript typings, while remaining mostly* backwards compatible:
Action
tracking the type of thetype
tag.Dispatch
(but keep it around for backwards compatibility).Dispatch
which is the type of things which may be dispatched (previouslyDispatch
had a polymorphic apply which allowed anything to be dispatched).A
andN
toStore
which respectively track the actions it may dispatch and all other non-action things which may be dispatched. Then itsdispatch
method can takeA | N
.A
toReducer
which tracks the actions it understands, instead of just usingAnyAction
.With these changes, type inference for a lot of redux functions improves dramatically, and thus we get more type safety with less effort. I won't pretend these are perfect; in particular I think the middleware/enhancer functions could use some work to make the
N
variables accumulate as appropriate, but it's hopefully a step in the right direction.As part of this, upgraded the TypeScript dependency to
^2.4.2
.* As you can see from some of the modified testcases, in a few rare cases it's necessary to modify type parameter annotations, e.g. when calling
combineReducers
.