Skip to content
This repository has been archived by the owner on Dec 15, 2018. It is now read-only.

Dispatch in middleware does not use store-enhancer dispatch #36

Closed
dpwrussell opened this issue Aug 23, 2016 · 17 comments
Closed

Dispatch in middleware does not use store-enhancer dispatch #36

dpwrussell opened this issue Aug 23, 2016 · 17 comments
Labels

Comments

@dpwrussell
Copy link

dpwrussell commented Aug 23, 2016

Hi,

I am experimenting with this:

Your custom middleware can intercept this action to dispatch new actions in response to URL changes.

I have written a barebones middleware for routes with auth in their result. Like so:

export const auth = store => next => action => {

  if (
    action.type === LOCATION_CHANGED &&
    action.payload &&
    action.payload.result &&
    action.payload.result.auth
  ) {

    const loggedIn = selectors.isLoggedIn(store.getState());

    if (!loggedIn) {
      setTimeout(() => {
        store.dispatch({
          type: PUSH,
          payload: '/login'
        });
        // Other dispatch which works perfectly
        // store.dispatch(fetchVocabsIfNeeded());
      }, 3000);
    }
  }
  return next(action);
};

The setTimeout is just to simulate an async fetch that I would usually dispatch to the server to see if I had an active session. If not, then I want to go to the login page.

This seems to generate a ROUTER_PUSH action with a payload of '/login'. The route does not change and a fragment for '/login' is not displayed. However, if I dispatch that same action from an onClick on my App component, the result is a ROUTER_LOCATION_CHANGE with the payload of pathname, key, route, params, etc, and it works fine.

I am not experienced with redux middleware and suspect that this has something to do with the middleware chain and where redux-little-router sits in that chain, but I'm unsure how to address this. Should this be possible?

Cheers

@dpwrussell
Copy link
Author

I created my store like so:

export default function configureStore(initialState = undefined) {
  const enhancer = compose(

    createStoreWithRouter({
      routes,
      pathname: location.pathname,
      basename: ''
    }),

    // Middleware
    applyMiddleware(
      thunkMiddleware,
      auth
    ),

    DevTools.instrument()
  );

  const store = createStore(
    reducers,
    initialState,
    enhancer
  );

@dpwrussell dpwrussell changed the title Dispatching PUSH in middleware Dispatch in middleware does not use store-enhancer dispatch Aug 23, 2016
@dpwrussell
Copy link
Author

dpwrussell commented Aug 23, 2016

Ok, I dug a little more. The problem is that the store.dispatch in the middleware does not use the store enhancer's dispatch function. So actions dispatched in middleware will go direct to the store's dispatchmethod which explains why it works for other things, but not PUSH etc. react-little-router's enhancer would have intercepted PUSH if it was in the chain.

@tptee
Copy link
Contributor

tptee commented Aug 24, 2016

Thanks for reporting this! I'll be investigating this tomorrow 👍

@dpwrussell
Copy link
Author

Actually, it's even worse than I thought. Middleware should always be the first enhancer and I jsut realised that when fiddling around to get it to work the above is the wrong order.

With the correct order of applyMiddleware then createStoreWithRouter, the middleware will never receive LOCATION_CHANGED events at all.

The fix for this isn't that complicated, but it is a bit messy. I've been having a look at the redux documentation and while it doesn't say that you shouldn't dispatch actions from within an enhancer, I'm beginning to think it isn't a good idea because unlike the middleware, it does not start again from the beginning of the chain.

The only way I could fix this was to split redux-little-router into two parts. I now have a middleware and a store enhancer. This also meant that I configured the history, basename and matcher in my configureStore function as these are passed into both the middleware and enhancer.

Because middleware is designed with this exact problem in mind, it works. If you are interested in this solution I am happy to write it up or submit a PR.

@tptee
Copy link
Contributor

tptee commented Sep 1, 2016

Middleware should always be the first enhancer

Not always true (wish it were that easy!). See where redux-loop ran into this same issue:
redux-loop/redux-loop#28
redux-loop/redux-loop#39

This issue appears to be a pretty serious (and intentional) limitation of Redux:
reduxjs/redux#1240
reduxjs/redux#1051

...which really puts library authors in a tight spot. Older versions of this library required you to install both a store enhancer and a middleware, which definitely hurts usability and expands boilerplate.

Going to hack on a solution as soon as I can, would love feedback when I have something ready!

@tptee
Copy link
Contributor

tptee commented Sep 1, 2016

Also had a thought: in your example, can you try calling next instead of store.dispatch just to see what happens?

@tptee
Copy link
Contributor

tptee commented Sep 10, 2016

I can't figure this out for the life of me! 😒 The only solution I know of is to reintroduce the routerMiddleware, but as an optional piece to allow dispatching router events in middleware.

@caesarsol
Copy link

caesarsol commented Sep 10, 2016

Hello, I don't know if this makes sense, bit I would use something else to dispatch a second action when receiving a first one, and that's a Saga from redux-saga (or maybe something from redux-loop if I understand that project correctly).

So my question is: are you sure this is a redux-little-router problem?

(Still I'm curious too if using next() instead of dispatch() in @dpwrussell 's code could work. @tptee which one do you use from your enhancer?)

@dpwrussell
Copy link
Author

dpwrussell commented Sep 13, 2016

@tptee Hi, I finally made time to go and do the investigation you asked for.

Doing next instead of store.dispatch does not work. I wasn't really expecting it to, because the order that the enhancers are dispatched is left-to-right as they are in the compose statement.

I double checked this by adding this enhancer:

const testEnhancer = createStore => (reducer, preloadedState, enhancer) => {

  const store = createStore(reducer, preloadedState, enhancer);

  const dispatch = action => {
    console.log('testEnhancer dispatch', action);
    return store.dispatch(action);
  }

  return { ...store, dispatch };
};

Which is composed like so:

  const enhancer = compose(

    routerEnhancer,

    applyMiddleware(
      thunkMiddleware,
      auth
    ),

    testEnhancer,

    // Browser addon redux tools (develop mode only)
    window.devToolsExtension ? window.devToolsExtension() : f => f
  );

As expected, testEnhancer logs the next(action) call that I make from my middleware. If the testEnhancer is moved before applyMiddleware in the composition, then it is not.

It is my belief that actions should only be dispatched from middleware.

@philipyoungg
Copy link

The problem happens because with dispatch an action in store enhancer. It didn't follow the apply middleware generated dispatch action.

We can fix this by separating it to two parts: middleware and store enhancer.

Adding the dispatch 'ROUTER_PUSH' to part of middleware should've fixed the problem.

@dpwrussell
Copy link
Author

dpwrussell commented Sep 29, 2016

@philipyoungg Separating the enhancer and middleware does fix the problem as I commented in an earlier post in this thread.

@philipyoungg
Copy link

philipyoungg commented Sep 29, 2016

@dpwrussell @tptee I think I know the problem now.
I've read the codes and realized that there are 2 store: The first one is router store, and the second one is store that generated from applyMiddlewares.

Both have different dispatch method.

On link.js, there's this happens when we dispatch a path payload

if (router) {
    router.store.dispatch({
      type: replaceState ? REPLACE : PUSH,
      payload: location
    });
  }
};

either replace or push, it will do this (from wrap-dispatch)

export default (store, history) => action => {
  switch (action.type) {
  case PUSH:
    history.push(action.payload);
    return null;
  case REPLACE:
    history.replace(action.payload);
    return null;
  case GO:
    history.go(action.payload);
    return null;
  case GO_BACK:
    history.goBack();
    return null;
  case GO_FORWARD:
    history.goForward();
    return null;
  default:
    // We return the result of dispatch here
    // to retain compatibility with enhancers
    // that return a promise from dispatch.
    return store.dispatch(action);
  }
};

…which subsquently will dispatch ROUTE_LOCATION_CHANGED type on the correct store. That's why ROUTER_PUSH never logged as an action, but ROUTE_LOCATION_CHANGE logged correctly from middleware.

history.listen(newLocation => {
      /* istanbul ignore else */
      if (newLocation) {
        store.dispatch(locationDidChange({
          location: newLocation, matchRoute
        }));
      }
    });

the hacky solution is to export router.store.dispatch API to be accessible from redux-little-router, not only inherited from React Context

the non hacky solution is to rewrite the library with only one dispatch action: ROUTE_LOCATION_CHANGED

@tptee
Copy link
Contributor

tptee commented Sep 30, 2016

I think I'm going to give up and add the middleware again. I hate that there's no other way to solve this!

reduxjs/redux#1702 might make this possible in Redux 4, but it'll also make other things more difficult 😬

@tptee
Copy link
Contributor

tptee commented Sep 30, 2016

PR for this: #96

Added a test that confirms that your custom middleware can dispatch router actions!

@joshdcomp
Copy link

How we doing on this? Dispatching in middleware is really important for async actions like ajax form submission handling via redux-saga

@dpwrussell
Copy link
Author

@joshdcomp There is #96, but I added some comments to it about other aspects of accessing the store which are problematic which you might be interested in also.

@tptee
Copy link
Contributor

tptee commented Oct 7, 2016

Fixed in v11.0.0! Huzzah!

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

No branches or pull requests

5 participants