RFC: Make combineReducers() shape-agnostic

[gaearon]

I propose we should make combineReducers() work with ImmutableJS and any other libraries. This way all the checks it makes would work out of the box, but the actual state transformation for associating the values would be swappable.

I’m thinking something like combineReducers(reducers, [options]).
By default options is something like

{
  create: (obj) => obj,
  get: (state, key) => state[key]
}

but you could supply something like

{
  create: (obj) => (obj instanceof Immutable.Map) ? obj : new Immutable.Map(obj),
  get: (state, key) => state.get(key)
}

Does this make sense to anyone?
If you’d like to work on a PR to make a proof of concept, please comment here and submit it!

If you think it’s a bad idea, please write here as well.

[bvaughn]

I know it's just an example listed, but I'd suggest changing to use fromJS (since it does a deep conversion). Edit: Removed after below discussion.

[markerikson]

Ooo. I _like_ this idea. 👍 💯

[gaearon]

Deep conversion is not necessarily what you want (you might have nested plain object reducers). Also it would be much slower in the common case.

[bvaughn]

Fair enough. In my experience, shallow conversion commonly leads to errors (eg methods like getIn and setIn don't work as expected, you can accidentally mutate nodes in your immutable tree, etc). But I'm sure you've thought about this particular use case much more than I have. :)

[markerikson]

To expand on my comment: this seems useful, simplifies interop, and is consistent with the "sensible default with optional customization" approach used by mapState and mapDispatch. Seems like all-upsides on this one.

[gaearon]

@bvaughn

Since it's pluggable you would be able to customize it. But it would be super confusing if applying it at the root level broke third-party reducers from another packages, for example.

Usually combineReducers() has no effect on the state of the tree below. It just manages one level deep. It would be strange if suddenly it took over the state returned by nested reducers and transformed it. So while the approach you suggest is possible, it isn't something we'd encourage.

[markerikson]

Quick question: How well does this tie in with the other open PRs to combineReducers, #1768 and #1789 ?

[kennetpostigo]

Don't use fromJS it's really expensive operation in immutable lib.

[bvaughn]

There are lots of good reasons to use it, just not in this context as Dan explained. I've struck-through my suggestion so others won't read it and not notice the below discussion. :)

[rt2zz]

Big 👍 . Having this in redux will provide guidance to other projects for how to support this scenario, e.g. this has been a long standing question in redux-persist.

I believe the configuration would also need an iterator method.

[benlesh]

I've personally never been a fan of these sorts of pre and post processing configs. js-csp has something like this I think. I prefer something more functional and composable I think

[deanrad]

It's fine that combineReducers has its utility for delegating responsibility for keys of objects, I just wish the Redux docs showed a simple reducer to be delegation driven as well, ala this gist or simply:

createReducer({
  ADD: (s, a) => s + a,
  MULT: (s, a) => s * a
}, 10)

[markerikson]

@deanius: switches and maps are equivalent. Both have their uses. The docs teach switches first for clarity, with map lookups shown later as a shorter approach. See http://redux.js.org/docs/recipes/ReducingBoilerplate.html and http://redux.js.org/docs/FAQ.html#reducers-use-switch.

Would appreciate any input you have on topics for a "Structuring Reducers" docs page in #1784. Docs PRs also welcome :)

[gaearon]

@Blesh I'm not a big fan of them either but it sounds better than people having to reimplement combineReducers warnings (often lagging behind the main version) by hand. If you have specific other ideas please suggest them though!

[mattkrick]

Not sure if it fits in with this RFC, but while we're talking about combineReducers, I'd love a way to nuke the Unexpected keys will be ignored. warning, which would allow us to use it with 3rd party packages that create a dynamic state.

[aweary]

So this would mean you could pass, say, an Immutable.Map instance as the first argument to combineReducers instead, is that correct?

const reducers = Map({ foo, bar, baz });
const rootReducer = combineReducers(reducers)

If that's the case, wouldn't it also need a way to query all valid keys from the passed instance to build up the finalReducerKeys object?

[markerikson]

@aweary : Ah... no, as I understand the proposal, the idea is that this would modify the actual handling of return values from the sub-reducers. So, rather than var nextState = {} and nextState[key] = nextStateForKey, you'd have var nextState = options.create() and options.set(nextState, key, nextStateForKey). The reducer shape would still be defined using a plain JS object of functions.

[aweary]

So the advantage here is that your top level state returned from the rootReducer could be whatever you returned in create()? That makes sense.

[gnoff]

I'm plus one on this as well. Iterator or keys config seems necessary though since many enhancers examine the top level keyspace and rely on the assumed plain object nature of the atom today

[borkxs]

Quickly threw something together for this with a spec using the ImmutableJS example.

borkxs@7b3315e

Thing to note is that for this to work with ImmutableJS (as far as I can tell) if the user wants to provide a set method it will have to return the new state, it can't rely on side-effects being applied to the state object it gets passed.

Also I wonder if the interface should merge with the default options or require that you provide all three if you want to do anything differently.

EDIT: Also it seems like for it to support ImmutableJS (for the individual reducers) the state comparison would be sort of odd. It would always return a new state unless it did a deep comparison. Should that also be configurable then?

hasChanged = hasChanged || nextStateForKey !== previousStateForKey
...
return hasChanged ? nextState : state

[acdlite]

@gaearon

it sounds better than people having to reimplement combineReducers warnings

Could we put those warnings into a reducer enhancer, to make it easier to replace combineReducers?

[eremzeit]

Quick question: How well does this tie in with the other open PRs to combineReducers, #1768 and #1789 ?

I can speak for #1768. If we're just talking about the ability to define custom create and get semantics (ie. borkxs@7b3315e), this doesn't conflict with my modifications, though I would have to merge those changes into mine or vice-versa.

[bdwain]

@gaearon

If the store was an immutableJs object, it would be much faster to have all of the state changes wrapped in a withMutations block to avoid having to rewrite more than necessary. I'm not sure if there's a good way to write a generic combineReducers that could perform this optimization though.

Is it worth considering that providing a solution that works out of the box for any state type will discourage people from writing a combineReducers designed for their use case? I feel like 90% of cases where this new proposed method was used for an immutablejs state, you'd be better off just writing one optimized for immutablejs anyway.

[markerikson]

I don't think the intent here is to do anything regarding deeper updates. combineReducers "just" iterates keys, calls reducers, and reassembles the results back into an object. The intent here is to make the "key iteration" and "object re-assembling" processes swappable, since a plain JS object and an Immutable Map obviously require different code to do those steps.

[markerikson]

Huh. Interestingly, rt2zz/redux-persist#113 looks like basically the same concept, and that issue is actually waiting on this one to be resolved :)

[bdwain]

@markerikson When you say deeper updates, did you just mean updating lower portions of the state tree?

I was just saying that this block of code from combineReducers (edited to use the generic set get and create options)

    var nextState = options.create({})
    for (var i = 0; i < finalReducerKeys.length; i++) {
      var key = finalReducerKeys[i]
      var reducer = finalReducers[key]
      var previousStateForKey = options.get(state, key)
      var nextStateForKey = reducer(previousStateForKey, action)
      if (typeof nextStateForKey === 'undefined') {
        var errorMessage = getUndefinedStateErrorMessage(key, action)
        throw new Error(errorMessage)
      }
      nextState = options.set(nextState, key, nextStateForKey) //this creates a new intermediate state object every loop iteration if your state is immutable
      hasChanged = hasChanged || nextStateForKey !== previousStateForKey
    }

would be worse than this block written strictly for immutablejs.

   var nextState = Map().withMutations(initState => { //this prevents the issue with the above code
    for (var i = 0; i < finalReducerKeys.length; i++) {
      var key = finalReducerKeys[i]
      var reducer = finalReducers[key]
      var previousStateForKey = state[key]
      var nextStateForKey = reducer(previousStateForKey, action)
      if (typeof nextStateForKey === 'undefined') {
        var errorMessage = getUndefinedStateErrorMessage(key, action)
        throw new Error(errorMessage)
      }
      nextState.set(key, nextStateForKey) 
      hasChanged = hasChanged || nextStateForKey !== previousStateForKey
    }
  });

because of all of the intermediate objects created.

I'm not sure there's a good way to optimize for all libraries, but I feel like providing one that "works" with performance problems will discourage people from writing a combineReducers that actually performs well for their use case.

[markerikson]

Hmm. Interesting point.

For what it's worth, glancing at two existing implementations of combineReducers for use with Immutable.js, one does use withMutations (https://github.com/gajus/redux-immutable/blob/master/src/combineReducers.js), and one does not (https://github.com/indexiatech/redux-immutablejs/blob/master/src/utils/combineReducers.js).

Issue #548 is somewhat relevant as well.

[bdwain]

Yea I've seen it in other places too. redux-loop/redux-loop#51

I always viewed the combineReducers methods included with any libraries as more of a guideline, because writing a good generic one seems like a very difficult problem and it's not that difficult to write one for any specific project.

[markerikson]

So, part of the reason for wanting to change this is that since this combineReducers is included with the core library, it's the one that everyone uses by default. (And, unfortunately, most people also seem to assume that they must use combineReducers to implement reducer logic, not realizing that they can, y'know, actually do some custom structuring of things themselves.)

[bdwain]

At the very least the documentation could reflect that this version of combineReducers is probably not ideal except for a very basic use case.

[borkxs]

Started looking at getUnexpectedStateShapeWarningMessage. First to do something different than the isPlainObject(state) check because that obviously won't work with Immutable.

But then looking at the rest of it, to be able to do this check would mean the user would have to provide a method to iterate over the keys of their state (.keySeq().toArray()) in order to be able to check that the shape is unchanged, or provide a method that compares two states and validates that their shape is what is expected.

I could be missing something but my overall impression is that "shape-agnostic" and "supports ImmutableJS" don't have a lot of overlap.

[bdwain]

@borkxs the object passed to combineReducers is usually a plain js object, even if your state is immutable. it's not the reducer, just a map of names (of subtrees) to reducers.

[borkxs]

@bdwain I'm talking about the method that validates the shape of the previous state, not about checking the input to combineReducers. That will be a plain object, yes.

[bdwain]

Ah my mistake. Yea

On Jun 15, 2016, 6:08 PM -0500, Erik Michaelsonnotifications@github.com, wrote:

@bdwain(https://github.com/bdwain)I'm talking about the method that validates the shape of the previous state, not about checking the input tocombineReducersThat will be a plain object, yes.

—
You are receiving this because you were mentioned.
Reply to this email directly,view it on GitHub(#1792 (comment)), ormute the thread(https://github.com/notifications/unsubscribe/ADzDDiBq8CN6GfCswDDzzarrIFvU_Z6Kks5qMIXWgaJpZM4IufK5).

[Kureev]

What would be the next actionable step I can help you with?

[Florian-R]

What would be the next actionable step I can help you with?

There was an attempt in #1817, but @timdorr closed it, so this seems pretty stalled.

[ncphillips]

Rich Hickey gave a talk at Strangeloop about Transduceres.

It seems pretty relevant to this discussion.

https://www.youtube.com/watch?v=6mTbuzafcII

[markerikson]

Realistically, I don't think this is ever going to happen. I still sorta think it'd be neat if combineReducers was customizable, but at this point it's not worth the effort.