reduxjs / redux

A JS library for predictable global state management
https://redux.js.org
MIT License
60.88k stars 15.27k forks source link

ActionCreators shouldn't have dispatch method #199

Closed quangbuule closed 9 years ago

quangbuule commented 9 years ago

Take a look at Jing's slide about benefit of Flux: Benefit of Flux

What we just introduced to the Redux users: ActionCreators can dispatch other other actions, which is Flux anti-pattern.

Then users will have some code like this: Bad code

Which is not pure, hard to debug, and hard to tell to other team members (or new member) to follow up.

s-lambert commented 9 years ago

An ActionCreator dispatching multiple different actions is pretty normal in Flux for asynchronous operations. I think when it says nested updates there it's more akin to having stores dispatch actions in response to other actions.

gaearon commented 9 years ago

I don't think Jing talked about action creators. I interpreted this as referring to “stores shouldn't fire actions” and “there shouldn't be dispatch inside dispatch” (really, the same thing). This is precisely what Flux enforces.

What we just introduced to the Redux users:

What do you mean? We had this functionality from the beginning. Otherwise it's really hard to do async workflows. That said, I'm happy to explore the alternative expressive ways of performing async / multiple actions.

matystl commented 9 years ago

It would be nice to have also actionCreators as pure function. But somewhere you have to make side effects. Redux take away need to modify state by stores(reducers) and therefore making stores pure functions. Same is done for action creators as they don't have to call dispach by themself. My main need for async dispach(or promis dispaching) is now ajax calls. This could be push more to edge of system away from actionCreator to some middlewere or to higher order store.

Example to clarify my idea:

fetchData(someData) {
  return (dispach) =>{
    dispach({type: FECHING_DATA, status: 'started', ...})
    makeAjaxCall(...someOptionsForAjax...)
      .then(data => dispach({type: FECHING_DATA, status: 'done', data}))
      .catchError(error => dispach({type: FECHING_DATA, status: 'error', error}) 
  }
}

fetchData(someData) {
  return {
    type: MAKE_AJAX_CALL,
    url: ...
    actionData: {
       type: FECHED_DATA,
       someOtherDateThatWillBeMergedIn,
       //status, data, error will be merged here by HOS/middlewhere
    }  
  }
}

Advantages:

This idea is similar to relay that components(here actionCreators) should not fetch data rather specify what they want and relay(HOS/middlewhere) will take care of fetching.

And for whole topic of removing dispach from actionCreator i don't buy it. Even if you cover some ideas for async dispach with declarative approch(without need of dispach), we should not limit users if thay want be in control of dispaching manualy.

Would love to hear thoughts on this idea.

gaearon commented 9 years ago

We are removing built-in support for thunk middleware from core in 1.0. It is a separate package now (and as a middleware needs to be applied explicitly).

I think we can close this because thunk middleware is now not in any kind of priveleged position.