Suggestion: Warn when bindActionCreators encounters non-function property

[artgillespie]

I've found myself debugging 'Dude, where's my prop?' situations when I've done something like

import { doSomething } from './actions' // for whatever reason, `doSomething` is `undefined`

...

const MyComponent = props => {
   ...
   const onClick = () => {
     props.doSomething() // error at runtime --> doSomething is undefined
   }
}

const mapDispatchToProps = {
  ...
  doSomething
  ...
}

It would speed up debugging in these situations if bindActionCreators logged a warning when it's passed an object with non-function properties.

[timdorr]

Seems reasonable to me. Anyone else have any input?

[markerikson]

Seems reasonable.

(Also, I should actually read prior comments more carefully so I don't sound like I'm just parroting someone else :) )

[timdorr]

OK, let's merge it in.

[sebcode]

I think the automatic filtering was quite convenient for projects where you define both action creators and action types in the same file and then do import * as actions from 'actions' in your container and use that as second argument for connect().

This usage pattern is also used in the official universal example that now throws warnings.

container.js:

import * as actions from 'actions'

[...]

export default connect(mapStateToProps, actions)(Counter)

actions.js:

export const SET_COUNTER = 'SET_COUNTER'
[...]

export const set = (value) => ({
  type: SET_COUNTER,
  payload: value
})
[...]

Not sure if that approach is considered outdated, but at least I use it in my project a lot and it's still described in the official docs.

The not so pretty workaround I use now in my project:

export default connect(mapStateToProps, { ..._.pickBy(actions, a => _.isFunction(a)) })(Counter)

[kulakowka]

I updated the redux to 3.7.0 and got the error:

bindActionCreators expected a function actionCreator for key '__esModule', 
instead received type 'boolean'.

Tell me how to fix it? Have any ideas?

[elrumordelaluz]

I found the warning a source of some problems or misunderstanding.

I replied some questions related.

Since the application code continues working as expected (at least in my usecase), could be helpful if the the warning is a console.warn() instead of an console.error. Maybe with another util?

[anishmprasad]

Try this

import { action1,action2, … } from '../action'
// component
export default connect(mapStateToProps,{
  action1,
  action2,
  …
})(component);

[aghreed]

I have to agree with @sebcode.

Importing in actions with import * as action from 'actions' is definitely a common pattern, and one that saves a lot of headache when you've got larger actions files. Using the * import not only saves me from a giant import block, but also stops issues of variables being declared in upper scope when you use destructuring assignment.

import { action1, action2, ... } from 'actions'
...
class App extends Component {
...
componentWillMount() {
  const { action1 } = this.props; // <-- action1 is now already defined in upper scope
  action1();
}
...
export default connect(
  mapStateToProps,
  mapDispatchToProps,
)(App));

Now I get that you could export out of your actions files differently or add a little util function (like @sebcode is using) to pull out any non-function values pulled in with import * to solve this - but in my opinion these changes are more disruptive and time consuming than the original issue here of "dude where's my props".

[vhpoet]

@artgillespie any possibility of reverting this? The error is super annoying.

[geminiyellow]

this merge is really a bad idea.

[timdorr]

@markerikson @gaearon What do you think? This seems to be getting a lot of people in trouble with how Babel puts extra properties on modules (default and _esModule, by default). Outside of special casing those properties, which seems like the wrong solution, I think we should just remove this for now.

[markerikson]

Well, on the one hand, it is just a warning. On the other hand, it does seem to clash with how Babel currently works, and our own samples show the usage pattern that leads to that warning.

Given that bindActionCreators appears to skip over non-functions already, I'm okay with dropping the warning.

(I'm also still seeing pros and cons for having filtering for only functions in stuff like compose.)

[betoharres]

I think that at least a warn instead of error message on the console would better suit this situation. When I saw the error msg I expected that my code was broke and things related to the error wasn't working, turns out it's just a warning, actually.

[digitalmaster]

Agree that downgrading to a warning will be better but this should really just be reverted :-/

[timdorr]

Wait, our warning isn't a warning? Hmmm... 🤔

Anyways, I'll look at getting this change reverted shortly. I need to switch laptops, but I'll do that tonight.

[psychobolt]

Running tests with Jest will see it as a console.error:

console.error node_modules\react-native\Libraries\Core\ExceptionsManager.js:71
      bindActionCreators expected a function actionCreator for key 'Actions', instead received type 'object'.

[artgillespie]

OP here... I'll get the revert PR together now. Sorry for all the trouble.

[timdorr]

Reverted and pushed out in 3.7.1. Thanks for the feedback everyone!

[elrumordelaluz]

Wait, our warning isn't a warning? Hmmm... 🤔

@timdorr I tried to say that here 👇

could be helpful if the the warning is a console.warn() instead of an console.error

[gaearon]

Sorry I didn't chime in on this. That's exactly the reason we didn't do this in the first place 😛

[gaearon]

Even filtering __esModule would not work well because people often export constants as named exports from the same file.