Preserve any extra keys for composability

[sheerun]

This is a fix for behavior noticed in #2058 that is considered by @markerikson as "working as intended", but effectively making reducers produced by combineReducers not composable. I strongly disagree with this opinion, so I ask for second opinion of @gaearon before you close this PR.

PR solves an issue where I compose reducer produced by combineReducers with other reducer (for example by using reduceReducers, but also simple applying):

const result = combinedReducer(otherReducer(state, action))

const composedReducer = reduceReducers(combinedReducer, otherReducer)
const result = composedReducer(state, action)

If otherReducer returns keys not used in combinedReducer, then combinedReducer not only shows a warning that Unexpected key "extra" found in previous state received by the reducer (what I can leave with evenrually), but also removes extra keys set by otherReducer.

This severly limits composability of reducers, and I think it shoudn't be a default behavior of redux.

You can see simplest example possible in test I attach to this PR, where an extra key is present in state passed to reducer produced with combineReducers, but this keys is missing in result.

This modification is fully backward compatible, I hope you agree to include it.

[sheerun]

Travis build error is not related to PR. All tests are passing.

[naw]

I agree with @markerikson that combineReducers is working as intended. Removing extra keys is very intentional. Whether it should be intentional is debatable.

Some people want extra keys removed (see #1748, for example), whereas some people want extra keys retained (such as yourself). I think you could make convincing arguments either way.

The bottom line is that combineReducers can't please everyone, and trying to get people to agree on an ideal version is an exercise in futility.

A variation of combineReducers that preserves unknown keys seems like a useful pattern for certain situations, and in all honesty, I'd probably have slight preference for that over the current behavior.

However, I doubt there is a compelling reason that this should be the default behavior for everyone, so generally I think I'd be opposed to this PR. It's easy to build what you need in user land or release a 3rd party library with your own version of combineReducers that does exactly what you want. It's also feasible to build your state shape so that combineReducers only manages a slice.

[alechill]

For what its worth I just came upon this PR because I had this exact same issue today, using this (simplified) reducer pattern...

const rootReducer = combineReducers({
    branch: reduceReducers( branchReducer, combineReducers({
        leaf1: leaf1Reducer,
        leaf2: leaf2Reducer
   }))
})

This pattern is useful when there is also an optional leaf3 in the state which cannot have its own sliced reducer (as cannot have undefined as default accumulator) but can be accounted for in the branchReducer default state

While I understand that combineReducers can't be and shouldn't be all things to all people, I find it a little counterintuitive that composability is actively discouraged here, as redux champions it elsewhere.

Its such a simple change it seems a shame to have to recreate the entire thing from scratch, perhaps an optional allowComposability: boolean = false or preserveUnspecifiedKeys: boolean = false or whatever flag arg so you can enable it if you know what you're doing, but still maintain backwards compatability with the super-safe opinionated default behaviour?...the warnings about unexpected keys etc will still be useful to catch any mistakes for those using the composability feature

[sheerun]

I understand warnings could be useful, but I don't understand how removing keys helps anyone, except confusing. On top, you make it impossible to follow your own guidelines:

You may need to write some custom functions for handling some of these actions. This may require replacing combineReducers with your own top-level reducer function. You can also use a utility such as reduce-reducers to run combineReducers to handle most actions, but also run a more specialized reducer for specific actions that cross state slices.

With combineReducers removing extra keys, it's hard to reliably use reduce-reducers with it.

You're also loosely breaking one of the rules of redux:

If no changes are needed, it should return the existing state as-is.

Strictly speaking, you're not breaking this rule, but if reducer passed to combineReducers returns something like Object.assign({}, state) instead of state, you completely change the behavior and remove any extra keys. I vote it inconsistent and unnecessary.

Removing extra keys is not a behavior isn't documented anywhere (yet). But instead of documenting it, I suggest removing this side effect all together.

[naw]

@alechill

Its such a simple change it seems a shame to have to recreate the entire thing from scratch

It's completely feasible to build an adapter in front of combineReducers that does what you want without recreating anything from scratch.

Here is an example:

const adapter = (reducerHash) => {
  const combined = combineReducers(reducerHash);
  return (state, action) => {
    return Object.assign({}, state, combined(state, action));
  }
}

Then, wherever you would normally call combineReducers to create a reducer, you call adapter instead.

If you want to avoid the warnings too you can enhance this example to restrict what you pass to combined based on the keys of reducerHash.

My point is simply that you don't have to start from scratch.

[timdorr]

@naw is right on this. combineReducers() shouldn't be composable. That's not its job, nor is it how it's built. The composition should happen externally to it, creating a sort of "meta-reducer" that wraps up all your reducer functions. That means it can be used as a tool in your reducer building, rather than a necessary component to it (it's not meant to be the primary way to build your state tree!).