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

Addresses #959 without affecting the library's existing functionality… #1813

Closed
wants to merge 2 commits into
base: master
from

Conversation

6 participants
@robertleeplummerjr

robertleeplummerjr commented Jun 16, 2016

…. Ultimately tries to prevent re-renders when multiple states are needed in quick succession.

Addresses #959 without affecting the library's existing functionality…
…. Ultimately tries to prevent re-renders when multiple states are needed in quick succession.
@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jun 16, 2016

Contributor

This seems like an issue that's already resolved in userland. Per #959 The current recommended approach is to use redux-batched-actions.

Contributor

aweary commented Jun 16, 2016

This seems like an issue that's already resolved in userland. Per #959 The current recommended approach is to use redux-batched-actions.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

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.

robertleeplummerjr commented Jun 16, 2016

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

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.

robertleeplummerjr commented Jun 16, 2016

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

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.

robertleeplummerjr commented Jun 16, 2016

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

@kswope Since you originally opened #959, would you be so kind to offer input?

robertleeplummerjr commented Jun 16, 2016

@kswope Since you originally opened #959, would you be so kind to offer input?

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jun 16, 2016

Contributor

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

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.

Contributor

aweary commented Jun 16, 2016

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

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

Could you point me in the direction of the code you are referencing?

robertleeplummerjr commented Jun 16, 2016

Could you point me in the direction of the code you are referencing?

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 16, 2016

@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?

robertleeplummerjr commented Jun 16, 2016

@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?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 16, 2016

Contributor

"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).

Contributor

markerikson commented Jun 16, 2016

"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).

@aweary

This comment has been minimized.

Show comment
Hide comment
@aweary

aweary Jun 17, 2016

Contributor

Could you point me in the direction of the code you are referencing?

If you're referring to redux-batched-actions you can see the source here

Contributor

aweary commented Jun 17, 2016

Could you point me in the direction of the code you are referencing?

If you're referring to redux-batched-actions you can see the source here

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 17, 2016

@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:

  • This week at least one other of my fellow engineers ran into an issue where this would solve the problem of multiple rerenders. This is probably niche.
  • There are a few closed, related, not handled directly in redux, unresolved issues on this project, and perhaps a closed pr (this one looks like it may soon be). (#544, #797, #959, #1070) Still probably niche.
  • If I search for "redux multiple dispatches", I get 58,000 results on the most popular search engine, wow!

Let us break these results down a bit:


Seems like niche may not be the right word here, I'm 3 pages into the search! What if there were a small number of these that were directly related to dispatching multiple actions are limited to 10,000, a relatively big number. A similar number (http://www.skincancer.org/skin-cancer-information/skin-cancer-facts) will die this year from skin cancer, is that niche? I'm getting too philosophical, let's shrink the number. Let's say there are just 100, of the 58,000 results that are related to dispatching multiple actions (yes that is right 0.01%... I could be wrong, was never that good at math) that is still 100 people who have taken the time (at least 4 to 20 hours) to learn about redux, start to use it, and realize that they need to dispatch multiple actions with redux. Let us do the math, shall we!

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 $99,530 / (52 - 2) / 40 = $49.765 hourly. We'll round down to $49 hourly. If 100 people spent 10 hours to finally arrive at that they needed to dispatch multiple actions and spent another two hours looking for the solution, I believe that is (10 + 2) * 100 = $58,800 worth of niche, and that is humbly without adding myself.

But Han, I must!
There are as well at least 4 solutions dispatching multi actions (https://github.com/acdlite/redux-batched-updates, https://github.com/tshelburne/redux-batched-actions, https://github.com/stephan83/redux-smart-action, https://github.com/acdlite/redux-promise) let us say each of these had a good 50 or so hours under their belt to create their proposed solution: (4 * 50) * $49 = $9,800, add that to the $58,800 and you arrive at $58,800 + $9,800 = $68,600.

Now guys, that is a lot of cheddar.

I can think of a few things I'd buy if I had it...


(yea.... that is a Telsa)

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:

robertleeplummerjr commented Jun 17, 2016

@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:

  • This week at least one other of my fellow engineers ran into an issue where this would solve the problem of multiple rerenders. This is probably niche.
  • There are a few closed, related, not handled directly in redux, unresolved issues on this project, and perhaps a closed pr (this one looks like it may soon be). (#544, #797, #959, #1070) Still probably niche.
  • If I search for "redux multiple dispatches", I get 58,000 results on the most popular search engine, wow!

Let us break these results down a bit:


Seems like niche may not be the right word here, I'm 3 pages into the search! What if there were a small number of these that were directly related to dispatching multiple actions are limited to 10,000, a relatively big number. A similar number (http://www.skincancer.org/skin-cancer-information/skin-cancer-facts) will die this year from skin cancer, is that niche? I'm getting too philosophical, let's shrink the number. Let's say there are just 100, of the 58,000 results that are related to dispatching multiple actions (yes that is right 0.01%... I could be wrong, was never that good at math) that is still 100 people who have taken the time (at least 4 to 20 hours) to learn about redux, start to use it, and realize that they need to dispatch multiple actions with redux. Let us do the math, shall we!

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 $99,530 / (52 - 2) / 40 = $49.765 hourly. We'll round down to $49 hourly. If 100 people spent 10 hours to finally arrive at that they needed to dispatch multiple actions and spent another two hours looking for the solution, I believe that is (10 + 2) * 100 = $58,800 worth of niche, and that is humbly without adding myself.

But Han, I must!
There are as well at least 4 solutions dispatching multi actions (https://github.com/acdlite/redux-batched-updates, https://github.com/tshelburne/redux-batched-actions, https://github.com/stephan83/redux-smart-action, https://github.com/acdlite/redux-promise) let us say each of these had a good 50 or so hours under their belt to create their proposed solution: (4 * 50) * $49 = $9,800, add that to the $58,800 and you arrive at $58,800 + $9,800 = $68,600.

Now guys, that is a lot of cheddar.

I can think of a few things I'd buy if I had it...


(yea.... that is a Telsa)

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:

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 17, 2016

Contributor

Erm. Speaking for myself: arguing via gifs is not inclined to make me support anything.

Contributor

markerikson commented Jun 17, 2016

Erm. Speaking for myself: arguing via gifs is not inclined to make me support anything.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 17, 2016

Contributor

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.

Contributor

markerikson commented Jun 17, 2016

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 17, 2016

Did I argue with gifs? I humbly thought I did it with numbers.

robertleeplummerjr commented Jun 17, 2016

Did I argue with gifs? I humbly thought I did it with numbers.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 17, 2016

Hestation on single for loop, lol. It has been real fellas. Adios.

robertleeplummerjr commented Jun 17, 2016

Hestation on single for loop, lol. It has been real fellas. Adios.

@thinkloop

This comment has been minimized.

Show comment
Hide comment
@thinkloop

thinkloop Jun 17, 2016

I too think this is an easy and important change to make without the heavy-handed recommended solutions.

dispatch([action1, action2]) is pretty clean and straightforward.

Is it possible to achieve this using enhancers without forking?

thinkloop commented Jun 17, 2016

I too think this is an easy and important change to make without the heavy-handed recommended solutions.

dispatch([action1, action2]) is pretty clean and straightforward.

Is it possible to achieve this using enhancers without forking?

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

This solution isn't: dispatch([action1, action2]), it is actually: dispatch(action1, action2) (no array), which keeps the entire existing solution intact, and adds the functionality you mention.

robertleeplummerjr commented Jun 18, 2016

This solution isn't: dispatch([action1, action2]), it is actually: dispatch(action1, action2) (no array), which keeps the entire existing solution intact, and adds the functionality you mention.

@thinkloop

This comment has been minimized.

Show comment
Hide comment
@thinkloop

thinkloop Jun 18, 2016

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:

dispatch(obj) core

dispatch(fn) thunk

dispatch(array) batch

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.

thinkloop commented Jun 18, 2016

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:

dispatch(obj) core

dispatch(fn) thunk

dispatch(array) batch

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.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 18, 2016

Collaborator

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).

Collaborator

gaearon commented Jun 18, 2016

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).

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

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 dispatch(oneAction) that is perfectly fine, this pr doesn't change that. However for those that want to live a little on the wild side and decide at some point to pass in another action to dispatch dispatch(one, and, you, can, keep, em, comin, boss) can do so and render will only be called once at the end.

robertleeplummerjr commented Jun 18, 2016

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 dispatch(oneAction) that is perfectly fine, this pr doesn't change that. However for those that want to live a little on the wild side and decide at some point to pass in another action to dispatch dispatch(one, and, you, can, keep, em, comin, boss) can do so and render will only be called once at the end.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

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).

robertleeplummerjr commented Jun 18, 2016

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).

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

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.

robertleeplummerjr commented Jun 18, 2016

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.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

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!

robertleeplummerjr commented Jun 18, 2016

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!

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 18, 2016

Contributor

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.

Contributor

markerikson commented Jun 18, 2016

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.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 18, 2016

Collaborator

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!

Collaborator

gaearon commented Jun 18, 2016

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!

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 18, 2016

Where is that github dance emoji when you need it?

robertleeplummerjr commented Jun 18, 2016

Where is that github dance emoji when you need it?

@thinkloop

This comment has been minimized.

Show comment
Hide comment
@thinkloop

thinkloop Jun 21, 2016

It doesn't look like thunk would break, any example of one that would break? This is a bit different because this arg can be fully ignored without any differences in functionality, whereas if the additional actions were ignored, they would be omitted and functionality would be very different.

thinkloop commented Jun 21, 2016

It doesn't look like thunk would break, any example of one that would break? This is a bit different because this arg can be fully ignored without any differences in functionality, whereas if the additional actions were ignored, they would be omitted and functionality would be very different.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 21, 2016

Collaborator

If we were to add an argument to the dispatch function, usage like <div onClick={dispatch.bind(store, myAction)} would break: the previously ignored event argument suddenly gains a meaning (and significantly changes the behavior).

Collaborator

gaearon commented Jun 21, 2016

If we were to add an argument to the dispatch function, usage like <div onClick={dispatch.bind(store, myAction)} would break: the previously ignored event argument suddenly gains a meaning (and significantly changes the behavior).

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 22, 2016

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.

multi(dispatch(one, two, and, you, can, keep, em, comin, boss)) <- works amazing and only triggers render once
dispatch(one, two, and, you, can, keep, em, comin, boss) <- throws exception because multi was not called first

I'm just trying to think outside the box here, feel free to shoot down.

robertleeplummerjr commented Jun 22, 2016

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.

multi(dispatch(one, two, and, you, can, keep, em, comin, boss)) <- works amazing and only triggers render once
dispatch(one, two, and, you, can, keep, em, comin, boss) <- throws exception because multi was not called first

I'm just trying to think outside the box here, feel free to shoot down.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 22, 2016

Collaborator

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 redux-batch-actions completely solves the problem and doesn’t introduce complexity for anyone.

Don’t want to install that package? Copy and paste it. It’s literally 16 lines.

Collaborator

gaearon commented Jun 22, 2016

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 redux-batch-actions completely solves the problem and doesn’t introduce complexity for anyone.

Don’t want to install that package? Copy and paste it. It’s literally 16 lines.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 22, 2016

Contributor

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 dispatch([{type : "INCREMENT"}, {type : "INCREMENT"}]), see the number go up by 2, and only have one notification. I was even able to put the batched dispatch enhancer after applyMiddleware, and dispatch an array of actions from a thunk. I've updated the gist appropriately: https://gist.github.com/markerikson/0383775096f2b32e48ad41499cf8a1fb

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 dispatch itself at this time.

Contributor

markerikson commented Jun 22, 2016

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 dispatch([{type : "INCREMENT"}, {type : "INCREMENT"}]), see the number go up by 2, and only have one notification. I was even able to put the batched dispatch enhancer after applyMiddleware, and dispatch an array of actions from a thunk. I've updated the gist appropriately: https://gist.github.com/markerikson/0383775096f2b32e48ad41499cf8a1fb

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 dispatch itself at this time.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 22, 2016

Collaborator

After #1702 this enhancer should get considerably less hairy.

Collaborator

gaearon commented Jun 22, 2016

After #1702 this enhancer should get considerably less hairy.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 22, 2016

Good job @markerikson! Shall I close this then?

robertleeplummerjr commented Jun 22, 2016

Good job @markerikson! Shall I close this then?

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Jun 22, 2016

Contributor

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.

Contributor

markerikson commented Jun 22, 2016

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.

@gaearon

This comment has been minimized.

Show comment
Hide comment
@gaearon

gaearon Jun 22, 2016

Collaborator

It may be worth revisiting after #1702 is done just to see where things stand

Yea. #1702 really changes a lot of the internal wiring so it’s best to at least wait until that happens.

Collaborator

gaearon commented Jun 22, 2016

It may be worth revisiting after #1702 is done just to see where things stand

Yea. #1702 really changes a lot of the internal wiring so it’s best to at least wait until that happens.

@robertleeplummerjr

This comment has been minimized.

Show comment
Hide comment
@robertleeplummerjr

robertleeplummerjr Jun 22, 2016

Thanks everyone who got involved! This was fun. I hope to have more fun in the future.

robertleeplummerjr commented Jun 22, 2016

Thanks everyone who got involved! This was fun. I hope to have more fun in the future.

@arcanis

This comment has been minimized.

Show comment
Hide comment
@arcanis

arcanis Sep 16, 2016

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 commented Sep 16, 2016

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.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Sep 16, 2016

Contributor

@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 combineReducers. It can be things like:

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 BATCH_ACTION is "unknown to other enhancers` is a feature and not a bug.

Another approach would be to modify your RESOURCE_SET handler itself to accept multiple possible values in one action (conceptually different from BATCH_ACTION in that you're not re-running RESOURCE_SET multiple times, but rather that RESOURCE_SET can apply to multiple things at once).

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 RESOURCE_SET event. You can use Redux-ORM to look up multiple entities, queue updates, and apply them in a single step.

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.

Contributor

markerikson commented Sep 16, 2016

@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 combineReducers. It can be things like:

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 BATCH_ACTION is "unknown to other enhancers` is a feature and not a bug.

Another approach would be to modify your RESOURCE_SET handler itself to accept multiple possible values in one action (conceptually different from BATCH_ACTION in that you're not re-running RESOURCE_SET multiple times, but rather that RESOURCE_SET can apply to multiple things at once).

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 RESOURCE_SET event. You can use Redux-ORM to look up multiple entities, queue updates, and apply them in a single step.

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.

@markerikson

This comment has been minimized.

Show comment
Hide comment
@markerikson

markerikson Sep 18, 2016

Contributor

@arcanis : As one more reference point, Dan demonstrated exactly that BATCH_ACTION approach in a tweet last year:

https://twitter.com/dan_abramov/status/656074974533459968

Contributor

markerikson commented Sep 18, 2016

@arcanis : As one more reference point, Dan demonstrated exactly that BATCH_ACTION approach in a tweet last year:

https://twitter.com/dan_abramov/status/656074974533459968

@arcanis

This comment has been minimized.

Show comment
Hide comment
@arcanis

arcanis Sep 19, 2016

@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:

  • Either it won't work with third-party middlewares (unless they add a specific support for a common "batch action", at which point the arguments in favor of not breaking the enhancers become moot)
  • Or it will require a particular setup from the user, requiring them to add a specific enhancer before any other one. So more boilerplate code, with the risk that it needs to be done multiple times if multiple third-parties use multiple different implementations.

As for expanding actions in order to take more responsibilities (such as my RESOURCE_SET that could take an array of resources), while it would work fine at the beginning, it would start to come short once we reach the point were we need to do different things all at once. To keep on the resource example, in some case (deleting a resource while updating its parent relationship) we need do to both a RESOURCE_SET on the resource A and a RESOURCE_UNSET on a resource B. Supporting this use case as a single action would requiring duplicating code, which is something that I would like to avoid.

arcanis commented Sep 19, 2016

@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:

  • Either it won't work with third-party middlewares (unless they add a specific support for a common "batch action", at which point the arguments in favor of not breaking the enhancers become moot)
  • Or it will require a particular setup from the user, requiring them to add a specific enhancer before any other one. So more boilerplate code, with the risk that it needs to be done multiple times if multiple third-parties use multiple different implementations.

As for expanding actions in order to take more responsibilities (such as my RESOURCE_SET that could take an array of resources), while it would work fine at the beginning, it would start to come short once we reach the point were we need to do different things all at once. To keep on the resource example, in some case (deleting a resource while updating its parent relationship) we need do to both a RESOURCE_SET on the resource A and a RESOURCE_UNSET on a resource B. Supporting this use case as a single action would requiring duplicating code, which is something that I would like to avoid.

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