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

Make middleware API dispatch pass through all call arguments #2560

Merged
merged 1 commit into from
Aug 15, 2017

Conversation

Asvarox
Copy link
Contributor

@Asvarox Asvarox commented Aug 15, 2017

As discussed in #2501, there's a bit of inconsistency as you can call store.dispatch with multiple arguments just fine, but in context of middlewares (say, thunk) only the first one is passed through. This PR tries to fix that.

@timdorr
Copy link
Member

timdorr commented Aug 15, 2017

This should be fine. It definitely isn't breaking. Middleware can still block other args from being passed through to next(), but it at least opens up the possibility to do so.

@timdorr timdorr merged commit e2e9648 into reduxjs:master Aug 15, 2017
@gaearon
Copy link
Contributor

gaearon commented Aug 15, 2017

I think the reason we didn't do this initially is because we wanted to make it easier for all middleware to be compatible. Now we need to either force all middlewares to pass it through or live with a situation where adding a seemingly harmless middleware can break the other code in a somewhat non-obvious way.

@gaearon
Copy link
Contributor

gaearon commented Aug 15, 2017

As for the use case in #2501, I don't quite see what prevents the user from passing an array: dispatch([myThunk, arg1, arg2]).

@timdorr
Copy link
Member

timdorr commented Aug 15, 2017

Middleware changes the definition of what dispatch's args can be anyways. So whether you use an array or allow multivariate functions, it's really no different because the function signature is changed. It's odd for a loosely-designed API to have a restriction thrown in there.

@markerikson
Copy link
Contributor

markerikson commented Aug 16, 2017

@timdorr : I'd disagree with the phrase "changes the definition of what dispatch's args can be", at least to some extent. Yes, they allow you to pass in something other than a plain action, but the working assumption thus far has been that there's only one value being passed through, much like a promise pipeline.

As you said, allowing multiple args here shouldn't break any existing behavior, but given that all existing middlewares assume there's only one arg, I'm not sure about the actual benefit. (This is closely related to the very long discussion in #1813 .)

@gaearon
Copy link
Contributor

gaearon commented Aug 17, 2017

IMO allowing this will benefit a tiny minority of users (who already have a fine workaround) but will add cost to every middleware author who will eventually be asked to support this. This change, to be successful, will have to trigger a cascade of changes in every library that delegates to dispatch. And it makes Redux harder to type (which is already hard). In my eyes the cost outweighs the benefit.

seantcoyote pushed a commit to seantcoyote/redux that referenced this pull request Jan 14, 2018
@@ -24,7 +24,7 @@ export default function applyMiddleware(...middlewares) {

const middlewareAPI = {
getState: store.getState,
dispatch: (action) => dispatch(action)
dispatch: (...args) => dispatch(...args)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why not just return dispatch?

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey! Can someone have an explanation for this change?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FranciscoMSM This particular change is to allow middleware to change the arguments of dispatch and pass that through.

The reason for the extra function is to remove direct access to dispatch so middleware doesn't try to attach properties or other hidden metadata to it. Each middleware gets its own unique copy of dispatch this way, and there is less chance of interference between middlewares.

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

Successfully merging this pull request may close these issues.

6 participants