Adding ReduxStoreWithHistory to allow undo/redo operations.#9
Conversation
| { | ||
| State = state; | ||
|
|
||
| if (action != null) |
There was a problem hiding this comment.
This is breaking change compared to the last version. Let's create an issue to discuss how to handle null action and remove the check on this PR.
There was a problem hiding this comment.
The original intent of this check was to allow the overloaded method to pass a null in here, but you have a good point about breaking change. I'll revisit this.
There was a problem hiding this comment.
Well. Let's see if you still need to do it on your next iteration.
| public abstract class ReduxStoreWithHistory<TState> : ReduxStore<TState> | ||
| where TState : class, new() | ||
| { | ||
| private readonly Stack<TState> _previousStates = new Stack<TState>(); |
There was a problem hiding this comment.
Would it be better to stack Action instead of State? If you have an initial state + the list of actions, you can recreate any State at any time.
And the main advantage is that an Action is generally lighter than a State.
There was a problem hiding this comment.
Good point, I will refactor this.
There was a problem hiding this comment.
Was thinking about it for the last 10 minutes. I think the best solution is still to only save a list of Action.
But if you have executed a lot of actions and you want to go back in history, you will call the Reduce function too much. A smart way to handle that is to store a State at regular interval (of actions) in an attempt to optimize the move in history. We can plan that for a later version since it is simply an optimization and not a full feature.
There was a problem hiding this comment.
I'm thinking if we saved the action and the previous state at each step, then we can always recreate any state. Will try it out.
| where TState : class, new() | ||
| { | ||
| private readonly Stack<TState> _previousStates = new Stack<TState>(); | ||
| private readonly Stack<TState> _nextStates = new Stack<TState>(); |
There was a problem hiding this comment.
Same. But we can save both forward Actions and UndoneActions, or a better name if you have.
|
|
||
| _nextStates.Push(State); | ||
|
|
||
| GoToState(_previousStates.Pop()); |
There was a problem hiding this comment.
We need to know what devs think will happen when using the Undo method.
Should we trigger a new OnNext event for the State and the Action? Does it make it sense because to me it will like we continue the flow of actions forward and not going backward? Should we introduce new Subject<Action> per Undo call so devs will handle the change by themselves?
There was a problem hiding this comment.
I was thinking that triggering a new event for Action would be kind of awkward because we're not technically performing the action again when we undo. Your idea for having a separate Subject<Action> for Undo calls makes sense though.
| return _actionSubject.OfType<T>().AsObservable(); | ||
| } | ||
|
|
||
| private void GoToState(TState state, object action) |
There was a problem hiding this comment.
Not sure we need to extract the method. Based on my comments, the code should normally go back to Dispatch method.
|
Wow. Thank you for taking the lead on what I can call the largest PR of the history of this project. :) Of course, there will be iteration on it but for now, it's a great start. Already made some comments we can discuss. |
|
Made some changes based on your comments. I had to keep a version of the Another difference is that previously |
| return _actionSubject.OfType<T>().AsObservable(); | ||
| } | ||
|
|
||
| protected void GoToState(TState state) |
There was a problem hiding this comment.
I suppose we can now rename the method UpdateState.
| private readonly Stack<object> _futureActions = new Stack<object>(); | ||
|
|
||
| public bool CanUndo => _pastMementos.Count != 0; | ||
|
|
| private class ReduxStoreMemento | ||
| { | ||
| public TState State { get; } | ||
|
|
| State = Reduce(State, action); | ||
|
|
||
| GoToState(Reduce(State, action)); | ||
|
About the order of Subject publication, I don't think the order matters. What is important is that they should be published at the same time, one after another, and that is still the case. |
2 participants
We can iterate over the design a bit, but this will accomplish what #1 asks for.