Introduce new middleware API #29
Conversation
lib/Store.lua
Outdated
| local dispatch = function(...) | ||
| return unboundDispatch(self, ...) | ||
| end | ||
|
|
||
| for _, middleware in ipairs(middlewares) do |
There was a problem hiding this comment.
Do we want to do anything with #28 to make middleware ordering line up with Redux?
There was a problem hiding this comment.
This would be the time to do it, but I'm unsure if we want to change the order. Because the middleware call sequence depends entirely on what the middleware actually do, I don't think either behavior is particularly intuitive...
There was a problem hiding this comment.
They're not very intuitive, but at the same time I'm not sure there's a nicer way of specifying middleware order besides an array >.<
I think if we're going to stick to an array for middlewares then invoking them from the front of the list forward makes the most sense, but that's just me
There was a problem hiding this comment.
I agree that it's not intuitive either way.
I think the best approach is to probably document the way that the middleware is applied, because if you ever write any middleware there's a good chance you'll need to know one way or another. As long as the info is easy to find that should be enough.
Also, this doesn't necessarily need to be added in this PR.
|
What. The tests pass still, that's probably not a good sign. |
|
Most test cases should have passed still, actually...but the |
|
Tests are updated, and I updated the CHANGELOG, since it was completely empty! |
LPGhatguy
changed the title
lib/Store.lua
Outdated
| local dispatch = function(...) | ||
| return unboundDispatch(self, ...) | ||
| end | ||
|
|
||
| for _, middleware in ipairs(middlewares) do |
There was a problem hiding this comment.
I agree that it's not intuitive either way.
I think the best approach is to probably document the way that the middleware is applied, because if you ever write any middleware there's a good chance you'll need to know one way or another. As long as the info is easy to find that should be enough.
Also, this doesn't necessarily need to be added in this PR.
docs/api-reference.md
Outdated
| A single middleware is just a function with the following signature: | ||
|
|
||
| ``` | ||
| (next) -> (store, action) -> result | ||
| (nextDispatch, store) -> (action) -> result | ||
| ``` | ||
|
|
||
| That is, middleware is a function that accepts the next middleware to apply and returns a new function. That function takes the `Store` and the current action and can dispatch more actions, log to output, or do network requests! |
There was a problem hiding this comment.
This probably needs to be updated as well:
"That is, middleware is a function that accepts the Store and the next middleware to apply and returns a new function. That function takes the current action and can dispatch more actions, log to output, or do network requests!"
More detail wouldn't hurt either, but can be added later.
|
After our discussion, I think this middleware sequence makes the most sense |
This changes the middleware signature and makes it line up more closely with Redux. It solves a problem we hit of not being able to get the store's state until an action was dispatched.
It also reverses the order that middleware runs in to be left-to-right. Previously, it was right-to-left and a lot of people were really confused, including us. Now people should be less confused, and we can use more precise language to describe middleware execution order.
This is a breaking change.
Old signature:
New signature:
In addition, the
nextDispatchfunction (previously callednext, which collided with Lua's standard library) no longer accepts the store as the first argument, ie it is bound to the store instance.TODO: