Add a separate enhance store, to allow better async dispatch

[claudiuandrei]

When we have two store enhancers that want to dispatch async, then we have no way to dispatch correctly as each enhancer has access only to it's own dispatch. Ex applyMiddleware and redux-loop.

This provides this option by replacing the third parameter of the store enhancer with a async (scheduled on next event loop) dispatch from the top.

I removed the ehancer from the createStore, and provided a method to actually wrap the final store. This simplifies the implementation.

[claudiuandrei]

@gaearon @lukewestby What do you guys thing, I looked into this because of redux-loop, this should solve the problem of what to place first into the enhancers.

It still does not have tests, so it is just for review, I'll push the tests in the next few days.

[gaearon]

Thanks for the PR! I’m very hesitant about expanding the public API. I think #1576 might solve those problems but let’s keep this open for discussion.

[claudiuandrei]

#1584 is not solving the same problem, it just makes sure that the @init is dispatched through the whole chain of enhancers, but then the enhancers themselves don't have access to the final store.

The reason why the enhancers might need access to the final store is to allow async dispatch. The best example with this is when using redux-loop and applyMiddleware together. They can both do async dispatch, but in the current implementation, they only have access to their own dispatch so only the very top enhancer will dispatch properly.

This is trying to give access to the top dispatch method to every enhancer, and forces the dispatch to be async in order not to interfere with the current dispatch cycle.

@gaearon I was looking for ways to do this without actually changing the API, but I'm not sure how it can be done as I did my best to keep the impact to a minimum. I actually hope there is a better implementation for this.

[claudiuandrei]

By the way, thanks for looking into this

[gaearon]

The best example with this is when using redux-loop and applyMiddleware together.

Can you give me more details about this use case?

[claudiuandrei]

The reason I looked into this is because of this bug:
redux-loop/redux-loop#28

This is an example that cannot work with the current implementation:

import { createStore, applyMiddleware, compose } from 'redux'
import { install } from 'redux-loop'
import thunk from 'redux-thunk'

const firstAction = () => ({
  type: 'FIRST_ACTION_WITH_EFFECT'
})

const secondAction = () => ({
  type: 'SECOND_ACTION_WITH_EFFECT'
})

const thirdAction = () => ({
  type: 'THIRD_ACTION'
})

const triggerSyncAction = () => {
  return new Promise((resolve) => {
    setTimeout(() => {
      resolve(thirdAction())
    }, 1000)
  })
}

const thunkAction = () => {
  return dispatch => {
    setTimeout(() => {
      dispatch(secondAction())
    }, 1000)
  }
}

const triggerThunkAction = () => {
  return new Promise((resolve) => {
    setTimeout(() => {
      resolve(thunkAction())
    }, 1000)
  })
}


function rootReducer(state, action) {
  switch(action.type) {

  case 'FIRST_ACTION_WITH_EFFECT':
    return loop(
      state.set('FIRST_ACTION_WITH_EFFECT', true),
      Effects.promise(triggerThunkAction),
    );

  case 'SECOND_ACTION_WITH_EFFECT':
    return loop(
      state.set('SECOND_ACTION_WITH_EFFECT', true),
      Effects.promise(triggerSyncAction),
    )

  case 'THIRD_ACTION':
    state.set('THIRD_ACTION', true),

  default:
    return state;
  }
}

const enhancer = compose(
  install(),
  applyMiddleware(thunk)
)

const store = createStore(
  rootReducer,
  enhancer
)

store.dispatch(firstAction())

Solution with the current implementation of redux is to use only one library to dispatch async and put it at the top all the time, like the recommendation for apply middleware. That is not a bad thing as it forces you to be consistent.

Another solution is strip the enhacer out, letting people to build their own enhancers, which actually removes api surface. You can still do:

compose(
  install(),
  applyMiddleware(thunk)
)(createStore)

or

enhance(createStore)(compose(
  install(),
  applyMiddleware(thunk)
))

What do you think of just removing the enhancer as a third parameter and have this as a separate module for people who need top dispatch? There is no reason to have that logic there when you can simply wrap it, but it adds the overhead of nested enhancers.

[gaearon]

I don’t quite understand what you propose there. Isn’t this pretty much what the API has been all along before #1294?

Can you help me understand why something like

const enhancer = compose(
  applyMiddleware(thunk),
  install()
)

doesn’t work for you?

[claudiuandrei]

Because the redux-loop does not have access to the top store dispatch, so the action triggerThunkAction will not go through the middleware and will never be triggered. So in the current situation, only the top enhancer will have access to the final store dispatch.

Yes, that is one thought to go back to before #1294, as the current API allows nested enhancer which makes it kind of complicated. This change will allow to use the 3rd parameter of the ehancer to pass the top store dispatcher down the ehancer chain, without further complications to the API as in the pull request.

[gaearon]

Yes, that is one thought to go back to before #1294, as the current API allows nested enhancer which makes it kind of complicated.

I don’t understand what you mean by nested enhancer. #1294 just changed how an enhancer is applied, but there’s always only one enhancer. (compose turns many enhancers into one in both cases.)

[gaearon]

I also don’t really see why redux-loop wants to be compatible with the middleware system. Isn’t the whole point that it manages effects returned from reducers, whereas with the middleware model, middleware executes those side effects during the action dispatch? I can’t quite see why you’d want to mix these models, and why you’d want redux-thunk there.

[claudiuandrei]

Yup, sorry, disregard the last comment, my bad, it is not nested, but initially I wanted to update this line.
https://github.com/reactjs/redux/blob/master/src/createStore.js#L39 and the fact that the third parameter was the enhancer made it harder.

I will close the pull request, I agree that we should not use them both. Maybe this should be just a documentation to say, that you should only use only one async store enhancer and put that at the top.

Thanks.

[gaearon]

Yeah, we don’t really have good docs on enhancers as we don’t really have best practices either, and people still experiment a lot with them so wouldn’t want to restrict that. Maybe some alternative pattern could help, e.g. install() could take middleware as an argument and treat it in its own way.

[lukewestby]

This originally came not in the context of using more than one effects system, but with redux-loop hiding react-router-redux's actions from its middleware (just as @claudiuandrei already linked to, redux-loop/redux-loop#28). The issue is that both applyMiddleware and install return a modified dispatch, and redux-loop happens to need to call dispatch on its own on occasion. If install is composed into the enhancer before applyMiddleware then redux-loop will call a version of dispatch that was never modified by applyMiddleware and any async dispatching it does will not flow through the middleware at all. This manifested itself in the case of react-router-redux by allowing routing actions to work when dispatched manually but not when dispatched as the result of an effect, because redux-loop was passing them to a pre-applyMiddleware version of dispatch.

I think for now I'm comfortable recommending that install just be the first thing in the compose args since I've got an idea about how to get effects out of the state and not modify getState which has the same concern as dispatch but in the opposite direction. It'll be bad news if anyone ends up with two enhancers that both modify and call dispatch internally, but I don't think that's likely enough to worry about right now.