Addresses #959 without affecting the library's existing functionality… #1813
Conversation
…onality. Ultimately tries to prevent re-renders when multiple states are needed in quick succession.
|
This seems like an issue that's already resolved in userland. Per #959 The current recommended approach is to use redux-batched-actions. |
|
I did see that approach but felt ultimately that if one simply wants to dispatch multiple actions, it would be a whole lot easier to allow for that, and iterate through them giving redux the ability to solve it. Also, I noticed that there were a lot of people asking for this feature (including me and my team) and I just felt like it made sense as a natural progression of the library. |
|
As for it being resolved, you are right it was resolved. My solution requires much less knowledge and is more performant. To illustrate: Suppose there were two free cars, a Tesla Roadster and a coal power 1950's VW Bug. Both get the job done, getting one from point a to point b, but one is just... cooler. 😛 The above is not meant as a zing to any library or work, just that redux is cool. |
|
Thinking further about the issue, I actually don't think the recommended approach sovles the issue. If I search for "redux multiple dispatches" (or I reference #959) I get people asking something to the likeness of: "Can redux do multiple actions with one dispatch"? The above solution is not done in redux, it is done in react, and then after it is done, the middleware/addon tells redux to essentially fire a dispatch with the same state it already has, which afterwards calls listeners. So the recommended solution to get redux to handle multiple state changes isn't even in redux. This pr does answer #959 directly, inside redux, specifically allowing for multiple actions, and then after the state is satisfied, it calls listeners. |
redux-batched-actions provides a higher-order reducer and batching function to use with dispatch. It's not coupled to React in any way. Redux has purposefully maintained a small API surface area and left enhancements up to third party libraries. I don't think making dispatch polymorphic brings enough advantages; it may or may not be more performant, but it seems like it would be marginally so and at the expense of expanding the API. |
|
Could you point me in the direction of the code you are referencing? |
|
@aweary & @markerikson do you feel that a single loop, iterating through the arguments of dispatch is the tipping point in complexity for the redux api? |
|
"Tipping point"? Dunno about that. But, we do have plenty of overloads and such already, and I'm hesitant to support further changes to the API for something that's a relatively niche use case (especially given that there's already a "blessed" userland approach). |
If you're referring to redux-batched-actions you can see the source here |
|
@markerikson I understand your hesitation, and I would be hesitant too. A lot of people use this tool and it is very powerful. @aweary thank you for clarification on the recommended approach, I got it mixed up with another approach that was found in userland, and saw that I was ultimately wrong in saying that. The solution that I was thinking of was: https://github.com/acdlite/redux-batched-updates/blob/master/src/index.js, which uses a now redacted addon in react to accomplish batch actions. As for this being "niche", lets us assess the numbers so that we have an accurate view of what this adds:
Let us break these results down a bit:
The average salary per year for a software engineer (as I'm sure you are aware) is $99,530 (according to the most popular search engine). Broken down by hour that is Now guys, that is a lot of cheddar. I can think of a few things I'd buy if I had it...
In the humblest way, it just feels like this tiny improvement adds what a lot of people have taking time to both communicate about and to try and find an answer to. There are many people involved on many levels who would like to see this feature implemented directly into redux. If you are satisfied with not having this in the api, that is fine and I am fine with that, but my humble and non-argument is that based off sheer numbers, it would find a very cozy home amongst the other bits that are of representation of your fine work. But please do not feel that I am negative in adding to the already sterling work here. Please don't think: I like simplicity and saw that it could be added here for the greater good. Also, I got to use a few gifs, which is always fun. Thank you for your hard work, and fully expect this pr to be closed by someone directly in the redux team. But in the unlikely chance that it makes it into redux, even if there are requests made to refine said pr: |
|
Erm. Speaking for myself: arguing via gifs is not inclined to make me support anything. |
|
Having said that, let me try to respond more directly. First, I want to be clear: I have been given commit access to the ReactJS org repos, including the Redux repo. However, I was given that access because I've spent a bunch of time working on the docs, and I've kept my actual commit actions limited to that side of things. I've participated in discussions regarding code changes, such as this one, but I don't regard myself in any way as an arbiter of what actually makes it into the ReactJS org codebases. Having said that, I do also have opinions on where the APIs should go, such as this one. There's a couple main conceptual ways someone may want to do "multiple dispatches". They may simply want to do a number of things in sequential order, or they may be looking to combine multiple actions into one dispatch so that there's only a single subscription notification emitted. Yours appears to be the latter. Regarding the appeal to popularity based on search results: the search approach really isn't valid. Of the eight search result links you provided, all but one of them appear to be talking about sequential dispatches, and the other one simply has a link to the "redux-multi" middleware, which also does sequential dispatches. In addition, when I ran that search myself, I did see roughly 58K search results claimed, but it stopped listing any after 10 pages, and many of the search results appeared to be Stack Overflow clone spam results. For that matter, the words could appear separate anywhere in a page, rather than together as a specific phrase. So, as an appeal to "everyone wants this", that doesn't really work. As for the whole "time is money" argument: I see your point, but I'm basically not going to waste my time coming up with a further response to that aspect. Ultimately, my opinion is that while the concept is useful, and there are reasons why someone would want it, and it is a relatively small change... I don't think there's enough overall usefulness or value to justify including it in the core. If someone wants to simply dispatch multiple actions sequentially with a change event for each as normal, all they need are thunks or even just multiple dispatches from a component. If someone is actually in a situation where they need to batch dispatches so that there's only one change event, there's options out there, and the FAQ points to them. |
|
Did I argue with gifs? I humbly thought I did it with numbers. |
|
Hestation on single for loop, lol. |
|
I too think this is an easy and important change to make without the heavy-handed recommended solutions.
Is it possible to achieve this using enhancers without forking? |
|
This solution isn't: |
|
also nice, the array solves the ordering issue as well tho, and an array as input is an unused/unsupported object - this makes a very nice interface:
Whether the array is in redux or middleware doesn't matter, but I don't think it's possible with middleware, is it? If it is, I'd be happy with that. |
|
It's not about the loop, it's about adding something to the core API. Anything we add we basically have to support forever. This is not an “easy” change because it would affect any extension to Redux (hundreds of userland middlewares would have to be adapted to support this). |
|
This is open source, you can rely on the world to help a little, after all we use this stuff to feed our families and to build a better planet. Also, I disagree that it would affect hundreds of userland middlewares directly, it would only affect them if they wanted it. IE: if they'd like to continue using |
|
Too, it actually is easy, it is a single loop that doesn't affect anything currently. Doubt me? I added a unit test (3824a21#diff-4977f3c2200f2ec8ff062b6bd8c7fa55R80), and your existing unit tests don't break (https://travis-ci.org/reactjs/redux/builds/138167268). |
|
Sorry, the only thing that is affected, is (ironically) the unit test. Because it is the only thing that currently takes advantage of it. My bad. |
|
I feel like I'm hearing negative about supporting open source. Come on guys, chins up! Open source rocks! The ideas are always bigger than the code that runs them, and this library is quite the idea! |
|
Open source is great, but just a project is open source doesn't mean that every PR must automatically be accepted. And, the more popular the project, the more changes have to be carefully considered. |
|
First of all I would like to ask everybody to remain calm and respectful to each other. Let's remember each of us has the best intentions. I'm sorry I haven't addressed this thread timely. I'm currently busy with other projects, and I can't dedicate as much time to Redux—also I'm deliberately trying to move slow here because of the size of the ecosystem. Any decisions are not taken lightly, especially when concerning the core APIs. Right now it's 3am so I won't dive into details of what I think about this change. I will try to write up my thinking tomorrow. Hopefully we can avoid the dismissive tone, respect each other's choices and opinions, and avoid bikeshedding as we discuss this. Thanks for your time! |
|
Where is that github dance emoji when you need it? |
|
If we were to add an argument to the dispatch function, usage like |
|
We could expose a function that alters how dispatch works for one iteration that reverts afterwards. This way we would always know that what comes next will be a multi dispatch that will revert to a single dispatch after or not be a multi dispatch afterall.
I'm just trying to think outside the box here, feel free to shoot down. |
|
This sounds like a lot of accidental complexity to me. Yes, we can theoretically devise ways to make it work but I’m afraid we’re not going to understand all possible edge cases until we release this, and later we might find ourselves unable to fix it, thus making it painful for everyone around. On the other hand, explicit batching on reducer level like Don’t want to install that package? Copy and paste it. It’s literally 16 lines. |
|
Hah. So. I just tried testing out the proof-of-concept enhancer I linked earlier. Borrowed the source for the "counter-vanilla" Redux example, transpiled the enhancer to ES5 for good measure, and put them together. And other than the one capitalization typo I'd left in there... _it worked_. I was able to run So. Based on all that, it appears that a simple enhancer solves this use case sufficiently. It requires no change to the core API, it's entirely opt-in, and it works. I don't see any reason to make any changes to the core |
|
After #1702 this enhancer should get considerably less hairy. |
|
Good job @markerikson! Shall I close this then? |
|
Yeah. I don't see any changes being made to the core at this time. It may be worth revisiting after #1702 is done just to see where things stand, but for now it seems like that enhancer solves the use case easily enough. |
|
Thanks everyone who got involved! This was fun. I hope to have more fun in the future. |
|
I'm just sharing my experience: in a decent-sized project, we currently have a redux-saga that handle the resource management logic. Basically, when it "sees" a RESOURCE_UPDATE action, it will first push the update resource into the store (optimistically) via dispatching a RESOURCE_SET action, send the data, then push the updated resource again - either to apply what the server answered, or to rollback if an error happened. In some cases, a single RESOURCE_UPDATE action actually needs to update multiple resources. A prime example is when adding resources to a to-many relationship: we need to update both ends of the relationship. In such a case, we need to dispatch multiple RESOURCE_SET actions, one for each resource that needs to be updated. We started doing this by simply dispatching multiple times, but issues arised when react-redux would trigger a rerender in-between those RESOURCE_SET actions. In such a case, the store was still in an incomplete state: some data would say something that wasn't baked by the others. We eventually implemented a simple BATCH_ACTION in our primary reducer so that we could dispatch multiple RESOURCE_SET actions at once, and everything just worked. Our tree stopped being rerendered when the state wasn't yet ready to be read. My concern is that we fixed this behaviour via an application-specific piece of code. Redux enhancers have no idea that BATCH_ACTION is a batched action, and will treat it like any other one. I could use a third-party packages to add this action, but the result would be the same: most enhancers would have no idea what to do with the third-party action, unless they explicitely write code to support such an action. But at this point, why not add it to the core, since they will have to support it anyway? I could also use @markerikson's enhancer, or a similar one, but then what if I want to extract this resource management code into its own library (which is what I'm doing)? Should I tell users that they need to manually add this enhancer for everything to work? On the plus side, this issue has a few easy workarounds, so it's not the most urgent one to solve. But I still think it should be seriously considered for inclusion in the next major release. The more soon you publish an Intent-to-implement, the more time the third-party enhancers have to adapt. |
|
@arcanis : Thanks for your comment. Some thoughts: First, creating a "batched reducer" that wraps up other reducers is an entirely valid approach. In fact, it very much mirrors the common approach seen in Elm. Remember that "reducer composition" doesn't just mean use of const combinedReducer = combineReducers(bunchOfReducers);
const batchedReducer = createBatchedReducer(combinedReducer);
const hydratableReducer = createHydratableReducer(batchedReducer);
const store = createStore(hydratableReducer);Each layer of wrapping is independent of the others, can be composed together. So yes, that's a great approach right there, and the fact that your Another approach would be to modify your A third approach, particularly if you're dealing with managing relational entities, would be to use a library like Redux-ORM, and write a case reducer that handles that specific scenario rather than a generic Beyond that, there's the variety of possible batching strategies already discussed in this thread, all of which have their own tradeoffs. So yes, my take is that this is still entirely solveable in userland, the proper approach is still specific to each user's use cases, and there is still no need to add this to the Redux core. |
|
@arcanis : As one more reference point, Dan demonstrated exactly that |
|
@markerikson Yep, that's what we're currently using :) I guess my take is that while it is possible to implement this feature userland, any such implementation will eventually fall into one of those two buckets:
As for expanding actions in order to take more responsibilities (such as my |
…. Ultimately tries to prevent re-renders when multiple states are needed in quick succession.