Remove "enhancer" reference from applyMiddleware function

[marcreicher]

I could be wrong here, but was reviewing the createStore and applyMiddleware functions, and it seems like there should not be a reference to any "enhancer" within applyMiddleware.

It is my understanding that the applyMiddleware function typically gets called and then passed into createStore as the "enhancer" argument. Within createStore, we then have:

if (typeof enhancer !== 'undefined') {
    if (typeof enhancer !== 'function') {
      throw new Error('Expected the enhancer to be a function.')
    }

    return enhancer(createStore)(reducer, preloadedState)
  }

Specifically, now focusing on the last line:

return enhancer(createStore)(reducer, preloadedState)

Since no enhancer would ever be passed in as a 3rd argument, then it seems unnecessary to include the reference to an enhancer in the applyMiddleware function.

Again, could be missing something here, but it seems including a reference to an "enhancer" within the applyMiddleware function is getting away from the spirit of what the function is trying to accomplish (wrapping the "unenhanced" store with a modified dispatch method).

[markerikson]

The idea of Redux store enhancers is that, in theory, any enhancer can "wrap around" an implementation of createStore, and present its own version of the store API as a result. Multiple enhancers can then be composed together. As an example, you might see something like:

const composedEnhancers = compose(
    applyMiddleware(...middlewares),
    batchedActions(),
    DevTools.instrument()
);

const store = createStore(rootReducer, preloadedState, composedEnhancers);

In order for that to work properly, each enhancer needs to support the full signature of createStore.

[jimbolla]

@markerikson I think this same PR has happened before. Maybe we can add a test around applyMiddleware that fails if this change is proposed.

github feature idea: show rejected PRs that touch a file/line.

[markthethomas]

@marcreicher 👍 👎 💯

[marcreicher]

@markerikson @jimbolla

Alright, I assume you're right, but it's 10:30pm on a Monday and I am not satisfied with your answer. This is going to be long, but I want to step through your example to illustrate my point. You mentioned we might see something like:

const composedEnhancers = compose(
    applyMiddleware(...middlewares),
    batchedActions(),
    DevTools.instrument()
);

const store = createStore(rootReducer, preloadedState, composedEnhancers);

First off, let me just I have not used the batchedActions or DevTools.instrument() enhancers, but I did some research to get an understanding of what they are used for. The one think I am immediately thrown off by here is the reference here to batchedActions(). Correct me if I am wrong, but it seems like you are referencing this module:

https://github.com/mrydengren/redux-batch-middleware

I am confused because this seems like a piece of "middleware" but it is used as an "enhancer" in your "compose" function above.

For my own sanity, I am going to remove it from your composition, and just leave us with:

const composedEnhancers = compose(
    applyMiddleware(...middlewares),
    DevTools.instrument()
);

const store = createStore(rootReducer, preloadedState, composedEnhancers);

I feel like whether we have a composition of 2 enhancers or 10, the problem is materially the same.

Let's also assume we are using the redux-thunk middleware (i.e. we are passing that into our applyMiddleware function). Let's now evaluate:

applyMiddleware([reduxThunk])

I pulled the code from 'redux' and 'redux-thunk' libraries:

const reduxThunk = ({ dispatch, getState }) => next => action => {
    if (typeof action === 'function') {
      return action(dispatch, getState);
    }

    return next(action);
};

function applyMiddleware(...middlewares) {
  return (createStore) => (reducer, preloadedState, enhancer) => {
    var store = createStore(reducer, preloadedState, enhancer)
    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
    }
  }
}

So, I would say this...

applyMiddleware([reduxThunk])

roughly evaluates to this... (I will simply refer to it as "middlewareFunction")

const middlewareFunction = (createStore) => (reducer, preloadedState, enhancer) => {
    var store = createStore(reducer, preloadedState, enhancer)
    var dispatch = store.dispatch

    dispatch = (action) => {
        if (typeof action === 'function') {
          return action(store.dispatch, store.getState);
        }

        return store.dispatch(action);
    }

    return {
      ...store,
      dispatch
    }
}

Alright, now the dev tools instrument. I pulled some code from
https://github.com/zalmoxisus/redux-devtools-instrument/blob/master/src/instrument.js

const instrument = createStore => (reducer, initialState, enhancer) => {

    function liftReducer(r) {
      if (typeof r !== 'function') {
        if (r && typeof r.default === 'function') {
          throw new Error(
            'Expected the reducer to be a function. ' +
            'Instead got an object with a "default" field. ' +
            'Did you pass a module instead of the default export? ' +
            'Try passing require(...).default instead.'
          );
        }
        throw new Error('Expected the reducer to be a function.');
      }
      return liftReducerWith(r, initialState, monitorReducer, options);
    }

    const liftedStore = createStore(liftReducer(reducer), enhancer);
    if (liftedStore.liftedStore) {
      throw new Error(
        'DevTools instrumentation should not be applied more than once. ' +
        'Check your store configuration.'
      );
    }

    return unliftStore(liftedStore, liftReducer);
};

I omitted some of the helper functions that the instrument function uses, but they won't be necessary for the purposes of this exercise.

Let's now return to the enhancer composition:

const composedEnhancers = compose(
    applyMiddleware(...middlewares),
    DevTools.instrument()
);

Or, with my renamed functions, we have:

const composedEnhancers = (...args) => {
    middlewareFunction(instrument(...args))
}

I guess here is where the fun starts happening (says the guy who is writing this at 10:30pm for literally no reason). We actually create the store with this line of code:

const store = createStore(rootReducer, preloadedState, composedEnhancers);

To reiterate from my original PR comment, the createStore method begins:

export default function createStore(reducer, preloadedState, enhancer) {
  if (typeof preloadedState === 'function' && typeof enhancer === 'undefined') {
    enhancer = preloadedState
    preloadedState = undefined
  }

  if (typeof enhancer !== 'undefined') {
    if (typeof enhancer !== 'function') {
      throw new Error('Expected the enhancer to be a function.')
    }

    return enhancer(createStore)(reducer, preloadedState)
  }

Since we provided an enhancer, this is really the only line I care about:

return enhancer(createStore)(reducer, preloadedState)

Using some of my code above, this evaluates to

return middlewareFunction(instrument(createStore))(reducer, preloadedState);

This is kind of ugly, but let me present a simplified version of...

middlewareFunction(instrument(createStore))

...as...

function (reducer, preloadedState, enhancer) {
    var store = instrument(createStore)(reducer, preloadedState, enhancer)
    var dispatch = store.dispatch

    dispatch = (action) => {
        if (typeof action === 'function') {
          return action(store.dispatch, store.getState);
        }

        return store.dispatch(action);
    }

    return {
      ...store,
      dispatch
    }
}

(Just to be clear, the "createStore" referenced in this snippet would represent the plain redux createStore function (that we invoked to actually create our store)).

So now that we have a definition for...

middlewareFunction(instrument(createStore))

...it's time to invoke it. But when we do invoke it, we call

middlewareFunction(instrument(createStore))(reducer, preloadedState);

We are clearly not passing in an enhancer, and so unless I am crazy, our "composed enhancer" never receives an enhancer argument. This brings me back to my original question and the purpose for my pull request. Why does the applyMiddleware method have a reference to an enhancer?

[markerikson]

I appreciate you've spent some time digging through this, but I'm afraid you're still confused by a few things. (Admittedly, I was confused by a couple bits of this too.)

First, by batchedActions I was referring to https://github.com/manaflair/redux-batch , which is indeed another store enhancer.

Second, if you look at the history for the file, you can see when and why that argument was included. It was added in the 3.1.0 release, to help fix an edge case where mixing the "old style" of create the store and the "new style" might create a problem.

See https://github.com/reactjs/redux/releases/tag/v3.1.0 , https://github.com/reactjs/redux/releases/tag/v3.1.1 , https://twitter.com/acdlite/status/692772885186764802 , and #1302 for more details on this.

So, overall, it does appear a bit redundant at this point in time, but I don't see an actual reason to remove it. "If it ain't broke, don't fix it."

[marcreicher]

Ah thank you for clearing that up!