Improve TypeScript types #3566
Improve TypeScript types #3566
Conversation
|
Deploy preview for redux-docs ready! Built with commit ebe7423 |
|
Can you clarify and document what all is being changed? |
src/applyMiddleware.ts
Outdated
| ): StoreEnhancer<any> { | ||
| return (createStore: StoreCreator) => <S, A extends AnyAction>( | ||
| reducer: Reducer<S, A>, | ||
| export default function applyMiddleware< |
There was a problem hiding this comment.
The previous implementation only supported a max of 5 middlewares before types started being swallowed. This change fixes that, but introduces a breaking change for TypeScript users who were passing in up to 5 middleware types plus a state S type. Now, the state type is the first generic type. You can provide a state type manually, but the middlewares type will be automatically applied, whether you provide a state type or not.
There was a problem hiding this comment.
@timdorr now that I look at this again, I don't think we should merge this, because it requires each middleware to be of the same type. As we discussed at length in other threads, there seems to be no way around this Ext1, Ext2, ... business.
| M extends Middleware<any, any, infer D> ? { dispatch: D } : never, | ||
| S | ||
| > { | ||
| return (createStore: StoreEnhancerStoreCreator<any>) => ( |
There was a problem hiding this comment.
This StoreEnhancerStoreCreator type is new and more accurate than just a plain StoreCreator.
There was a problem hiding this comment.
I'm not groking why the StoreEnhancer's Extension here would be an any. We know what apply middleware does (it has a very specific API) and it doesn't do any actual extension of the store's properties. So, at the very least, shouldn't it be {}?
| @@ -273,11 +273,7 @@ export default function createStore< | |||
| throw new Error('Expected the nextReducer to be a function.') | |||
| } | |||
|
|
|||
| // TODO: do this more elegantly | |||
There was a problem hiding this comment.
The elegance is done by line 100, defining a Reducer<any, any>, so that we can keep overwriting it with new reducers w/o issues. This is fine too, because this particular type isn't being returned. It's only used internal to this function.
test/applyMiddleware.spec.ts
Outdated
| import * as reducers from './helpers/reducers' | ||
| import { addTodo, addTodoAsync, addTodoIfEmpty } from './helpers/actionCreators' | ||
| import { thunk } from './helpers/middleware' | ||
|
|
||
| describe('applyMiddleware', () => { | ||
| it('warns when dispatching during middleware setup', () => { | ||
| function dispatchingMiddleware(store) { | ||
| function dispatchingMiddleware(store: Store<any, rootActionTypes>) { |
There was a problem hiding this comment.
Previously, this store was implicitly an any type, which isn't a helpful TS type and is discouraged. Being more explicit here.
| @@ -51,16 +59,11 @@ describe('applyMiddleware', () => { | |||
| const spy = jest.fn() | |||
| const store = applyMiddleware(test(spy), thunk)(createStore)(reducers.todos) | |||
|
|
|||
| // the typing for redux-thunk is super complex, so we will use an as unknown hack | |||
There was a problem hiding this comment.
Now that the store is properly typed, there's no need to use any "hacks".
test/helpers/middleware.ts
Outdated
| export function thunk({ dispatch, getState }: MiddlewareAPI) { | ||
| return (next: Dispatch) => <T>(action: ThunkAction) => | ||
| export const thunk: Middleware< | ||
| ThunkDispatch<any, {}, rootActionTypes>, |
There was a problem hiding this comment.
This first generic type isn't even used by Middleware.
test/helpers/middleware.ts
Outdated
| export const thunk: Middleware< | ||
| ThunkDispatch<any, {}, rootActionTypes>, | ||
| any, | ||
| ThunkDispatch<any, {}, rootActionTypes> |
There was a problem hiding this comment.
rootActionTypes here limits dispatching only to these action types.
test/helpers/reducers.ts
Outdated
|
|
||
| export function todos(state: Todo[] = [], action: TodoAction) { | ||
| export function todos(state: Todo[] = [], action: rootActionTypes) { |
There was a problem hiding this comment.
Now that the action is properly typed as rootActionTypes, the switch statement will narrow down the types to the appropriate one. This type of stuff is what TypeScript is all about!
| @@ -82,6 +82,7 @@ | |||
| "prettier": "^1.18.2", | |||
| "rimraf": "^3.0.0", | |||
| "rollup": "^1.20.3", | |||
| "redux-thunk": "^2.3.0", | |||
There was a problem hiding this comment.
If we're going to be testing thunks, it only makes sense that this dev dependency would aid in that effort.
|
Do you think we could break this up into smaller PRs? It might be easier to look through that way. Either that or at least multiple commits. |
|
Unfortunately, it's harder than it seems to break this one apart, as each piece is quite interconnected. If a 150-line PR is too big, I can close it. |
jednano
force-pushed
the
better-types
branch
from
6a03145 to
7195eae
Compare
jednano
force-pushed
the
better-types
branch
from
7195eae to
cf2b892
Compare
|
@jedmao Can you give this a poke to fix the prettier error in |
|
Done! |
|
@timdorr any updates? |
|
@timdorr @markerikson any change to get an alpha release with these types? |
|
No, I haven't had a chance to really think through them. |
|
Closing this out because we never got good consensus. I would recommend re-opening with a smaller scope of work. |
The TS enhancement stage got the green light 🚦, so this is a PR towards that effort. Could be some more after this.