Preserve state reference if unchanged and not in transaction

[aduth]

Fixes #22

This pull request seeks to resolve an issue where redux-optimist returns a new state reference on every dispatch, regardless of whether the original reducer has in-fact returned a new value.

The approach is perhaps not quite as elegant as I'd hope, since it moves logic from what might otherwise be considered part of the baseReducer into the return function of the top-level optimist. After banging my head at several attempts trying to incorporate into baseReducer while also accommodating commitReducer and revertReducer, this is what I landed on.

The idea is that if we're not in the middle of a transaction and the original reducer produces a strictly equal value to the previous state, we shouldn't bother to separate state, which is not only potentially costly (in iterating over the spread assignment of innerState from separateState) but is the root cause for the new reference being returned (destructuring this way will guarantee innerState is always a new value).

I've included a test case which fails in master, and granted you access to edit my branch, in case you'd like to hack away at a refactor while achieving the same intent.

[ForbesLindesay]

We should still be separating out the state, I think. otherwise the fn gets called with a different value depending on whether there is a transaction in progress. My suggestion would be to do:

const separated = separateState(state);
const nextState = fn(separated.innerState, action);
if (nextState === separated.innerState) {
  return state;
}
validateState(nextState, action);
return nextState;

[aduth]

Thanks for the feedback @ForbesLindesay . I've made updates in 087cba9 to ensure that the original reducer is called with only the innerState value. It seems to be a fair bit more simplified now overall.

[aduth]

And for good measure, added an extra unit test in 1682380 to verify the expected behavior that the original reducer is not passed the state with optimist.

[ForbesLindesay]

I think there are two new problems here:

1. If optimist.length > 0 we should skip straight to baseReducer, not also run fn first.
2. It looks like fn is potentially getting called twice here (if it applies a change). The fn should only ever be called once per action.

[aduth]

I think there are two new problems here:

1. If optimist.length > 0 we should skip straight to baseReducer, not also run fn first.
2. It looks like fn is potentially getting called twice here (if it applies a change). The fn should only ever be called once per action.

Thanks for the continued feedback. This is addressed in 46b6329. I still get a sense there's some redundancy here with baseReducer (validating and returning the new merged state), but it's proven difficult to refactor baseReducer while also accommodating its use in the other reducer handlers.

[ForbesLindesay]

I think it's ok that some of the code is sort of duplicated, as long as we're not running potentially costly reducers unnecessarily.

[aduth]

Thanks @ForbesLindesay . While not entirely relevant to the changes here, considering how the destructuring operates to separate state, an operation which may end up not yielding any changes, the pragmatist in me wonders if it's even worth trying to keep the optimist array in state itself, vs. a variable scoped to the return value of optimist( reducer ), avoiding the need for separating or combining the array from the innerState. Food for thought, in any case.

[ForbesLindesay]

The problem with doing that is that it wouldn't play nicely with other clever reducer additions like an "undo"/"redo" reducer. It's important that we keep all our state in redux's store. Options that could be explored would be having the state always be {optimist, value}, but that requires all connect calls to separate out the value key, or passing optimist into the fn anyway, which would save us splitting it out, but does feel less "clean".