Possibility to have flat redux structure

[nosovsh]

Currently redux-undo adds one more level of hierarchy to your redux state:

{
  past: [...pastStatesHere...],
  present: {...currentStateHere...},
  future: [...futureStatesHere...]
}

Maybe it's a good default but if you already have a big application and want to start using redux-undo in different places you will face a problem that you have to change paths to you state in all your connect functions from path.to.my.state to path.to.my.state.present. Which can cause a lot of bugs and anyway not a very pleasant thing to do. My idea was to add option to undoable reducer that will force it to use such state structure instead:

{
  ...currentStateHere...
  past: [...pastStatesHere...],
  future: [...futureStatesHere...]
}

Setting option:

combineReducers({
  counter: undoable(counter, {
     flat: true,
  })
})

A far as I can see it can be easily implemented. What do you think?

[davidroeca]

See #154 for some discussion on this. While it's (somewhat) easy to implement, name collisions are the main issue we want to avoid, so if you can think of a way to handle name collisions I'm all ears

[nosovsh]

hi @davidroeca , thanks for the quick answer. I see that I'm not the first facing this problem. Naming collisions can be prevented in few steps:

1. You have to explicitly set flat: true in order to use flat structure. So it is advanced scenario and users who know what they are doing will use it.
2. We can raise warnings when user's reducer returns key that is used by undo.
3. And we can hide everything in one key to lower probability of collision, like

{
  ...currentStateHere...
  __reduxUndoHistory: {
    past: [...pastStatesHere...],
    future: [...futureStatesHere...]
  }
}

@ugo-buonadonna I see that you make a fork with flat structure. Did you face any problems caused by such structure?

[davidroeca]

These are the trade-offs that make me less inclined to adopt this approach:

1. This breaks support of alternative state data structures such as the Immutable Map.
2. Specifying flat: true changes the behavior of the entire library and makes comparisons between the present and other elements of the undo/redo history (e.g. distinct state comparisons) much more resource-intense. Instead of comparing references, the library will have to compare, at the very least, the reference (if not a bool/string/int) of every relevant key's value to determine distinct states, with the exception of _reduxUndoHistory.

If the main headache is the selective use of the library at sub reducers, maybe an alternative to this is to make it more practical to wrap the combined reducers at the top level with redux-undo. I.e. we could make it easier to specify which subsections of the tree you want to track in the undo history--that way, any refactoring is minimized to top-level state access

[davidroeca]

@nosovsh for the time being, would the following sort of filtering helper be useful at all?

// say this is included in the library or in your code somewhere
export function basicGetIn(state, path) {
  return (state) => {
    let currentObj = state
    for (const key of path) {
      if (typeof currentObj === 'undefined') {
        return undefined
      } else {
        currentObj = currentObj[key]
      }
    }
    return currentObj
  }
}

// accessFunc could be the return value of basicGetIn
export function includeSubsection(accessFunc) {
  return (action, state, prevHist) => (
    accessFunc(state) !== accessFunc(prevHist._latestUnfiltered)
  )
}

The use case would be the following:

import undoable, { includeSubsection, basicGetIn } from 'redux-undo'
// assume we only want to undo the following portions of the undoable state:
// {
//   'userState1': {...I want to undo this state...}
//   'nested': {state: {with: {user: {state: {...I want to undo this state...}}}}},
//   ...I want redux-undo to not update the past with remaining keys...
// }

// necessary because combineFilters is an 'and' operation
const orFilters = (...filters) => {
  return filters.reduce(
    (prev, curr) => (action, state, prevHist) => prev(action, state, prevHist) || curr(action, state, prevHist),
    () => false
  }
}

const finalReducer = undoable(reducer, {
  filter: orFilters(
    includeSubsection(basicGetIn(['userState1'])),
    includeSubsection(basicGetIn(['nested', 'state', 'with', 'user', 'state']))
  )
})

In theory you could also pass a memoized selector as the argument to includeSubsection to improve performance. This would let you merely wrap your top-level reducer with redux-undo and only change your state tree in one place.

EDIT: Made a few updates to the code and switched from combineFilters since inclusions are OR'd rather than AND'ed

[nosovsh]

Do I understand correctly that you suggest to put this at top level app reducer? And inside filter put all paths I need to make undoable? So the state of the whole app would be

{
  present: {
     'userState1': {...I want to undo this state...}
     'nested': {state: {with: {user: {state: {...I want to undo this state...}}}}},
     ...I want redux-undo to not update the past with remaining keys...
  },
  past: ...
  future: ...
}

right?

[davidroeca]

Yes. Or the highest level it needs to be, wherever you plan to put it. That way there are fewer presents to worry about

[davidroeca]

Also updated the orFilters function to do what the name intends (previously used the && operator which is basically combineFilters)

[nosovsh]

But that means that all paths in app in connect/selectors should be updated even which don't need undo :)

I come up to the following reducer-wrapper that I can use in my code which will preserve my old state structure and will add undo state. What do you thing?

// myReducer.js

// original reducer
const myReducer = (state={}, action) => ...

// reducer wrapper with undoable state
export default (state={}, action) => {
  const newUndoableState = undoable(myReducer)(state.undoable, action);
  if (newUndoableState === state.undoable) {
    return state;
  } else {
    return {
      ...undoableState.present,
      undoable: newUndoableState,
    }
  }
}

[davidroeca]

Hmm undoableState.present should be newUndoableState.present. But more importantly, the undoable reducer actually has a state of its own (initialization), so it shouldn't be created more than once. I'd say if anything, do the following:

// myReducer.js

// original reducer
const myReducer = (state={}, action) => ...

// the undoable portion
const undoableReducer = undoable(myReducer)
// reducer wrapper with undoable state
export default (state={}, action) => {
  const newUndoableState = undoableReducer(state.undoable, action);
  if (newUndoableState === state.undoable) {
    return state;
  } else {
    return {
      ...newUndoableState.present,
      undoable: newUndoableState,
    }
  }
}

Let me know if that gives you the desired behavior--should merely copy the references of the values in newUndoableState a level up

[nosovsh]

Thanks for tips. I will try it in real project this week

[dbkaplun]

This can be solved simply:

{
  ...currentState,
  [PAST_KEY]: [...],
  [FUTURE_KEY]: [...]
}
export const PAST_KEY = '@@reduxUndo/past';
export const FUTURE_KEY = '@@reduxUndo/future';

Prior art

[omnidan]

@dbkaplun that solves one of the problems. however, there are still other tradeoffs, as mentioned by @davidroeca earlier here: https://github.com/omnidan/redux-undo/issues/179#issuecomment-316475124

if we can find a solution to these issues, it would be great if we could switch to a flat structure, as it would make it even easier to integrate redux-undo.

[vincerubinetti]

For anyone stumbling across this, here's a solution I came up with that's similar to @nosovsh 's before I found this thread.

export const undoer = (reducer) => ({ past = [], future = [], ...present } = {}, action) => {
  const newReducer = undoable(reducer, { OPTIONS });
  const newState = newReducer({ past, present, future }, action);
  return {
    ...newState.present,
    past: newState.past || [],
    future: newState.future || []
  };
};

You wrap your main/root reducer with this undoer higher-order reducer. I haven't tested this thoroughly yet but it seems to work.

It takes the incoming state (first argument, before action), and destructures the past and future keys out of it, and takes the rest of the keys as the present. Then it uses this library to convert your main reducer to an undoable one. Then it runs the reducer with the past, present, and future keys that this library expects and gets the new state. Finally it transforms this new state back into the schema you want, { ...state, past, future }, and returns it.

[posita]

I took a different approach:

import { StateWithHistory } from "redux-undo"

type WrappedSelectorFnT<S, R> = (state: StateWithHistory<S> | S) => R
type SelectorFnT<S, R> = (state: S) => R

export const wrapPresentStateSelector = <S, R>(
  selector: SelectorFnT<S, R>
): WrappedSelectorFnT<S, R> => {
  return (state: StateWithHistory<S> | S): R => {
    return selector(presentState(state))
  }
}

export const presentState = <T,>(state: StateWithHistory<T> | T): T => {
  if (stateHasHistory(state)) {
    return state.present
  } else {
    return state
  }
}

const stateHasHistory = <T,>(state: StateWithHistory<T> | T): state is StateWithHistory<T> => {
  return (
    "past" in state
    && "present" in state && Array.isArray((state as StateWithHistory<T>).past)
    && "future" in state && Array.isArray((state as StateWithHistory<T>).future)
  )
}

You can write your selector as if the state isn't undoable, then wrap it with wrapPresentStateSelector. The resulting wrapped function should work the same whether called with a raw state or an undoable one. Alternatively, if you're being handed state elsewhere, you can apply presentState.

import { StateWithHistory } from "redux-undo"
import { presentState, wrapPresentStateSelector } from "./undoable"

interface State {
  one_fish: number;
  two_fish: number;
  red_fish: string;
  blue_fish: string;
}

type UndoableState = StateWithHistory<State>

const rawSelector = (state: State): string => {
  return `${state.one_fish} ${state.red_fish}, ${state.two_fish} ${state.blue_fish}`
}

const wrappedSelector = wrapPresentStateSelector(rawSelector)

describe("undoable", () => {
  describe("state", () => {
    it("should return the raw state itself", () => {
      const raw_state: State = {
        one_fish: 1,
        two_fish: 2,
        red_fish: "red",
        blue_fish: "blue",
      }
      const unwrapped_state = presentState(raw_state)
      expect(unwrapped_state).toEqual(raw_state)
    })

    it("should return the raw state from the wrapped state", () => {
      const raw_state: State = {
        one_fish: 1,
        two_fish: 2,
        red_fish: "red",
        blue_fish: "blue",
      }
      const undoable_state: UndoableState = {
        past: [],
        present: raw_state,
        future: [],
      }
      const unwrapped_state = presentState(undoable_state)
      expect(unwrapped_state).toEqual(raw_state)
    })
  })

  describe("selector", () => {
    it("should use the raw state itself", () => {
      const raw_state: State = {
        one_fish: 1,
        two_fish: 2,
        red_fish: "red",
        blue_fish: "blue",
      }
      const result = wrappedSelector(raw_state)
      expect(result).toEqual("1 red, 2 blue")
    })

    it("should use the raw state from the wrapped state", () => {
      const raw_state: State = {
        one_fish: 1,
        two_fish: 2,
        red_fish: "red",
        blue_fish: "blue",
      }
      const undoable_state: UndoableState = {
        past: [],
        present: raw_state,
        future: [],
      }
      const result = wrappedSelector(undoable_state)
      expect(result).toEqual("1 red, 2 blue")
    })
  })
})

It's a little dicy if you give up Typescript checking and have past, present, and future in your non-wrapped state, though. That could be mitigated by, e.g., adding another property to StateWithHistory that is unlikely to appear in the underlying state (e.g., "@redux-undo/StateWithHistory": true or type: "@redux-undo/StateWithHistory" or similar).