Actions that didn't change the state shouldn't be written in history

[gaearon]

Or maybe this should be configurable.. I guess?

You can reproduce the problem in https://github.com/rackt/redux/pull/777:

1. Add a couple of todos
2. Switch visibility filter to "Active"
3. Press "undo"

Expected: todo is removed. Actual: nothing happens until you press it again.

Reason: currently even the actions that did not result in state modification are written to history. Proposed solution: don't write actions to the history if state === previousState.

[danielkcz]

@gaearon There is surely some problem, but I don't think the solution you propose is a good one. Consider the scenario where you have a different parts of state you want undoable. These would get desychronized when only one of them changes (which can easily happen) based on action. Doing undo would create some not so pretty mess. You would essentially need separate UNDO/REDO action for each state piece, but that's quite mindblowing (at least for me).

Perhaps this should support multiple slices naturally so they cannot get into bad shape. I am not entirely sure how to approach to this.

[gaearon]

Consider the scenario where you have a different parts of state you want undoable. These would get desychronized when only one of them changes (which can easily happen) based on action.

Why is this a problem? If you want them to be in sync, put undoable on their shared parent reducer.

[danielkcz]

And what if I don't have shared parent reducer? I would need to change whole structure of the state just to be able use undo? I don't like that much and I can imagine not many people are thinking ahead and grouping together data that might be undoable eventually.

export default combineReducers({
    tgDocument: undoable(tgDocument, undoableOptions),
    tgFocus: undoable(tgFocus, undoableOptions),
    tgZoom,
    tgCommand,
});

And that is still the easy case. Some people have quite hierarchical states and might want to undo only part of that.

[gaearon]

Then I think each undoable call should have action types specified as mandatory. There shouldn't be two undoables in your reducers if you want them to act as one. There shouldn't be a way to have a single action dispatch(undo()) affect two completely unrelated undoables unless the user specifies this explicitly. There are many ways you can get weird inconsistencies: for example, different history lengths and a single action means they will always get out of sync.

If you want to have a single history, I believe yes, you should take care of it right away:

export default combineReducers({
    tgData: undoable(combineReducers({
      tgDocument,
      tgFocus
    }), undoableOptions),
    tgZoom,
    tgCommand,
});

Then you have a single consistent history of states and actions.

[danielkcz]

I think it needs to be wrapped to something like this.

import { createUndoable } from 'redux-undo';
const undoableDocument = createUndoable({
    limit: 1024,
    filter: ....
});

export default combineReducers({
    tgDocument: undoableDocument(tgDocument),
    tgFocus: undoableDocument(tgFocus),
    tgZoom,
    tgCommand,
});

I am not sure yet how it would work inside, but key is basically to copy the previous state to the current one for the reducer that didn't produced the change while some other did. That way we can ensure that invoking UNDO would use synchronized states.

[danielkcz]

@gaearon As I am saying, mine is still the easy case, but I can imagine the complex one and I believe that not many people would like to change their state composition when the request to implement undoable comes in. That might be a lot of work for complex applications.

[gaearon]

Maybe I'm just missing your point. You need to change it anyway because you need to add currentState. This is a change on the same scale and in the same place.

[gaearon]

What you suggested above seems to involve hidden state. If this is the case, it won't work with DevTools.

Maybe we can introduce the behavior I described with an opt-in parameter? Like yet another kind of filter, which filters using previous and current state instead of based on action.

[gaearon]

Say, filterAction can accept (action, state, previousState). That wouldn't even be a breaking change.

[omnidan]

I like the filter(action, currentState, previousState) solution - I could also make the helpers filter out everything that is currentState === previousState. I don't see the need of introducing another filter function for that.

Maybe a function that filters out currentState === previousState should also be the default filter function.

As for rackt/redux#777 - I think a simple solution to the problem would be using the ifAction helper on todo actions only? If I add the default filter this wouldn't even be needed, though.

@FredyC @gaearon what do you guys think? would that solve your problems? :grin:

[gaearon]

I probably wouldn't go as far as making it a default, but exposing such filter alongside ifAction will be helpful. We could call it distinctStates.

[gaearon]

I don't want to use ifAction in the Redux example because it's a time bomb: somebody adds a new action operating on that state, forgets to add it to the list, and we get inconsistent undo history.

[omnidan]

@gaearon why not make it default though? I can't think of a way this could break anything. If the state is exactly the same, why would you want to store it again?

[danielkcz]

Maybe I'm just missing your point. You need to change it anyway because you need to add currentState. This is a change on the same scale and in the same place.

Well, @omnidan is working on solution using Symbol, so there wouldn't be any currentState and wont need to change anything in ideal case. However when there will be requirement of having more undoable parts of the state, you suddenly would need to change it. It just doesn't feel right to change my application state structure just because I want undo functionality.

Exposing states to a filter function definitely sounds like a good way to go and I agree with @gaearon that distinctStates sounds better like helper. It shouldn't be too magical by default, it might confuse some people.

[omnidan]

Alright, I'll implement the new filter function and the helper :smile:

It can always be made default later if we decide it does make sense, so we don't need to decide now :sweat_smile:

[gaearon]

Well, @omnidan is working on solution using Symbol, so there wouldn't be any currentState and wont need to change anything in ideal case.

Where can I read a discussion on this? Using symbols to hide essential parts of state seems like a move towards implicitness.

[danielkcz]

@gaearon #8

[danielkcz]

I personally see the redux-undo as something that you can easily plug-in or take out and you don't need to really care about it's internals (unless you want to disable undo button) or change your app to be able use it. I don't know if @omnidan has a different view on it

[omnidan]

I personally see the redux-undo as something that you can easily plug-in or take out and you don't need to really care about it's internals (unless you want to disable undo button) or change your app to be able use it.

I agree, but as @gaearon said, there are some issues with it and it hides important details from the developer. And it's a breaking change anyway, which is never good :sweat_smile:

[gaearon]

Breaking changes are fine in early days; it's just that you need to be careful what to change. :wink:

[omnidan]

@FredyC @gaearon thank you so much for all your discussions, feedback and help - I really appreciate it :grin:

I just published 0.4: it uses .present and has the new filter(action, currentState, previousState) function as well as the distinctState helper. I also updated the example, @gaearon :wink:

[codeclown]

In case anyone lands here from Google, like I did, here's an example of how to implement this:

undoable({
    filter: (action, currentState, previousHistory) => {
        const previousRevision = previousHistory.present;
        const isEqual = JSON.stringify(currentState) === JSON.stringify(previousRevision);
        return !isEqual;
    }
})