-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Update middleware api #213
Conversation
Discovered a subtle and confounding bug with composeMiddleware where it only works if dispatch is the last argument. It did not combine multiple middlewares into a single middlewares as advertised; it only combined multiple middlewares with dispatch to create a new dispatch. This didn't come up earlier because the codebase never happened to use it in the former way. This commit fixes the issue and adds a test for it.
Thanks for tackling this. I was shuffling this around in my head today but didn't settle on anything. I'll take a close look on Monday or Tuesday. Pretty sure this is going in. |
@@ -1,5 +1,5 @@ | |||
export default function thunkMiddleware({ dispatch, getState }) { | |||
return (next) => (action) => | |||
return next => action => | |||
typeof action === 'function' ? | |||
action(dispatch, getState) : |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we consider having the default thunk middleware send the same methods
type object to action
instead of splitting them into separate parameters like this? i.e.
action({ dispatch, getState })
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought about that, too, but 9 times out of 10 a thunk just needs dispatch()
, and single argument form is easier.
I understand it now. Great job. |
See reduxjs/redux#213 for explanation
This PR updates the middleware API in several ways.
All middleware is now smart middleware
The API becomes much simpler if we just assume that all middleware is "smart" middleware. I know previously I argued against something like this:
because it's fragile, hard to compose, and awkward to use from the call site. I also figured most middleware wouldn't need either
dispatch()
orgetState()
. But as we discovered with the thunk middleware, any asynchronous middleware is going to needdispatch()
so that it can dispatch from the beginning of the chain. So let's just make all middleware "smart" like this:where
methods
is a subset of the store, currently{ dispatch, getState }
. Now it's easy to compose using just function composition. And if users have already written their own "dumb" middleware, all they need to do to update is wrap it in one more function:applyMiddleware(...middlewares)
Now that all middleware is smart middleware, we don't have to deal with this nonsense. We get a nice, simple API for applying middleware:
applyMiddleware()
is a higher-order store, just like @gaearon's devtools (I assume... haven't seen the final version yet.). So, it composes!Caveat:
thunkMiddleware()
is no longer built-in tocreateStore()
This is because middleware needs to come before any other higher-order stores, since middleware has the potential to be asynchronous.
However! It is still the default when using
applyMiddleware()
. It's additional step versus today, but I imagine most users will be usingapplyMiddleware
, anyway:Updated
composeMiddleware()
I updated
composeMiddleware()
to support the new signature. In the process, I also discovered a bug with the previous implementation where it only works if dispatch is the last argument. It did not combine multiple middlewares into a single middlewares as advertised; it only combined multiple middlewares with dispatch to create a new dispatch.The reason is because it was simply doing function composition, where
compose(a, b, c)
becomesa(b(c))
. But this isn't right, because thisis not the same this
Which means previously we couldn't do this, as advertised in the docs (and by, ya know, the name of the function):
I fixed the issue and updated the test. We'll need to update the docs in master, too, as they're currently incorrect.
I've renamed the previous method to just
compose()
, since it's still useful elsewhere.