Middleware is broken

[benoneal]

Bug

An important middleware pattern has been broken. IMO the majority of middleware solutions encapsulate their own concerns and are independent of the existence of other middleware. A change was made recently to break a pattern that only caused a minor issue for a specific subset of middleware: those that exhibit different behaviour when interacting with other middleware during the applyMiddleware initialisation path.

Here are some examples:

// Make state aware of browser media queries
const mediaQueries = (mediaQueries = defaultMediaQueries): Middleware => store => {
  if (typeof window !== 'undefined') {
    const reverseMap = createReverseMap(mediaQueries)

    const handleMediaChange = ({media, matches}: MediaChangedActionPayload) =>
      store.dispatch(mediaChanged(reverseMap[media], matches))
  
    const addMatchListener = (media: string) => {
      const match = window.matchMedia(media)
      match.addListener(handleMediaChange)
      return match.matches
    }
  
    values(mediaQueries).forEach(media => 
      handleMediaChange({media, matches: addMatchListener(media)}))
  }

  return next => (action: any) => next(action)
}

Another example:

// Make state aware of user adblockers
const adBlockDetection: Middleware = store => {
  if (typeof document !== 'undefined' && !document.getElementById('XMpADSwgbPUC')) {
    store.dispatch({type: AD_BLOCKER_DETECTED, payload: true})
  }
  return next => (action: any) => next(action)
}

Another example:

// Make state aware of socket connectivity and allow synchronisation of actions
const socketMiddleware = ({actionCreator, url, dataSync}) => store => {
  const socket = new WebSocket(url.replace(/(http|https)/, 'ws'))

  socket.addEventListener('message', ({data}) => {
    store.dispatch(JSON.parse(data))
  })

  socket.addEventListener('open', () => {
    socket.send(syncCreator(CONNECT_TO_DOMAIN, store.getState(), null, socket))
    store.dispatch(actionCreator({connected: true}))
  })

  socket.addEventListener('close', reason => {
    store.dispatch(actionCreator({connected: false}))
  })

  return next => action => {
    if (dataSync.hasOwnProperty(action.type)) {
      socket.send(syncCreator(action.type, store.getState(), action.payload, socket))
    }
    return next(action)
  }
}

What is the current behaviour?

The following error is thrown during applyMiddleware, essentially breaking the application:

Uncaught Error: Dispatching while constructing your middleware is not allowed. Other middleware would not be applied to this dispatch

Refactoring the above middleware to be closed over by the next => function (like below) makes no difference (but is obviously not a viable solution either).

// Still broken:
const mediaQueries = (mediaQueries = defaultMediaQueries): Middleware => store => next => {
  // ...middlewareCodeGoesHere...
  return (action: any) => next(action)
}

What is the expected behavior?

Do not break the app. Log a warning if you want, so I can ignore it, because in 100% of my use cases I do not care if other middleware can handle that dispatch. I strongly suspect that there are (or should be) very few legitimate reasons why any dispatch from within a middleware context should ever interact with any other middleware in the stack, and if it did, I would consider that a code smell (it would place implicit order dependencies on middleware at the very least).

Which versions of Redux, and which browser and OS are affected by this issue? Did this work in previous versions of Redux?

Worked until this PR.

[markerikson]

The middleware init process was explicitly changed in 4.0 in the PR you linked. This was a deliberate design decision, and intended to resolve #1240.

I'm not sure I have an immediate suggestion for your use case, other than to try changing your middleware's logic to wait for some manually-dispatched "store is ready" action. You could also consider staying with 3.x instead.

[benoneal]

@markerikson Yeah I've seen that issue. The key problem that the new design "solves" is this:

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.

This is IMO, an implementation detail that the application developer should handle, not something that should break how most middleware works. With a warning instead of throwing an error, the application developer can identify the issue, and refactor their own middleware to address it.

In its current state, all existing middleware, including 3rd party middleware, that uses this internal dispatch pattern (basically anything that needs to instantiate listeners on anything), is broken.

Staying with 3.x isn't an option for us unfortunately, as we use Typescript, and only 4.x has the correct typings, and in general, we'd rather not be stuck with a legacy version of a core part of our application logic even if typings weren't an issue.

And in my opinion, even the error message itself hints at how unimportant it really is: the "problem" is that Other middleware would not be applied to this dispatch, which is very rarely actually desired, and so far, has only been an issue for one specific use-case, where analytics middleware depended on actions dispatched from a router.

It's important to note, the "fix" in redux right now breaks the code it was intended to fix too. It breaks the router middleware. Now authors of that middleware (and all other similar middleware) need to release 4.x compatible versions of their libs, which now need to expose the implementation details of a manually dispatched "store ready" action, which the consumer must implement.

Use of middleware libraries has changed from simply importing the package, and inserting into the middlewares array, to now additionally require app authors to find some hacky way to dispatch an arbitrary "store ready" action some time after the store has finished being created, but before it is being used. And now ALL their middleware must hopefully adhere to the same action type, otherwise app authors will need to dispatch multiple "store ready" actions, with different types matching whatever API the middleware authors implemented.

This is a real mess, and the "cure" is clearly way worse than the "disease" in this case.

[timdorr]

It's still possible to make every one of your examples work. Just dispatch after applyMiddleware. Perhaps in some sort of setupMiddleware(store) function or a store enhancer.

The Error was intentionally introduced as the alternative is to allow users to hit an edge case that has a very non-obvious cause (unless you're familiar with the internals of applyMiddleware). Redux takes the stance of preventing user error whenever possible, so this isn't going away.

One alternative might be some sort of callback the middleware can register as a sort of afterApply event. Then you would be guaranteed to be dispatching at the correct time, after everything is applied and in order. That would be interesting to think through.