Add types for redux-router5 #210
Conversation
|
@jkelin I'm just trying to use these definitions, but already seeing a few errors. I can fix them, how'd be better to do it? Fork your fork and send a PR may be? |
|
Any help is greatly appreciated, I have added you as collaborator for https://github.com/jkelin/router5 so you can work directly on branches for these PRs. What errors are you seeing? Compilation errors? Type errors (as in types not representing the actual data)? |
| const CAN_DEACTIVATE = '@@router5/CAN_DEACTIVATE'; | ||
| const CAN_ACTIVATE = '@@router5/CAN_ACTIVATE'; | ||
|
|
||
| export const actionTypes: { |
There was a problem hiding this comment.
Here, the correct pattern seems to be NAVIGATE_TO: typeof NAVIGATE_TO or else actionTypes keys seen as any.
There was a problem hiding this comment.
Ah correct, thank you!
packages/redux-router5/index.d.ts
Outdated
|
|
||
| export function reduxPlugin<TState extends RouterState>(dispatch: Dispatch<TState>): Plugin; | ||
|
|
||
| export function routeNodeSelector<TState extends RouterState>(routeNode: string, reducerKey?: string): (state: TState) => Route; |
There was a problem hiding this comment.
Here at the end should be (state: {router: TState}) => RouterState instead of (state: TState) => Route;. Because by default (or maybe even always) router5Reducer is meant to be used as such:
combineReducers({
// ...
router: router5Reducer,
// ...
})
And we should get RouterState instead of Route from it, because Route is for routes description, not for any state. And it's not needed to import it with that change :-)
There was a problem hiding this comment.
I went with export function routeNodeSelector<TState extends { router: RouterState }>(routeNode: string, reducerKey?: string): (state: TState) => Route; because I want to allow different root state not different RouterState.
|
@jkelin I wrote comments on relevant lines. If nothing breaks for you and you can apply them, please do. I'm a bit busy right now :-) |
|
Fixed, thank you. If you see anything else, please let me know. |
|
@jkelin Cool, thanks 👍 I will definitely let you know of other issues if there's any. I'm in the process of migrating a big project to TypeScript right now :-) |
|
Giving you collaborator access so you can contribute directly on the repo and merge when done. |
|
@troch I'm not sure yet, didn't migrate everything. But I think merging it will not hurt anybody. We can fix types later if needed. |
|
OK, I'll merge both then, cut a release later today, and you can adjust type definitions as needed |
|
@troch Can you also merge this one? Or just apply these changes manually. https://github.com/jkelin/router5/pull/1 |
|
No I can't it is not on this repo. Can you create a PR on this repo for it? (you shouldn't need your fork) |
|
Thanks both of you! |
No description provided.