omnidan / redux-undo

:recycle: higher order reducer to add undo/redo functionality to redux state containers
MIT License
2.91k stars 188 forks source link

Redux undo not working as expected after upgrading to 1.0 #261

Closed jordanrigo closed 4 years ago

jordanrigo commented 4 years ago

Hello,

After upgrading from 0.6.1 to 1.0.0 my state management is not working as before. Here is my implementatio:

const initialState = { hiers: [{name: 'initial state'}] };
const reducer = (state, action) => {
  state = state || initialState;
  switch (action.type) {
    case receiveHierarchies:
      return {
        ...state,
        hiers: action.payload
      };
    case reorderHierarchies:
      return {
        ...state,
        hiers: action.payload
      };
    case receiveHierarchy:
      return {
        ...state,
        hiers: state.hiers.map(hier => {
          if (hier.name === action.payload.name) {
            return action.payload;
          }
          return hier;
        })
      };
    case updateHierarchy:
      return {
        ...state,
        hiers: state.hiers.map(hier => {
          if (hier.name === action.payload.name) {
            return {
              ...action.payload,
              hierLevels: cloneDeep(action.payload.hierLevels)
            };
          }
          return hier;
        })
      };
  }
};

export const HierReducer = undoable(reducer, {
  filter: includeAction([updateHierarchy])
});

So basically what I would like to achieve is that I only want to add updateHierarchy action to history. The filter is working perfectly.

My problem is, when invoke updateHierarchy than the past array's new value is going to be the initial state. But this is not how the library used to work and it is not the expected behaviour. W

I tried to set ignoreInitialState to true. (ignoreInitialState: true,) This way the initial state was not added to the past array. But then another problem happened. For the first invoke of the updateHierarchy nothing happened. I had to invoke the action once again for it to work properly.

I am going to insert a few images about my redux store's state.

This is the state when I start the app. The _latestUnfiltered array contains my initial state. image

So when I invoke updateHierarchy the initial state is going to be added to the past array: image

ON the other hand when I set the ignoreInitialState to true the latestUnfiltered is null: image

So when I invoke the updateHierarchy for the first time then the latestUnfiltered's value is changed. But this way the past array stays empty and the first modification with the updateHierarchy is lost: image

How could I solve this problem?

Thank you,

Jordan

nmay231 commented 4 years ago

Hi Jordan!

So this is the intended behavior in versions 1.0 and up.

When you used ignoreInitialState, the first change caused by updateHierarchy will not be commited to history immediately. That's why _latestUnfiltered was set to the new state after the action was dispatched. When a second updateHierarchy is dispatched, _latestUnfiltered will be pushed into the past array. In effect, your state is not lost. It is only delayed being saved to history. You can read more about it here.

Hope that helps :+1:

jordanrigo commented 4 years ago

Hello,

Well It kinda helped me and kinda not :D

I understand that this is the inteded behavior, but I still do not know how to handle it and have the same behaviour as previously.

If I do not use ignoreInitialState then the const initialState = { hiers: [{name: 'initial state'}] }; will be commited to history ( this did not happen with the previous version), butI would not like to save this initial state in the past array, because than the user would be able to click 'Undo' and modify the hiers[] array back to hiers: [{name: 'initial state'}] thus it would ruin my implementation logic.

But if I set the ignoreInitialState to true, thenthe initial state will not be commited to history which is good, but as you described

the first change caused by updateHierarchy will not be commited to history immediately. That's why _latestUnfiltered was set to the new state after the action was dispatched

So my question is how could I not save the initial state to history, but still be able to even save the first updateHistory action and push it to the past array as well? This is still not clear for me. Because if the updateHierarchy action invoked only once, then the past array will stay empty thus the undo functionality will not work, until that action is fired again.

Thank you very much! I appreciate your help.

nmay231 commented 4 years ago

Ah, I see now.

First, try setting ignoreInitialState and syncFilter to true. That might solve your issue.

If not, then also dispatch a commitToHistory action after every updateHierarchy and change the filter to only include that action type:

filter: includeAction(commitToHistory)

Digging through past issues shows that there was always an issue of what is the best way to deal with filtered actions and past. I'll need to look into this further to see if there is a more satisfactory solution.

jordanrigo commented 4 years ago

Hello,

First, try setting ignoreInitialState and syncFilter to true. That might solve your issue.

This solved my problem. _latestUnfiltered is synchronized with present state when an excluded action is dispatched. So when receiveHierarchy action, that is an excluded action, is dispatched then _latestUnfiltered updated perfectly and my app is working as expected.

Although I got one additional problem. When I use clearHistory() then not just the past and future arrays get emptied, but the _latestUnfiltered becomes null. So the same issue occurs that only my second updateHierarchy is commited to the past array.

I am wondering if there is a way to solve this? So When I use clearHistory() then the _latestUnfiltered would not become null value, but it would become synced with the present array? Like when I receiveHierarchy action was dispatched and _latestUnfiltered got updated. Or could I sync the _latestUnfiltered manually? I tried to find a clearHistoryType, but I was not succeeded.

I am sorry for the trouble. But I think It is a real life siutation, when you get the data from the server. And then You would like to able to undo/redo modifications, cancel the changes and clear the history.

Thank you very much!

nmay231 commented 4 years ago

If you need to populate _latestUnfiltered after clearing the history, I would dispatch an action SYNC_FILTER_HIERARCHY that is also included in the filter.

filter: includeAction([updateHierarchy, 'SYNC_FILTER_HIERARCHY'])

While your reducer doesn't change, you just dispatch the extra action after each clearHistory() to get to the state you want.

If you need access to the built-in action types, you can import it.

import { ActionTypes } from 'redux-undo'

ActionTypes.CLEAR_HISTORY

Since this has become more of a support question, I'm going to close this. You can mention me (@nmay231) if you are still having problems.

itsmetambui commented 4 years ago

@nmay231, thanks for the wonderful package, I came across the same issue, and First, try setting ignoreInitialState and syncFilter to true. That might solve your issue. didn't work for me.

Here's a screencast of the behavior: https://gyazo.com/9b40ebecc93b70cf954963fae95ac174 The past state is behind 1 step with the present.

I'm not sure why this is the default behavior?

nmay231 commented 4 years ago

@itsmetambui Thank you for the compliment, but I am just a secondary maintainer! @omnidan and others put in the work to bring it to life over the years :wink:

But since you are not fetching data, you do not need ignoreInitialState. Try it with just syncFilter.