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.

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

Incorrect action creator type when using bindActionCreators and redux-thunk #6

Closed
rluiten opened this issue Aug 7, 2017 · 15 comments · Fixed by #36
Closed

Incorrect action creator type when using bindActionCreators and redux-thunk #6

rluiten opened this issue Aug 7, 2017 · 15 comments · Fixed by #36

Comments

@rluiten
Copy link

rluiten commented Aug 7, 2017

Specifically I have this action defined.

getTransitionForm: (params: GetTransitionFormParams) =>
    (dispatch: Dispatch<RootState>, getState: () => RootState, extraArgument: any) => 
         Promise<any>;

When mapped as follows as per your guide from section "Connected Container without OwnProps using Type Inference" https://github.com/piotrwitek/react-redux-typescript-guide#connected-container-with-ownprops

export const mapDispatchToProps = (dispatch: Dispatch<RootState>) =>
  bindActionCreators(
    {
      getTransitionForm,
    },
    dispatch
  );
const dispatchProps = returntypeof(mapDispatchToProps);

The resulting value of dispatchProps is

const dispatchProps: {
    getTransitionForm: (params: GetTransitionFormParams) =>
        (dispatch: Dispatch<RootState>, getState: () => RootState, extraArgument: any) => 
             Promise<any>;
}

I believe after binding it should be.

const dispatchProps: {
    getTransitionForm: (params: GetTransitionFormParams) => Promise<any>;
}

Which I expect is not going to be easy to derive.
I only discovered this as I wanted to do something with the returned promise.
Thanks for your great guide.

@piotrwitek
Copy link
Owner

piotrwitek commented Aug 9, 2017

@rluiten hello
EDIT: After writing my post I have noticed that maybe you forgot that you have a thunk action opposed to regular action creator and that is why the resulting value of dispatchProps is wrapped in another function, when you take it out, then everything is correct :)

Original response:
This is how bindActionCreators declaration looks:

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

As you can see it will return ActionCreatorsMapObject, so resulting value of dispatchProps is perfectly fine.
Moreover when you check the type of getTransitionForm prop in the connected component you will find you have exactly the type you have expected after the binding.

This is what I have in my project, the dispatched action is returning a Promise with updated model or undefined, which works exactly as I expect (this is a thunk action, so there is an additional function wrapper):

const mapDispatchToProps = (dispatch: Dispatch) => bindActionCreators({
  updateRegion: SitesReducer.updateRegion,
}, dispatch);
...
const updatedRegion = await this.props.updateRegion(region); // updateRegion: (payload: RegionModel) => (dispatch: Dispatch<RootState>, getState: () => RootState) => Promise<void | RegionModel | undefined>

PS: if your question is resolved, then please mark it closed. Thanks.

@rluiten
Copy link
Author

rluiten commented Aug 9, 2017

I freely admit I might have flubbed something.
But it compiles with no errors and appears to work (but that might be pure fluke).

Definition of my Component and its properties.

interface RouteParams {
  transitionStatementId: number;
}
export interface OwnProps extends RouteComponentProps<RouteParams> {}
export const mapDispatchToProps = (dispatch: Dispatch<RootState>) =>
  bindActionCreators(
    {
      getTransitionForm
    },
    dispatch
  );
const stateProps = returntypeof(mapStateToProps);
const dispatchProps = returntypeof(mapDispatchToProps);
export type Props = typeof stateProps & typeof dispatchProps & OwnProps;

class TransitionForm extends React.Component<AllProps, OwnState> { /* etc */ }

Inside my components member functions the type of 'props.getTransitionForm` appears to be.
(conveniently visible in VSCode but I did not assume VSCode was correct)
I am calling the action from a submit() function that is part of redux-form.

(params: GetTransitionFormParams) =>
        (dispatch: Dispatch<RootState>, getState: () => RootState, extraArgument: any) => 
             Promise<any>;

The fire*Action() functions are just examples.

I setup a test and i can use await it compiles with no errors, and appears to work correctly.

    const getPayload = {state, apiUrls, transitionStatementId: 23};
    const getResult  = getTransitionForm(getPayload);
    try {
        await getResult;
        fireGotAction();
    } catch (e) { 
        fireErrorGotAction();
    }

If I use .then() I get a compile error.

The getResult above gives me the compile error where i try to use .then().
TS2339: Property 'then' does not exist on type 'ThunkAction<Promise<any>, RootState, any>'.

    const getPayload = {state, apiUrls, transitionStatementId: 23};
    const getResult  = getTransitionForm(getPayload);
    getResult.then(() => fireGotAction(), () => fireErrorGotAction());  

This leads me to believe the types are not really correct, I am not implying they are easy to fix or can be fixed, I fudged the type for myself for now. It appears await isn't enforcing type constraints equivalently to .then(), and since the underlying value is actually a promise await works anyway.

Currently i am just casting the property action to any to call then and all seems well with the world.

Thanks for your response, I raised this more to see if it rings a bell with you that might point out something I had missed or possibly even something you can address. I think its complex enough some sort of fix might not be easy and may require much deeper type knowledge than I have, or even typescript may not be able to do express the constraint in its current version.

Thanks,

@piotrwitek
Copy link
Owner

piotrwitek commented Aug 11, 2017

@rluiten this time I have thoroughly investigated and I found you are right, types are not correct but only for the thunk action creators.
Specifically bindActionCreators could have been augmented by redux-thunk to fix that behaviour.

The types returned from connect should have bound action creators, but it seems they are still typed as thunk action creators so dispatch is not correctly applied.

When I changed mapDispatchToProps declaration to not use bindActionCreators it's fixed, you can try:
const mapDispatchToProps = (dispatch: Dispatch) => ({
updateRegion: (payload: RegionModel) => dispatch(SitesReducer.updateRegion(payload)),
});

The second one works because this time dispatch is correctly augmented by redux-thunk typings.

The necessary fix is to augment bindActionCreators to apply augmented dispatch that can handle thunk actions, but it's not possible with current Type Inference limitations.

@rluiten
Copy link
Author

rluiten commented Aug 12, 2017

Awesome, I have learned so much from this, and your answer makes a lot of sense.
Thanks for persisting with this :).

@piotrwitek
Copy link
Owner

@rluiten no problem, me as well learned a new trick :)

I'll add it later to the guide as caveat giving you a well earned credit :)

@piotrwitek piotrwitek changed the title I think the use of returntypeof(mapDispatchToProps) is incorrect. Incorrect action creator type when using bindActionCreators and redux-thunk Aug 28, 2017
@brianpaulcarpenter
Copy link

Hi, I just bumped into this issue as well and indeed switching away from bindActionCreators worked. Has anyone posted this to the redux team, to see if they can fix it? I can't find much documentation on this problem other than this thread. thanks

@piotrwitek piotrwitek added this to the Rework Redux Section milestone Dec 27, 2017
piotrwitek added a commit that referenced this issue Dec 28, 2017
piotrwitek added a commit that referenced this issue Dec 28, 2017
* improved redux section text and examples

updated tslint to strict for 2.6 and future new strict features

* review fixes

* updated deps

* closes #6

* updated TOC

* final touches
@piotrwitek
Copy link
Owner

Added a new paragraph warning and explaining root case of this issue: https://github.com/piotrwitek/react-redux-typescript-guide#caveat-with-bindactioncreators

@njgraf512
Copy link

Possibly helpful to others: I was having this issue as well, but it was resolved once I upgraded @types/react-redux to v5.0.19.

@davidspiess
Copy link

@njgraf512 can you post an example? I still have the issue with @types/react-redux v5.0.20

@kgaregin
Copy link

kgaregin commented Aug 1, 2018

same issue, @njgraf512 updating react-redux typings didn't help and nothing was found in updated package that could help to fix this

@davidspiess
Copy link

Maybe we should open a issue on https://github.com/DefinitelyTyped/DefinitelyTyped.

@njgraf512
Copy link

njgraf512 commented Aug 3, 2018

Sorry for the delay responding. It's been a while since I looked at this, but I'll get the conversation started with what I have and see if that's helpful. I'm not totally sure if my code will directly translate, but hopefully, it will be helpful.

type OwnProps = {
  foo: string,
};

type ReduxStateProps = {
  bar: boolean,
};

type DispatchProps = {
  myAC: typeof myAC.bound,
  // note the type of myAC is: (x: ArgType) => AbortableAction<FluxAction<void>>
  // see below for relevant type definitions of return type
};

type ComponentProps = OwnProps & ReduxStateProps & DispatchProps;

class ComponentFoo extends React.Component<ComponentProps> { // ... }

function mapStateToProps(state: RootState, props: OwnProps): ReduxStateProps {
  // ...
  return {
    bar: true
  };
}

function mapDispatchToProps(dispatch: Dispatch<RootState>) {
  const acs = {
    myAC,
  };
  return bindActionCreators<typeof acs, DispatchProps>(acs, dispatch);
}

export default connect(mapStateToProps, mapDispatchToProps)(DecisionDetails);

For reference, ReturnType<typeof mapDispatchToProps> is:

{
    myAC: (x: ArgType) => AbortableAction<FluxAction<void>>;
}

The utility types involved are:

export interface AbortableAction<R> extends BluebirdPromise<R> {
  abort: () => void;
  url: string;
}

export interface FluxAction<Payload> extends Action {
  type: string;
  payload: Payload;
  meta?: any;
  error: boolean;
}

export type ActionCreator<Arg, Payload> = {
  (a: Arg): FluxAction<Payload>,
  type: string,
  bound: (x: Arg) => AbortableAction<FluxAction<Payload>>,
  arg: Arg,
  pl: Payload,
};

Hopefully, that helps. Let me know if there are any questions, and I'll try to answer them.

@RMHonor
Copy link

RMHonor commented Nov 6, 2018

If people are still looking at how to get the correct typings for bindActionCreators when passing in ThunkActions, I've got an open PR to fix this, specifically the line: https://github.com/reduxjs/redux-thunk/pull/224/files#diff-b52768974e6bc0faccb7d4b75b162c99R31

Overloading the function definition and conditionally returning a different object type if the action passed is of type ThunkAction:

declare module 'redux' {
  /**
   * Overload for bindActionCreators redux function, returns expects responses
   * from thunk actions
   */
  function bindActionCreators<M extends ActionCreatorsMapObject<any>>(
    actionCreators: M,
    dispatch: Dispatch,
  ): { [N in keyof M]: ReturnType<M[N]> extends ThunkAction<any, any, any, any> ? (...args: Parameters<M[N]>) => ReturnType<ReturnType<M[N]>> : M[N] }
}

Let me know if it fits your needs

@chrispaynter
Copy link

The necessary fix is to augment bindActionCreators to apply augmented dispatch that can handle thunk actions, but it's not possible with current Type Inference limitations.

Thought I was going crazy till I found this thread :)

Does this still hold true @piotrwitek?

@piotrwitek
Copy link
Owner

piotrwitek commented Feb 4, 2019

Not sure, It's been a while since I've been using thunks. But type definitions correction for bindActionCreators was merged few days ago, so might be fixed soon reduxjs/redux-thunk#224

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants