.map function always returns new object

[idolize]

This isn't necessarily a bug, but I just recently learned that ImmutableJs's .map method always returns a new object, thus breaking === equality tests to see if something has changed. This can be a big deal if you are using something like reselect to memoize your state so you don't trigger expensive re-renders when the state hasn't changed. (Immutable.is is one workaround, but that does a deep equality check, so it negates a lot of the benefit of using ImmutableJs in the first place)

Is there some way we could avoid using .map in combineReducers? I know it looks elegant and works well from a correctness point-of-view, but the practical benefit of using .set or .merge (and thus allowing simple === checks for state changes) could be worthwhile for users of this library.

[davidmason]

You could just define a new mapping function that uses .forEach() and .set()

function keepReferenceMap(state, fun) {
  let newState = state
  state.forEach((val, key) => {
    newState = newState.set(key, fun(val))
  }
  return newState
}

It is a little less elegant than just calling .map() but not that bad:

import { keepReferenceMap as map } from 'keep-reference-map'

...
state = map(state, val => {
  return 8;
})

[kainoa21]

@davidmason I don't think this solves the problem as calling .set() is also going to return a new object so newState !== state.

I am thinking that the only way that state === newState after running the reducer is if the reducer returns the state object it was given. So we should check for that and only mutate our state if the reducer has.

finalReducers.forEach((reducer, key) => {
    var oldState = state.get(key);
    var newState = reducer(oldState, action);
    if (typeof newState === 'undefined') {
        throw new Error(getErrorMessage(key, action));
    }

    // Check to see if this reducer mutated the state
    if (newState !== oldState) {
        state.set(key, newState);
    }
});

// If none of the reducers mutated the state then we are returning the same object so any === checks will work
return state;

[davidmason]

@kainoa21 I thought immutablejs kept reference equality with .set when oldval === newval, if not then I agree with that check - actually I used exactly that in a proposed fix for the React immutability helper to keep reference equality: facebook/react#4968

[idolize]

@davidmason Yep, .set() already does this check, so there is no need to do it manually. (See here for an example illustrating just that)

Thinking about this more, it seems like there are a few important considerations here:

1. (Most important) Are people actually "selecting" the slice of the state that is returned by combineReducers?
   • I actually jumped the gun opening this issue, because in my own application I realized I never actually access this "top-level" Map directly - I just create more specific "selector" functions which access the children of the combineReducers state Map anyway
   • The reselect memoize functionality doesn't care if the parent has changed or not – only the specific item you "select" – so there is no real downside to using .map() to implement combineReducers if you aren't "selecting" this Map directly
2. How many reducers are people typically "combining" with combineReducers?
   • Since each call to .set() could potentially allocate a new object, there is a performance benefit to batching these into a single allocate (which is exactly what .map() and .withMutations() do)
   • If there are a lot of reducers being "combined", then that means a lot of calls to .set(), and thus potentially a lot of object allocations (which is slow), but if it's only a few then the performance difference is likely negligible
   • Also, if the answer to question (1) is "no" then the clear winner is to use .map() or .withMutations() regardless of how many reducers people are typically "combining"

[kainoa21]

1. I imagine that the most common usage will be as you described with selectors accessing the various domains which are the input reducers to combineReducers. But there are certainly use cases for selectors that will look at the root state object.
2. I think we could rephrase the question to be: How many reducers are actually mutating state for a given action?
   • Since each call to .set() will only allocate a new object when the state has been mutated, then this is really the performance impact to consider
   • I think the answer here is very likely to be that a single reducer is mutating the state for a given action, or at the very least a small number (not likely the entire list of reducers being combined).
   • Even then, one possible solution to mitigating performance impact would be to simply wrap all of the necessary .set() calls inside a .withMutations()
3. Another consideration: How likely is it that an action will NOT mutate the state whatsoever?
   • This is the only scenario in which using .set() over a simple .map() could potentially provide any benefit. And even then, that would be further limited to scenarios where selectors are accessing and doing a reference equality check on the root state object.

[davidmason]

I would expect to see multiple levels of combineReducers() in non-trivial apps, at least some of the time, but I don't see that being relevant to .map()

I guess the bottom line is that there may be different performance characteristics between the current .map() and a different strategy using .set() and .withMutations(). I think a sensible next step would be to compare the performance of .map() and the other strategy across a range of scenarios, and continue from there.

Something like this would be the thing to compare:

function keepReferenceMap(state, fun) {
  return state.withMutations(function (state) {
    var newState = state
    state.forEach(function (val, key) {
      newState = newState.set(key, fun(val))
    }
    return newState
  }
}

Are there already performance tests for immutable? If so, it would just be a matter of replacing .map() and running them.

[idolize]

.withMutations always returns a new object, so it wouldn't really be of
much help here FYI
On Sep 27, 2015 4:36 PM, "David Mason" notifications@github.com wrote:

I would expect to see multiple levels of combineReducers() in non-trivial
apps, at least some of the time, but I don't see that being relevant to
.map()

I guess the bottom line is that there may be different performance
characteristics between the current .map() and a different strategy using
.set() and .withMutations(). I think a sensible next step would be to
compare the performance of .map() and the other strategy across a range
of scenarios, and continue from there.

Something like this would be the thing to compare:

function keepReferenceMap(state, fun) {
return state.withMutations(function (state) {
var newState = state
state.forEach(function (val, key) {
newState = newState.set(key, fun(val))
}
return newState
}
}

Are there already performance tests for immutable? If so, it would just
be a matter of replacing .map() and running them.

—
Reply to this email directly or view it on GitHub
#11 (comment)
.

[davidmason]

.withMutations always returns a new object

It sounds like there is room for a review of immutable's API to assess it specifically for use-cases that want to use reference equality - initially to make a reference doc for anyone wanting to use reference-equality, then to figure out if there is a set of changes that would help support the use-case while having a neutral or positive effect on the other uses of immutable.

One use-case that would benefit from maintaining reference-equality is React with the pure render mixin. Another is using reselect to memoize state-selecting functions.

[dgreisen-cfpb]

I ran into this issue as I was creating a history reducer. I only wanted to push a new history when the state changed, but because a new object was returned every time, my history was getting cluttered with entries that didn't actually represent changes.

Just created a pr #15 which addresses this issue.

[asaf]

Folks, sorry for responding within a delay, holidays in Israel :-)
I wasn't involved in this discussion but it seems like @dgreisen-cfpb (thanks!) PR solves this issue, I merged it and released a new version.

Thanks !

[asaf]

@idolize Are you feeling comfortable with this solution?

[idolize]

@asaf Yeah 😄 Thanks everyone for the discussion, and thanks @dgreisen-cfpb for the simple (but effective) solution!