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

remove initial assignment of `dispatch` in applyMiddleware to make it more expressive #1592

Merged
merged 2 commits into from Apr 9, 2016

Conversation

3 participants
@mz026
Copy link
Contributor

mz026 commented Apr 8, 2016

In the original version, the dispatch local variable in applyMiddleware stands for
different things before and after the compose call.
It'd be more expressive to separate "the original one on store" and "the one with middlewares applied"

separate local variables in applyMiddleware to make it more expressive
In the original version, the `dispatch` local variable in applyMiddleware stands for
different things before and after the `compose` call.
It'd be more expressive to separate "the original one on store" and "the
one with middlewares applied"
@aweary

This comment has been minimized.

Copy link
Contributor

aweary commented Apr 8, 2016

I think that accessing dispatch as a method of store via store.dispatch is pretty clear, providing the same essential information that storeDispatch does. Is there another advantage of localizing it to storeDispatch?

@mz026

This comment has been minimized.

Copy link
Contributor

mz026 commented Apr 9, 2016

When first reading the code, I was pretty confused by middlewareAPI.dispatch: why wrapping dispatch into a function that simply returns directly without doing nothing else? That is, why not pass store.dispatch to it like this:

var middlewareAPI = {
  getState: store.getState,
  dispatch: store.dispatch
}

or like this:

var middlewareAPI = {
  getState: store.getState,
  dispatch
}

Then I realized the purpose of wrapping dispatch in the middlewareAPI was that the local variable dispatch changed after this line

dispatch = compose(...chain)(store.dispatch)

so we were actually using the dispatch with middleware in middlewareAPI.dispatch via closure.
However, the assigning statement var dispatch = store.dispatch shadowed this purpose, and by separating them can make this purpose clearer.

refactor to make `applyMiddleware` clearer
pass `store.dispatch` to `compose`, not through another local variable

@mz026 mz026 changed the title separate local variables in applyMiddleware to make it more expressive remove initial assignment of `dispatch` in applyMiddleware to make it more expressive Apr 9, 2016

@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Apr 9, 2016

It used to be this way some long time ago. Was changed later to appease Flow but we never merged Flow support anyway so I suppose there's no point to keeping it this way. Let's get this merged.

@gaearon gaearon merged commit 9496fd7 into reduxjs:master Apr 9, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@gaearon

This comment has been minimized.

Copy link
Contributor

gaearon commented Apr 20, 2016

FWIW this broke middleware that relied on dispatch during initialization.
We will be deprecating this pattern but for now, I reverted this in #1647.

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