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

Dispatching in a middleware before applyMiddleware completes #1240

Closed
timdorr opened this issue Jan 15, 2016 · 24 comments
Labels
bug
Milestone

Comments

@timdorr
Copy link
Member

@timdorr timdorr commented Jan 15, 2016

Preface: This may or may not be a bug in Redux.

Say you have a middleware that intercepts actions and emits async events to dispatch later on. We do something like this in redux-simple-router:

function middleware(store) {
  history.listen(location => { store.dispatch(updateLocation(location)) })

  return next => action => {
      if (action.type !== TRANSITION) {
        return next(action)
      }
      const { method, arg } = action
      history[method](arg)
    }
}

The key bit is the history listener at the top. The middleware intercepts actions of type TRANSITION and runs a history command (push, for example). That comes back to us via the listener and we dispatch an updateLocation action to update the store with the current location.

Now, let's say you have a little bit of analytics middleware that watches for updateLocation events and tells your analytics service there's a new page view. This works A-OK because store.dispatch above has had the middleware applied to it and will include the analytics middleware in the dispatch.

Here's the problem: That listener fires when you're applying the middleware, so calling store.dispatch is calling the un-middlewared dispatch and our analytics middleware won't see any event.

Should Redux handle this? It might be better handled by the history middleware, or by the analytics middleware, or by a non-middleware approach. But it certainly does end up creating some confusion for users when code operates differently the first time you run it.

@timdorr timdorr added the bug label Jan 15, 2016
@timdorr timdorr mentioned this issue Jan 15, 2016
@yelouafi

This comment has been minimized.

Copy link

@yelouafi yelouafi commented Jan 15, 2016

I encountered the same problem in redux-saga. When a saga dispatches an action at its start, so before the applyMiddleware function returns.

My workaround was to wrap the dispatch inside a Promise. It's a little better than setTimeout, because the dispatch will occur in the current event queue.

function middleware(store) {
  history.listen(location => { 
    Promise.resolve(1).then(() => store.dispatch(updateLocation(location)))
  })

  return next => action => {
      if (action.type !== TRANSITION) {
        return next(action)
      }
      const { method, arg } = action
      history[method](arg)
    }
}
@gaearon

This comment has been minimized.

Copy link
Contributor

@gaearon gaearon commented Jan 15, 2016

Can we fix this?

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Jan 15, 2016

I'm not sure, hence this possibly not being a Redux bug. It's just a weird edge case behavior of the applyMiddleware function. Maybe if JS had some sort of Future concept, but I'm not sure there's a good way.

Maybe someone else has a genius idea to resolve it 😄

@johanneslumpe

This comment has been minimized.

Copy link
Contributor

@johanneslumpe johanneslumpe commented Jan 16, 2016

Just a naive idea: What if we passed a promise into each middleware which gets resolved once the whole chain has been applied? Very similar to what @yelouafi has as a workaround, but standardized:

function middleware(store, afterApply) {
  afterApply.then(() => {
    history.listen(location => store.dispatch(updateLocation(location)))
  })

  return next => action => {
      if (action.type !== TRANSITION) {
        return next(action)
      }
      const { method, arg } = action
      history[method](arg)
    }
}
@rossipedia

This comment has been minimized.

Copy link

@rossipedia rossipedia commented Jan 16, 2016

I don't really think this should be standardized, at least not directly in Redux. There's nothing preventing somebody from creating a store enhancer library that provides this functionality, but it doesn't belong directly in Redux.

@yelouafi

This comment has been minimized.

Copy link

@yelouafi yelouafi commented Jan 18, 2016

@rossipedia
I think you're right. While there may be some ways to fix this directly inside Redux. IMO, It's unnecessary to complicate the library to just handle this edge case.

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Jan 27, 2016

I think we're just going to setTimeout and call it a day. If anyone has a genius idea to solve this, I'm all ears. But I don't think there's a better solution given JS async constructs.

@timdorr timdorr closed this Jan 27, 2016
@gaearon gaearon reopened this Feb 1, 2016
@gaearon

This comment has been minimized.

Copy link
Contributor

@gaearon gaearon commented Feb 1, 2016

If we close this as wontfix, let’s add a test and a doc section explaining what the current behavior is.
Like we did in 24aade4.

@gaearon gaearon mentioned this issue Feb 1, 2016
8 of 8 tasks complete
@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Feb 1, 2016

Yeah, I can do that. Do we want to add a warning to applyMiddleware if dispatch is called while it's running?

BTW, I ended up not using setTimeout in react-router-redux because it broke our server-side rendering (the listener never fired). But that's not to discourage this approach, as it may work for other types of middleware.

@yelouafi

This comment has been minimized.

Copy link

@yelouafi yelouafi commented Feb 1, 2016

just a quick idea; if the dispatch function passed to middlewares could be wrapped in a conditional behavior based on whether or not yet w've completed the middleware application phase

  • if the middleware is already applied dispatch as normal
  • if not yet, put the actions in a queue (could be attached as a field to dispatch middlewareAPI )

after the middleware application dispatch all queued actions

I hope I'm not telling sometheing dumb; just trying to find a solution

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Feb 1, 2016

put the actions in a queue

That would be a neat approach. Any downsides you could think of? I suppose if you dispatch and them immediately query the store, your state won't necessarily be there. Of course, I think practically most people aren't doing that, and that downside is less bad than your dispatch not having any direct means to interact with other middleware.

My PR (#1345) takes the more "not my problem" approach to it, which seems a little hand-wavey to me. I'm fine with not doing that 😄

@yelouafi

This comment has been minimized.

Copy link

@yelouafi yelouafi commented Feb 1, 2016

I can't think of any other downside. More generally, I think any form of interaction at this phase would be meaningless.

Perhaps a simpler solution is the middleware to return a tuple (next->action, startupActions). But that require a change in the API

@gaearon

This comment has been minimized.

Copy link
Contributor

@gaearon gaearon commented Feb 1, 2016

I don't like the queue because it complicates the existing assumption that "when all middleware is sync, actions are flushed immediately and dispatch is guaranteed to be sync". I'd rather throw in this case than make special affordances for this behavior.

@yelouafi

This comment has been minimized.

Copy link

@yelouafi yelouafi commented Apr 2, 2016

Perhaps a simpler solution. Actually createStore dispatches an initial action to reducers to get the initial state. Can we do the same by dispatching the same action to middlewares. This way the middleware can know when the store is created by checking only the first action.

From the sources it seems createStore actually uses the non-enhanced version of dispatch so the action goes directly to reducers. If we use the enhanced dispatch we can route the init action to all middlewares : sort of a lifecycle event

@jimbolla

This comment has been minimized.

Copy link
Contributor

@jimbolla jimbolla commented Dec 19, 2016

I feel like the best option is to fire an INIT_MIDDLEWARE action once all the middleware have been initialized. Then @timdorr's middleware would look like:

import { INIT_MIDDLEWARE } from 'redux'

function middleware({ dispatch }) {
  return next => action => {
    switch (action.type) {
      case INIT_MIDDLEWARE: 
        history.listen(location => { dispatch(updateLocation(location)) })
        return next(action)

      case TRANSITION:
        const { method, arg } = action
        history[method](arg)
        break

      default:
        return next(action)  
    }
  }
}
@lambert-velir

This comment has been minimized.

Copy link

@lambert-velir lambert-velir commented Mar 21, 2017

I like @jimbolla's idea. I've adopted that for now in my configureStore function:

export default function configureStore(rootReducer, initialState = {}, middlewares = []) {

  const store =  createStore(rootReducer, initialState, compose(
    applyMiddleware(...middlewares),
    // https://github.com/zalmoxisus/redux-devtools-extension#1-with-redux
    window.devToolsExtension ? window.devToolsExtension() : x => x
  ));

  // https://github.com/reactjs/redux/issues/1240#issuecomment-268061029
  store.dispatch({type: "@@INIT_STORE"});

  return store;
}
encodeering added a commit to encodeering/conflate that referenced this issue Mar 25, 2017
* middlware that wants to support long running processes - across multiple actions - can be written easier this way

NOTE

* introduces an issue that has been already discussed here: reduxjs/redux#1240
@canvaspixels

This comment has been minimized.

Copy link

@canvaspixels canvaspixels commented Mar 27, 2017

Granted this is completely unrelated to this thread, but why is it called next, and not dispatch?

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Mar 27, 2017

Because it's in a middleware chain and, as far as your middleware knows, it's just going to the next middleware in the chain. You don't know if that's another middleware or the original dispatch (nor should you care). It's a common nomenclature for middleware-like code.

@markerikson

This comment has been minimized.

Copy link
Contributor

@markerikson markerikson commented Mar 27, 2017

@canvaspixels

This comment has been minimized.

Copy link

@canvaspixels canvaspixels commented Mar 28, 2017

Great explanation, thanks @timdorr !

@Lucretiel

This comment has been minimized.

Copy link

@Lucretiel Lucretiel commented May 12, 2017

What if, instead of throwing a warning, or requiring middlewares to listen for a special action, we just have applyMiddleware queue up dispatched actions, and execute them after all the middlewares are installed?

@chajath

This comment has been minimized.

Copy link

@chajath chajath commented Jul 6, 2017

What happens if you close over next here? like:

function middleware(store) {
  return next => {
    history.listen(location => { next(updateLocation(location)) })
    return action => {
      if (action.type !== TRANSITION) {
        return next(action)
      }
      const { method, arg } = action
      history[method](arg)
    }
  }
}
@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Jul 6, 2017

@chajath You would be subscribing to history on every action that passed through. Which would be pretty bad, as your app would slow down and act weirder on every action.

@timdorr

This comment has been minimized.

Copy link
Member Author

@timdorr timdorr commented Oct 23, 2017

This is being warned against in 4.0, so it's at least something you will know to prevent. I consider that sufficient to close this out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.