Skip to content
This repository has been archived by the owner on Jul 28, 2022. It is now read-only.

mapDispatchToProps is wrong #2

Closed
nripendra opened this issue Mar 6, 2018 · 8 comments
Closed

mapDispatchToProps is wrong #2

nripendra opened this issue Mar 6, 2018 · 8 comments

Comments

@nripendra
Copy link

The implementation of mapDispatchToProps seems to be wrong.

I think this is more appropriate implementation.

function mapDispatchToProps(component: any, props: any) {
    Object.keys(props).forEach(actionName => {
      const action = props[actionName];
      Object.defineProperty(component, actionName, {
        get: () => (...args: any[]) => _store.dispatch(action(...args)),
        configurable: true,
        enumerable: true
      })
    });
  }
@mlynch
Copy link
Contributor

mlynch commented Apr 24, 2018

Wrong in what way?

@tonai
Copy link

tonai commented Apr 26, 2018

Maybe because original redux actions simply return objects:

function myAction() {
  return { type: 'MY_ACTION' };
}

redux-thunk should be an optional package.

In the mapDispatchToProps implementation that @nripendra is pointing, it seems that you are waiting for actions that look more like this:

function myAction() {
  return dispatch => dispatch({ type: 'MY_ACTION' });
}

@mlynch
Copy link
Contributor

mlynch commented Apr 26, 2018

Ah, good point on thunk, I tend to always include it so over looked this. Thoughts on making thunk required? Makes it so much easier to build actions

@tonai
Copy link

tonai commented Apr 27, 2018

Yes it is a very handy package.
And you should not care whether the action return an object or a function.
It is handled by the middleware.

In my opinion it would be great to have the mapDispatchToProps declaration in the component being more react-redux-like.

In fact, react-redux does have a shorthand where you only write something like this (like what you have to write with this package):

import { updateUserInput } from 'myActions.js';
const mapDispatchToProps = {
  updateUserInput
};

But the classic way is more like this:

import { updateUserInput } from 'myActions.js';
const mapDispatchToProps = (dispatch) => ({
  updateUserInput: value => dispatch(updateUserInput(value))
});

And letting the user manage the way he like dispatching actions, like, for example, using the bindActionCreators helper from redux to write this:

import { updateUserInput } from 'myActions.js';
const mapDispatchToProps = (dispatch) => bindActionCreators({updateUserInput}, dispatch);

@mlynch
Copy link
Contributor

mlynch commented Apr 27, 2018

Also I'm realizing that mapStateToProps is not actually mapping the state to props, it's requiring those be State() fields. So that needs to be fixed as well

@pablolazaro
Copy link

Any updates on this?

@chimon2000
Copy link

When attempting to integrate with rematch, not being able to pass a function to mapDispatchToProps causes this to break, since the library expects actions to be mapped this way.

@larionov
Copy link
Contributor

When I use reduxThunk.withExtraArgument the extra argument is not getting passed to the dispatch.

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

No branches or pull requests

6 participants