Pass storeState as a third argument to reducers when using combineReducers

[1000hz]

This pull request makes reducers have the following signature:
(state, action[, storeState]) => nextState

I first proposed this idea here:
https://productpains.com/post/redux/full-single-state-atom-not-passed-to-reducers

The more I think about it, the more I realize that having access to only the reducer's slice of state severely impacts the architecture of apps. It results in lots of business logic being forced into Action Creators. This means Action Creators are now becoming decision-makers in how state gets updated, rather than fulfilling their purpose of simply formatting an action object. When Action Creators own the business logic of what gets sent, actions end up being more like imperative commands tailored to a specific reducer as opposed to a description of an event that any reducer can react to as it sees fit. This goes against one of Redux's main strengths of decoupling actions from the reducer functions that handle them.

Testing becomes more difficult as now you're having to test your Action Creators for logic that's dependent on state and now buried in the returned action object. Testing reducers is a breeze since they are pure functions and you're testing input state vs. output state.

It also prevents you from reusing Reselect selectors inside of your reducers.

I still think having the sliced state as the first argument passed to a reducer is the right design choice for convenience, as that prevents you from having to slice it yourself and ensure you're returning the correct slice again from the function. However, that convenience should not be the reason for logic being forced into other parts of the application. Having access to the full store state as an additional argument will let reducers keep the separation of concerns clear.

[1000hz]

Any downsides or reservations about this?

[agundermann]

Some downsides I see about this:

• when using storeState, your reducers are coupled to the whole state tree which hurts reusability and (independent) testing
• combineReducers doesn't actually combine reducers anymore, since these functions are not reducers based on their signature, strictly speaking

Your reasoning sounds a lot like the best practice would be to use combineReducers as much as possible and fall back to thunks if things don't work out. I'm not overly familiar with redux best practices and what the docs say, but imo this approach is wrong.

Instead, I would only use combineReducers if you can partition your root reducer like that, and otherwise write your own way of composing reducers/functions which better suits your needs. Starting out with a big, monolithic reducer and refactor later may not always be a bad idea. And I would only use thunks for asynchronous things or, perhaps, when you need to access external APIs (eg. web storage).

After a quick look at the docs, I think they agree with what I said:

• it's mentioned several times that combineReducers is just an (opinionated) helper that you can, but don't have to use. You may write your own
• under "Reducers -> Splitting Reducers" the docs explicitly say that combineReducers can be useful when your reducers are independent; otherwise "more consideration is required"
• thunks are only introduced in relation to async actions

[gaearon]

Thank you for the PR!
Some past discussions on this:

• Question on how to manage dependency between reducers #1518
• Is passing top-level state object to branch reducer an anti-pattern? #1400

Instead, I would only use combineReducers if you can partition your root reducer like that, and otherwise write your own way of composing reducers/functions which better suits your needs. Starting out with a big, monolithic reducer and refactor later may not always be a bad idea.

This is what we recommend.

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.

Alternatively you can pass more information in the actions, use something like https://github.com/slorber/rereduce for expressing those dependencies, or something like https://github.com/reactjs/reselect for calculating derived data outside reducers so that they don’t need to depend on one another.

Hope it helps!

[1000hz]

Thanks for taking the time to think this over with me! However, I'm still not convinced this is a bad idea.

@taurose

when using storeState, your reducers are coupled to the whole state tree which hurts reusability and (independent) testing

That coupling has to live somewhere, and it makes more sense to have reducers be coupled to the state tree than to have action creators coupled to the state tree.

combineReducers doesn't actually combine reducers anymore, since these functions are not reducers based on their signature, strictly speaking

The way I see it, splitting reducers up is purely an organizational decision, not an architectural one. The fact that combineReducers deals with handling state slicing for you is just sugar. As you mention, we could avoid the problems this PR is addressing by eschewing reducer composition altogether and using a monolithic reducer, but who wants to work with thousand-line files? I don't think that reducers not being "strict" reducers is a valid argument against giving them access to the full state tree. In fact, it's actually kind of the problem itself.

it's mentioned several times that combineReducers is just an (opinionated) helper that you can, but don't have to use. You may write your own

The same could be said for pretty much all of Redux. Doesn't mean we can't rethink its design decisions. Opinions can change!

---

@gaearon

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.

As mentioned above, this coupling makes more sense in reducers than anywhere else. Plus, you can use Reselect to move the tight coupling into selectors.

I disagree with the final advice given in #1518 that in some cases the view should determine the next derived state, but I don't want to stray too far off topic.

Alternatively you can pass more information in the actions

Why should action creators be privileged to have access to the whole state tree and not reducers? Why is it ok for them to be tightly coupled to state?

use something like https://github.com/slorber/rereduce for expressing those dependencies

To me, this and most of the solutions presented feel like they're addressing symptoms (reducers need state other than their own) rather than the problem itself (reducers don't have access to state other than their own). Also, it seems like you could easily get yourself at an impasse with circular dependencies with something like rereduce.

or something like https://github.com/reactjs/reselect for calculating derived data outside reducers so that they don’t need to depend on one another.

This works great for reads, but not writes.

---

tl;dr: Reducer composition should only serve as a way to organize your code to make your app more maintainable. App state is a single atom. Reducer composition gives you a way to manage who is responsible for updating parts of the state. It shouldn't constrain what app state is visible, because... well, app state is an atom.

[kurtharriger]

I think those are some good points. Forcing additional state through the action creators increases complexity elsewhere else in a much less centralized way and is more difficult to manage.

[kurtharriger]

Additionally, I might add redux organizational structure already introduces a fair bit of coupling such as global actions that makes it difficult to reuse redux reducers in any other application anyway. So given a reducer is already coupled to an individual application, why not let it take advantage of that?

[1000hz]

I'd appreciate more of a discussion around this when you get a chance. If this truly is a bad idea, I'd love to be convinced of that now before I start using my own version of combineReducers only to later realize it's bad in the future and have to refactor a bunch of stuff.

[gaearon]

I understand your points. I stand by what I said before. Adding emphasis:

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.

[gaearon]

Taking on a dependency on the whole state tree makes reducer a lot more fragile and harder to test. Yes, sometimes, it’s necessary, but usually only for small parts of the tree. I find it important that such changes are as scoped as possible, are very noticeable in code review and are explicitly called out in code. Implicitly passing the whole state tree by default would not hit these goals well.

[1000hz]

I find it important that such changes are as scoped as possible, are very noticeable in code review and are explicitly called out in code. Implicitly passing the whole state tree by default would not hit these goals well.

You'd still need to explicitly reference storeState in your reducer's arguments, which is just as noticeable as using rereduce. You could even use argument destructuring to make the signature the same as with rereduce if you really wanted.

I understand rereduce isn't a default use case, but the reason it exists is that the way combineReducers behaves by default is too limiting for real-world apps with associations between data. For example, if you use normalizr to keep a set of entities in one reducer and have another one that references those entities by id, you don't have a lot of good options. You can tightly couple your reducer's action handler to the action creator and action creator to app state, instrument some hierarchical reducer chain a la rereduce, or just give up and put them both in the same reducer. I'd imagine this is a pretty common scenario people find themselves in, and if everyone has to write their own version of combineReducers then the library isn't being as useful as it could be, especially when a simple solution such as this exists.

As far as scoping goes, this wouldn't make your reducer dependent on the entire state tree. You're only dependent on the piece of data you reference. The only fragility is if you decide to change your other reducer's name or state shape, which would just bite you somewhere else (i.e. the action creator if you chose to pass in extra data via the action). Or, you could save yourself the repetition and use a selector (which can be tested in isolation). I might be missing your point here.

Anyways, thanks for responding. I don't mean to beat a dead horse. It seems like you're pretty firm on this, so I guess I'll go ahead and use my own version of combineReducers if you don't wish to discuss any further.