fix #190: make withUndoRedo sync with watchState instead of effect

[richardliebmann]

This pull request fixes bug #190 and causes the undo/redo feature to synchronise instead of operating asynchronously.

[rainerhahnekamp]

@richardliebmann I have a problem in understanding the way how clearStack works or should work. I would except that it clears the stack, but as soon as I update my state, I should be able to undo.

@manfredsteyer & @GregOnNet, you both worked on this. Maybe you could help here.

Here's an example of a failing test:

it('should define clearStack behavior', () => {
  const Store = signalStore(
    { providedIn: 'root' },
    withState({ number: 0 }),
    withMethods((store) => ({
      updateNumber: (newNumber: number) =>
        patchState(store, { number: newNumber }),
    })),
    withUndoRedo({ keys: ['number'] }),
  );

  const store = TestBed.inject(Store);

  store.updateNumber(1);
  expect(store.number()).toBe(1);
  expect(store.canUndo()).toBe(true);

  store.undo();
  expect(store.number()).toBe(0);
  expect(store.canUndo()).toBe(false);

  store.updateNumber(10);
  store.clearStack();

  expect(store.canUndo()).toBe(false);
  store.updateNumber(11);
  expect(store.number()).toBe(11);

  /**
   * The following assertions fails, although it should be possible
   * to undo the last change to 10.
   */
  expect(store.canUndo()).toBe(true);
});

[jdegand]

@rainerhahnekamp You can add something like this:

withMethods((store) => ({
  undo(): void {
    const item = undoStack.pop();

    if (item && previous) {
      redoStack.push(previous);
    }

    if (item) {
      skipOnce = true;
      patchState(store, item);
      previous = item;
    }

    updateInternal();
  },
  redo(): void {
    const item = redoStack.pop();

    if (item && previous) {
      undoStack.push(previous);
    }

    if (item) {
      skipOnce = true;
      patchState(store, item);
      previous = item;
    }

    updateInternal();
  },
  clearStack(): void {
    undoStack.splice(0);
    redoStack.splice(0);
    previous = null;
    updateInternal();
  },
  softClearStack(): void {
    undoStack.splice(0);
    redoStack.splice(0);

    // Capture current state as new baseline
    const cand = keys.reduce((acc, key) => {
      const s = (store as Record<string | keyof Input['state'], unknown>)[key];
      if (s && isSignal(s)) {
        acc[key] = s();
      }
      return acc;
    }, {} as StackItem);

    previous = cand;
    updateInternal();
  },
}))

to match the soft clear functionality you are testing for. Or you can make it soft clear only.

[GregOnNet]

Hi @rainerhahnekamp,

the test you brought up is worth discussing it.
However, it is not related to this PR but to #186.

The behaviour of clearStack is debatable.
Maybe another issue is a better place for it.

For my use case, clearStack is like starting all over again (initial state).
Therefore, we need to forget about the complete history.

Here is a table that describes the flow of your test, Rainer.

state	previousValue	operation	canUndo	comment
0	null	initial value	false
1	0	updateNumber	true
0	null	undo	false
10	0	updateNumber	true
10	null	clearStack	false	this is like enforcing the initial value
11	null	updateNumber	false	behaves like initial value
12	11	updateNumber	true	the next operation can be undone

Use Case

Imagine you have a form-editor that can load and edit different forms.
You can navigate between those forms.
You want to undo/redo changes to several forms.
Once you navigate to another form, you need to call clearStack and start all over with the next form.
We need to ensure that nothing from the previous form is present in the undo/redo-stack.
Otherwise, we would undo a change in the new form and would get the previous form-state.

[GregOnNet]

This change is great and allows us to make all tests in with-und-redo.spec.ts synchronous.

Can we omit all fakeAsync- and tick-usages there?

[richardliebmann]

Hi @GregOnNet and @rainerhahnekamp,

I have removed the asynchronous features from the test, including 'fakeAsync' and 'tick'.

In my opinion, @GregOnNet is right. We also use the clearStack feature to reset to the initial value. In our case, we call clearStack after selecting or loading a new file in an editor and don't want to go back to the previous file. I currently cannot imagine a requirement where softClearStack would make sense, but it might be a useful extension.

[rainerhahnekamp]

@GregOnNet, @richardliebmann, sorry but I still don't get it.

The core issue is that during initialization the stack always gets the initial state but not on clearStack().

As a result, the first change after clearing (e.g. setting value to 11) is treated as the new initial state, and there’s nothing to undo until the second change.

Using your editors analogy: If you move from one file to another, the second file also has an initial state, and that’s exactly what the stack should adopt as its baseline.

Pseudo code for Richard’s case:

store.selectNewFile('graz.jpg');

// store's state is cleared and becomes the initial state for Graz
store.clearStack(); // Graz file's state is now the baseline

store.setTitle('Wien');

store.undo(); // restores title back to Graz

---

@jdegand This is not about the actual watchState or other implementation details - it’s purely about defining the intended behavior of the feature.

[GregOnNet]

Hi @rainerhahnekamp,

@richardliebmann please correct me, if I am wrong.
From my understanding, the order of execution was another than expected.
The reason is, that undo/redo ran asynchronous, due to the use of effect.

If you write…

patchState(...);
store.clearStack();

… it is possible that clearStack, was executed before the change was added to the undo-Stack. Although we expect canUndo to be false in this case, it was true.

[rainerhahnekamp]

@GregOnNet Yes, this is addressing an issue unrelated to Richard’s original intention. I came across it while writing a test for this particular change.

Should we merge this as is and open a separate issue for the new problem?

[GregOnNet]

Yes, this PR can be merged.

Concerning the new problem, we might want to jump on a short call.
I am a bit confused what the new issue is.
Maybe the description in the new issue makes things clearer to me. :-)