-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Add overload for bindActionCreators #224
Conversation
I'm pretty sure this is fine. Any TS experts want to chime in? |
Am I approaching it wrong? I used your interface OwnProps {
a: number;
b: string;
}
// my custom helper
type ConnectedThunk<T> = T extends (...args: infer K) => ThunkAction<infer R, infer _S, infer _Element, infer _A> ? (...args: K) => R : T;
type ConnectProps<T extends object> = {
[P in keyof T]: ConnectedThunk<T[P]>
};
type DispatchProps = ConnectProps<{
someAction: typeof actions.someAction, // ReduxThunk<...>
}>;
type Props = OwnProps & DispatchProps;
class Test extends React.Component<Props, {}> { ... }
const actions = (dispatch: Dispatch<AnyAction>) => bindActionCreators({
someAction: actions.someAction,
}, dispatch);
export default connect<{}, DispatchProps, OwnProps, {}>(selector, actions)(Test); Is there a way to avoid some type declarations and my The above works very well so far, just a bit verbose and we may need to ship a helper like |
@farnoy Yes, you'll have to define your own return type if you don't pull it from the bindActionCreators return type. It's something I've built myself and didn't think to include inside redux-thunk, I'll add the base generic. |
I suppose that if you define your |
I think there is something wrong with Suppose I have this ThunkAction definition in my app
Then I have my thunk action creator
At this point, the typing for Then I do
At this point the typing is correct too. I can see that Now, I need to pass that
The only thing that works is this:
Only in this case, I have the |
@optimistiks Yes, you're right, I incorrectly typed |
@optimistiks I don't think you need to define the interface for DispatchProps at all. At this point though I don't see what the purpose is of the ThunkActionDispatch type. @RMHonor Could you explain why or when this type would be needed? |
@C-Higgins Some developers prefer to define what the component expects as actions, rather than the actions defining the props. Personally in future I think I'll take your strategy, it seems to be clearer and requires less in the way of interface definition. But for those who'd rather define props first, the option is there. |
chiming in to say that, as a non-expert user of redux and as someone attempting to add some I believe this fix would help me, but I'm not sure it'd be obvious what the right types are for each piece w/o a nice example. links welcome if this already exists somewhere! |
@ekilah : we do have a WIP docs PR to add a page on using TypeScript: Having said that, Tim and I don't actively use TS, and neither of us know enough on the topic to write docs like this ourselves. |
Tried your branch in my personal project, and it perfectly worked. Thanks for this PR, and waiting for merge. |
I use these lines for interface Dispatch<A extends Action = AnyAction> {
<T extends A>(action: T): T;
<R, S, E, T extends A>(action: ThunkAction<R, S, E, T>): R;
} |
Thanks for this. Using this branch in my code and it addresses the above issue. Here's an example of how I'm using it for others who come across this (because I always appreciate having lots of tangible examples). import React from 'react';
import { bindActionCreators, AnyAction } from 'redux';
import { ThunkDispatch } from 'redux-thunk';
import {AppState} from './AppState'
// Types to cut down on boilerplate across dozens of thunks.
type Dispatcher = ThunkDispatch<AppState, undefined, AnyAction>;
type GetState = () => AppState;
const getAuthToken = (username: string, password: string) => {
return async (dispatch: Dispatcher, getState: GetState) => {
await request.post(`auth/url/`).send({ username, password });
return dispatch({ type: 'SET_AUTH_TOKEN', token: res.body.token, username });
};
};
// Before this PR, `bindActionCreators` would mess up the typing, causing an error below...
const mapDispatch = (dispatch: Dispatcher) => bindActionCreators({
getAuthToken,
}, dispatch);
type Props = ReturnType<typeof mapDispatch>;
class MyComponent extends React.PureComponent<Props> {
public render() {
// Before this PR, the line below would complain that we can't await non-Promises.
// the bindActionCreators was messing up the typing so it thought we were returning a
// normal function, not an async function (I think).
await this.props.getAuthToken('username', 'password');
console.log('Then I do something else.');
}
}
export default connect(null, mapDispatch)(MyComponent)
|
Is there any chance of getting some movement on this? |
I haven't seen any unaddressed complaints, so I'll merge it in. |
Any idea when this would be released? |
@markerikson @timdorr can we publish this change to npm? |
Just wondering what the plan is for publishing a new version of this to npm? |
Just encountered this issue myself; merged long ago, but still not on NPM. Also wondering why. But not just here for a +1, I'm also wondering if someone can tell me why this Additionally, this appears to create inconsistent behavior. When I pass in an object, my TS looks like this: interface DispatchProps {
myActionCreator: MyThunkActionCreator;
}
// Please don't read too much into this dummy component
const MyComponent: FunctionComponent<ResolveThunks<DispatchProps>> = ({ myActionCreator }) => (
<h1>{myActionCreator()}</h1>
);
export default connect<{}, DispatchProps, {}, MyStore>(null, { myActionCreator })(MyComponent); You can see that the But when I use interface DispatchProps {
myActionCreator: MyThunkActionCreator;
}
// Please don't read too much into this dummy component
const MyComponent: FunctionComponent<ResolveThunks<DispatchProps>> = ({ myActionCreator }) => (
<h1>{myActionCreator()}</h1>
);
const mapDispatchToProps: MapDispatchToPropsFunction<ResolveThunks<DispatchProps>, {}> = (dispatch) => bindActionCreators({ myActionCreator });
export default connect<{}, ResolveThunks<DispatchProps>, {}, MyStore>(null, mapDispatchToProps)(MyComponent); |
Checking in: https://www.npmjs.com/package/redux-thunk hasn't been updated in over a year and therefore doesn't include this fix. Is there anything we can do to get this change published? |
+1 Any plans on release it? |
I don't think the types on master are in a good spot just yet. I don't want to push out a bad release and upset folks even more. |
bindActionCreators typings still making my prefrontal cortex bleed |
@timini, for me to be able to forget about this problem till it's released, I just created a typing file such as Disclaimer - I can't remember where I got these definitions from, but it'll be a simple case of deleting them when the release happens. I'm also not aware of any side effects it may have, but I've not experienced any yet.
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Hi @RMHonor,
|
As per #223, redux's
bindActionCreators
only works nicely for standard action creators. This implementation infers the return type of ThunkActions so the application could respond accordingly. E.g. action fires an XHR, the application has no way of knowing when the XHR has resolved, other than through watching the state. This way, you can now have promise resolution in your application outside of the redux store.I've updated the TypeScript version to 3.1, this gives us access to the
Parameters
generic, theReturnType
generic, and conditional types.Feedback welcome
Note, the should fix the concerns from the now closed #213