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

Let combineReducers pass full reducer signature #1913

Closed
pgilad opened this issue Aug 28, 2016 · 5 comments
Closed

Let combineReducers pass full reducer signature #1913

pgilad opened this issue Aug 28, 2016 · 5 comments

Comments

@pgilad
Copy link

pgilad commented Aug 28, 2016

Currently combineReducers applies the nested reducers with the following props:

reducer(stateByKey, action)

If you need the full state in one of the nested stores, you can't access it, and you'll be forced to create a custom combineReducers.

I suggest changing the signature (in a non-breaking way) to be fully compliant with the reducer signature:

reducer(stateByKey, action, key, state) - where the last property is the original state passed on to combineReducers.

This way, if you need the full state (or a state from a nested sibling reducer) you will be able to access it. This is compliant with the way lodash applies the reducer function to collections, and the way Array applies the reducer function to each element.

Also, it does not break things, but extends current functionality.

If approved I can add a PR for this.

@markerikson
Copy link
Contributor

Numerous changes to combineReducers have been suggested, and generally rejected. Please see #1768 , #1904 , and #1910 for recent discussion on the topic.

@jimbolla
Copy link
Contributor

I do think the number of times this issue keeps getting raised is an indicator that we should do something. But I'm not sure if that's a code change or a documentation change. I think part of it is that people (including myself initially) see combineReducers as the only way to combine reducers.

@pgilad
Copy link
Author

pgilad commented Aug 28, 2016

I don't see it as the only way, but adhering to the reducer signature and passing the complete state will definitely help with a lot of cases where you have to use your custom combineReducers

@markerikson
Copy link
Contributor

I do think the number of times this issue keeps getting raised is an indicator that we should do something. But I'm not sure if that's a code change or a documentation change. I think part of it is that people (including myself initially) see combineReducers as the only way to combine reducers.

Yes, which is one of the biggest reasons why I'm writing the "Structuring Reducers" pages.

I don't see it as the only way, but adhering to the reducer signature and passing the complete state will definitely help with a lot of cases where you have to use your custom combineReducers

I'm not sure what you mean by that. The signature of an Array.reduce()-compatible function is (accumulator, item) -> newValue, which in Redux terms is (state, action) -> newState. Passing additional arguments to those functions, technically speaking, would make them not reduce-compatible any more (which is why I spent time trying to nail down some terminology in the "Splitting Reducer Logic" page.

I do certainly understand the desire to share data across slices, but there's multiple valid approaches to the issue that do not require changes to combineReducers.

If anything actually is gonna happen with updating combineReducers, #1768 would be the place to resume work on it. Beyond that, there's dozens of published utility libs for various reducer use cases in my Redux addon catalog's "Reducers" page, including several variations on combineReducers that do something along the lines of "passing more state". And to re-re-re-re-iterate, combineReducers is _just a utility for the most common use case_, and you are absolutely encouraged to write functions that fit your own specific use case better.

@pgilad
Copy link
Author

pgilad commented Aug 29, 2016

I know, I read all of your docs. Nonetheless the signature for Array.reduce() is (accumulator, item, index, array) -> newValue and lodash signs it as: (accumulator, value, index|key, collection) -> newValue. So the reduce signature does have that extra shine 😄

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

No branches or pull requests

3 participants