add third argument state to combineReducer. add some documentation

[gurbraj]

This is another way to share state between reducers. Found this way to be more testable than the other ways.

Look forward to your feedback!

[timdorr]

Duplicate of #1904, #1768, and #1527. Related to #1518 and #1400.

The main arguments against this are:

It is not obvious that the previous state gets passed. What if you want to have the updated one?

In general passing the whole state to subreducers is an anti-pattern because it encourages tight coupling. This is why I’m inclined not to allow this by default, and not to merge this PR.

In some cases it can be useful to pass some small parts of the state (the smaller the better), and in this case I think that the parent reducer needs to be written manually to do that, so that the dependency is explicit rather than hidden away in the combineReducers() implementation.

Those still apply here, so I'm disinclined to allow it as well. If you need to write a more complex reducer, you either need to rethink your reducer/selector architecture or not use something as simplistic as combineReducers. It's not a big function, nor should it be. And I mean that both in terms of code size, but also in terms of scope of use. It should remain very much optional to your use of Redux. I'd almost argue it should be split into a separate package...

I'll leave this open so we can discuss things, though. I'm not against being persuaded! 😄

[andrevinsky]

@timdorr, hi! Sorry, let me step into this discussion.

@gurbraj's way of sharing the state seems pretty forceful (reducer cannot opt to NOT get it) but his intention (!) is nevertheless noteworthy.

How about not passing the state (as a third argument) but rather a way to get to it? Options are: a getState() (to provide ways to reducers to access the entire state) or a lookup function (to provide access to selected portions of the entire state and limit it to read-only)?

This may be a worthwhile case. I got myself in numerous instances constructing crutches to provide data from two parallel paths. List/data compartments and ids, UI state and errors, forms and UI state.

For instance, I have a questionnaire (with sections and questions). I receive answers to it and am interested in looking up a count of questions in a particular section - just to verify correctness of the answers. Obviously, questionnaires data (it is a form-related metadata) and answers data cannot be mixed and are stored in different store's paths. And the action which holds the portion of answers is not a perfect place to hold a number of expected questions answered. So instead of a simple action (where I say, "here are some more answers"), I need to construct thunked action, use getState() there, obtain the number of expected questions, and do stuff - in short I place more the logic in my actions rather than keeping it in reducers.

Now then, you may argue saying 'thunked actions is exactly the place where the logic must reside'. However, I am convinced that programmers' intentions may vary: some would care to have all of their logic reside in actions (say, saga-based), others, would be more inclined to hold it in their reducers, others still - elsewhere. Restricting them on the 'because reasons' pretext is a bit short-sighted, pardon me saying.

Back to the lookup function (I take it that how getState() function is supposed to work, is clear). The lookup function may either receive an array of prop/index path steps to get to the desired object/value of the state, resembling the Immutable.js's getIn(['a', 'b', 0, 'class']) method and/or it may take a string representation of the path (a.b.[0]["class"]). Dot ('.') returns an entire state.
The return value, if an array or object, is a copy.

The use of this function can be measured and certain performance bottlenecks identified. (I here recall John Resig's usage measurements of his initial iteration of jQuery, back then..) Still, it must be used first. Bottomline, this may be a pretty unobtrusive way of exposing an entire state to reducers.

As to the pattern, a lot of iterator-depending methods' implementations provide, say: value, key, and then - entire iterated object. In context of the Redux reducers the 'entire iterated object' is the state.

It is not obvious that the previous state gets passed.

Well, the rule is, the iterated object is 'frozen' till the iteration is over, isn't it so? Thus, it simply resolves to previous finalized state.

Did my arguments make any of your strings resonate?
With the best regards,
Andrew.

[gurbraj]

@AndrewRevinsky, thank you for your comments.

I agree with you that the "way of sharing the state seems pretty forceful (reducer cannot opt to NOT get it)". As an alternative solution, what do you think if combineReducer instead took an object of optional functions ( signature (state) => desiredSubState) for each reducerKey, where the functions would only put in desiredSubState into its assigned reducer? I think this might be one compromising way of using your idea of a look-up function. May I ask where you thought that this look-up function would live? How would state be passed to it?

Regarding using store.getState() in reducers.js:

Then store needs to be imported in reducers.js
but store.js imports reducers since it needs to be configured with the reducers.
This can be problematic in a test environment.