-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Shortcomings of the current applyMiddleware and composing createStore. #1051
Comments
I think we need to discourage "too powerful" store enhancers that introduce such problems. It's not nice that people start having two middleware chains before and after the Redux Router. This really complicates Redux and isn't quite how I imagined it would be used. Rather than allowing this use case, I would like to better understand why you need two different middleware chains. Is this a workaround for some deeper issue?
AFAIK this doesn't work with the DevTools. They want there to be two stores: the "real" store with DevTools state and the "facade" store that your app sees and interacts with. This was the design constraint of the store enhancers as they are now. |
I was trying to create an auth middleware that redirects user to login page by firing |
Why not put middleware before Redux Router? |
@gaearon When you put a middleware before Redux Router you can't see Redux Router's dispatches. I ran into a similar issue when trying to add an analytics middleware so that when the I agree about creating "too powerful" store enhancers. Do you think the redux-router middleware is too powerful? Should it be included in |
As @mikebarnhardt said, when you have a middleware chain A -> B -> C, B can listen to actions that A passes on, but cannot send any actions to A. Whenever B sends any action it will always start from C. At first without looking at the source code, I expected it to work like the following: Root dispatch -> A -> B -> C -> Final dispatch. |
It was my mistake to accidentally close the issue. I hate the button placement for close issue. |
I'm currently busy so I'll try my best to get back to this when I'm more available. |
Because I felt the |
Ran into this issue today. |
It does go through all the middlewares inside a single |
Yes, that was my current solution. I'm using middlewares for transformations, API calls, routing and flow control. I don't think I'm having very complex logic. Maybe the flow control is the most complex: the idea is to allow an action to reach the store based on the current state. Do you think this makes sense? |
Yeah. You might be interested in https://github.com/yelouafi/redux-saga which also allows this in a generic way. |
This is amazing! Thanks |
Just thought I'd chime in about the composition of
vs
|
Personally, I don't understand why we can't just supply a list of middlewares to
If you wanted to do all that in one call right now, it's a bit verbose:
Having to create an entire new |
@rossipedia Can you please show how your proposal works with store enhancers like Redux DevTools? |
Is there ever a point at which we don't want to combineReducers? In that case @rossipedia, we could go one step further and have:
|
This is exactly the API we had some time before 1.0 :-). |
Should the API be designed around the extension developer experience though? Wouldn't it make more sense to have a somewhat implicit high-level API for app developers? You could still use store enhancers the same way, but maybe in an array.
edit: I also just briefly looked at redux-devtools, and it's somewhat difficult to understand quickly. This might not be the best way to address it, but it's still pretty confusing to understand as is. |
Wow... I came off quite condescending there, didn't I. Apologies, @gaearon I'm not familiar enough with DevTools to comment on whether or not it's feasible, actually. I'll go ahead and read the middleware docs again, as I seem to remember they go over the rationale quite thoroughly. |
@rossipedia Same idea, but on store enhancer level: #1294. Hope you like it. |
I'll close this because the original issue is by design, and the later suggestion can be tracked in #1294. |
Hi there. I know @gaearon opinion about middleware not being "too powerful". But there are some cases, where there is difficult to do things other way. I can give some examples, but leave this for now. In vanilla redux applying middlewares and using basic enhancers works just great. But there are some enhancers like redux-loop (version 2), where this classic enhancer execution order is not enough. Redux-loop basically allows reducers to dispatch special kind of actions (effects) which are transformed to normal actions in redux-loop store enhancer. It works great, but only to the moment you want to dispatch those special actions (effects) in middleware. It just won't work, because of enhancer execution order. Let imagine following setup.
and you dispatch redux-loop action (effect) in middleware M1. It will be handled by M1,M2,E2,E3 but not E1. And this is problem, because this effect won't get transformed into actual action. I would like dispatch action in middleware that will be processed with all enhancers not just those who are registered after the middleware. I've created jsbin example that is showing current behaviour. It prints output to jsbin console. What I'm really trying to achieve can be shown on expected jsbin output for that example.
I know, this will require new What do you think? Does it makes sense? Will there be any complications with DevTools or other enhancers? |
Test case for above: |
@mauron85 : yeah, I don't think that's exactly feasible, given how each enhancer wraps the next one down. In your example, the I haven't actually used |
yeah, it any other case I would not work. I believe because it's main purpose is to translate redux-loop effects into actions. All the magic is done in this redux-loop enhancer: It add effect into currentEffect batch and returns model, which I believe is redux action. EDIT: |
Looking at the source code above,
applyMiddleware
creates a newstore
object and overridesdispatch
with newly composed dispatch, and returns the newstore
.There are shortcomings of this approach that I have experienced.
Take a look at the following scenario:
Because of the current implementation of
applyMiddleware
andcompose
to enhancecreateStore
,middleware A and middleware B will not share the same
dispatch
.That means
dispatch(action)
in A anddispatch(action)
in B will go through different code paths and may yield completely different outcomes, and it's confusing and difficult to debug.Also, there is no way for middleware A to dispatch any reduxRouter actions such as
pushState
,replaceState
, etc as those actions will never go through reduxRouter middleware.My proposed solution would be:
store
instance, and do not replace it bycompose
ingcreateStore
.dispatch
function on the onlystore
instance.store
, notdispatch
across all middlewares.This way, there is only one universal
store
instance, andstore.dispatch
will always go through the same code path and yield the same outcome regardless of which middleware.The text was updated successfully, but these errors were encountered: