Skip to content
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

Closed
joonhocho opened this issue Nov 18, 2015 · 26 comments
Closed

Comments

@joonhocho
Copy link

export default function applyMiddleware(...middlewares) {
  return (next) => (reducer, initialState) => {
    var store = next(reducer, initialState)
    var dispatch = store.dispatch
    var chain = []

    var middlewareAPI = {
      getState: store.getState,
      dispatch: (action) => dispatch(action)
    }
    chain = middlewares.map(middleware => middleware(middlewareAPI))
    dispatch = compose(...chain)(store.dispatch)

    return {
      ...store,
      dispatch
    }
  }
}

Looking at the source code above, applyMiddleware creates a new store object and overrides dispatch with newly composed dispatch, and returns the new store.

There are shortcomings of this approach that I have experienced.
Take a look at the following scenario:

compose(
  applyMiddleware(A),
  reduxRouter(...),
  applyMiddleware(B)
)(createStore)

Because of the current implementation of applyMiddleware and compose to enhance createStore,
middleware A and middleware B will not share the same dispatch.
That means dispatch(action) in A and dispatch(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:

  1. Create only one store instance, and do not replace it by composeing createStore.
  2. Enhance it by composing and replacing dispatch function on the only store instance.
  3. Share the store, not dispatch across all middlewares.

This way, there is only one universal store instance, and store.dispatch will always go through the same code path and yield the same outcome regardless of which middleware.

@gaearon
Copy link
Contributor

gaearon commented Nov 18, 2015

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?

  1. Create only one store instance

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.

@joonhocho
Copy link
Author

I was trying to create an auth middleware that redirects user to login page by firing replaceState action when current action.requireAuth is true and state.auth.user is not set.
This does not work because auth middleware's dispatch does not work properly with any of redux-router`s action creators due to the stated reasons from the previous comment.

@gaearon
Copy link
Contributor

gaearon commented Nov 20, 2015

Why not put middleware before Redux Router?

@mikebarnhardt
Copy link

@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 routerDidChangefired I could call ga.push() or whatever. The only way to get it working was to add another middleware after reduxRouter, but you need things like thunk to be first in the middleware chain so that async dispatches can still use pushState.

I agree about creating "too powerful" store enhancers. Do you think the redux-router middleware is too powerful? Should it be included in applyMiddleware() instead?

@joonhocho
Copy link
Author

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.
And any actions created at any point by any middleware always go through the same path, root -> A -> B -> C -> final.
I think it's similar to how the 'flux' works as well in principle. All events going through same action -> dispatch -> store -> view path.

@joonhocho joonhocho reopened this Nov 20, 2015
@joonhocho
Copy link
Author

It was my mistake to accidentally close the issue. I hate the button placement for close issue.

@gaearon
Copy link
Contributor

gaearon commented Nov 21, 2015

I'm currently busy so I'll try my best to get back to this when I'm more available.
Sorry!

@mikebarnhardt
Copy link

Because I felt the reduxRouter(...) middleware was too independent, I went back to using basic react-router and hooking into history.listen(...) for my purposes. I know that it removes the router state from the Redux store, but I haven't run into any issues using history.pushState(...) in my action creators. I know this probably won't work for everyone, though.

@acnovais
Copy link

acnovais commented Jan 6, 2016

Ran into this issue today.
While developing a middleware, one has the impression that by calling store.dispatch the action would go through all the middlewares from the beginning.
This was a bit frustrating to find.

@gaearon
Copy link
Contributor

gaearon commented Jan 6, 2016

It does go through all the middlewares inside a single applyMiddleware call. I don't see an easy way to make it jump to the first middleware chain when there any many such chains. In any case we don't suggest to make too powerful middleware. It's meant for simple transformations and not complex logic.

@acnovais
Copy link

acnovais commented Jan 6, 2016

Yes, that was my current solution.
Aggregate my middlewares in a single applyMiddleware chain.

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?

@gaearon
Copy link
Contributor

gaearon commented Jan 6, 2016

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.

@acnovais
Copy link

acnovais commented Jan 7, 2016

This is amazing! Thanks

@ninjasort
Copy link

Just thought I'd chime in about the composition of applyMiddleware. I'm wondering why it's not written as a .use convention that node and most other middleware chains use..? Would it be nicer to write:

const store = Redux.use([
  thunkMiddleware,
  loggerMiddleware
]).createStore(rootReducer)

vs

const createStoreWithMiddleware = applyMiddleware(
  thunkMiddleware, // lets us dispatch() functions
  loggerMiddleware // neat middleware that logs actions
)(createStore)

const store = createStoreWithMiddleware(rootReducer)

@rossipedia
Copy link

Personally, I don't understand why we can't just supply a list of middlewares to createStore(). Something like:

const store = createStore(
    combineReducers({ ... }),
    initialState,
    [
        thunkMiddleware,
        loggerMiddleware
    ]
);

If you wanted to do all that in one call right now, it's a bit verbose:

const store = (applyMiddleware(
    thunkMiddleware,
    loggerMiddleware
)(createStore))(combineReducers({...}, initialState);

Having to create an entire new createStore function by applying middlewares feels a wee bit overcomplicated (just my $0.02).

@gaearon
Copy link
Contributor

gaearon commented Jan 11, 2016

@rossipedia Can you please show how your proposal works with store enhancers like Redux DevTools?

@ninjasort
Copy link

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:

// reducers
import * as reducers from './reducers';

// middleware
import thunk from 'redux-thunk';
import logger from 'redux-logger';

// initialState
const initialState = {...someState};

const store = Redux.store(reducers, initialState, [thunk, logger]);

@gaearon
Copy link
Contributor

gaearon commented Jan 11, 2016

This is exactly the API we had some time before 1.0 :-).
It had downsides: it's very implicit and it's hard to build extensions like DevTools around it.

@ninjasort
Copy link

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.

import DevTools from '../containers/DevTools';

const store = Redux.store(reducers, initialState, [thunk, logger]);
const enhanced = store([
  DevTools.instrument(),
  DevTools.persistState()
]);

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.

@rossipedia
Copy link

Personally, I don't understand why we can't just supply a list of middlewares to createStore()

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.

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

@rossipedia Same idea, but on store enhancer level: #1294. Hope you like it.

@gaearon
Copy link
Contributor

gaearon commented Jan 28, 2016

I'll close this because the original issue is by design, and the later suggestion can be tracked in #1294.

@mauron85
Copy link

mauron85 commented Nov 4, 2016

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.

const enhancers = compose(
  // let's imagine E1 is redux-loop enhancer transforming effects into actions
  enhancerFactory('E1'),
  applyMiddleware(
    middlewareFactory('M1'),
    middlewareFactory('M2')
  ),
  enhancerFactory('E2'),
  enhancerFactory('E3')
);

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.
http://jsbin.com/quvuqof/edit?js,console

What I'm really trying to achieve can be shown on expected jsbin output for that example.

"[enhancerE1] dispatching action: INCREMENT"
"[middlewareM1] handling action: INCREMENT"
"[enhancerE1] dispatching action: ACTION_FROM_MIDDLEWARE" // this is it!! one enhancer call
"[middlewareM1] handling action: ACTION_FROM_MIDDLEWARE"
"[middlewareM2] handling action: ACTION_FROM_MIDDLEWARE"
"[enhancerE2] dispatching action: ACTION_FROM_MIDDLEWARE"
"[enhancerE3] dispatching action: ACTION_FROM_MIDDLEWARE"
"[middlewareM2] handling action: INCREMENT"
"[enhancerE2] dispatching action: INCREMENT"
"[enhancerE3] dispatching action: INCREMENT"

I know, this will require new createStore implementation. And I'm not saying it should be part of official redux package.

What do you think? Does it makes sense? Will there be any complications with DevTools or other enhancers?

@mauron85
Copy link

mauron85 commented Nov 4, 2016

Test case for above:

http://jsbin.com/zalevi/edit?js,output

@markerikson
Copy link
Contributor

@mauron85 : yeah, I don't think that's exactly feasible, given how each enhancer wraps the next one down. In your example, the applyMiddleware enhancer gets a reference to the store API returned by enhancer E2, and then exposes its own version of the store API. It has no idea that E1 exists. Really, that doesn't even have anything to do with createStore either.

I haven't actually used redux-loop myself, but is there a specific reason why you would want to put that before applyMiddleware?

@mauron85
Copy link

mauron85 commented Nov 4, 2016

is there a specific reason why you would want to put that before applyMiddleware?

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:
https://github.com/redux-loop/redux-loop/blob/0da84e946e2b262f98c836ad310fcd0d80bee94d/modules/install.js#L29

It add effect into currentEffect batch and returns model, which I believe is redux action.

EDIT:
My intention is not just fix my own issue with redux-loop. IMHO it could help eliminate other workarounds. Maybe also this one: FormidableLabs/redux-little-router#36 (comment)

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

No branches or pull requests

8 participants