fix(undo-redo): clearStack should also clear the previous item

[GregOnNet]

solves #185

[GregOnNet]

Sorry for the huge Diff.
I think the file was not formatted before, but due to lintstaged, prettier did its job. ;-)

[GregOnNet]

withUndoRedo is used as plugin inside a store.
When the undo & redo stack shall be cleared clearStack is called.

Currently, it is not clear which stack is cleared.
In fact, we are clearing two stacks.

Do we want to deprecate clearStack in favor of resetUndoRedo or clearUndoRedo?
I guess reset* fits better, @rainerhahnekamp .

[rainerhahnekamp]

Hey @GregOnNet, regarding the reset functions: there’s an open discussion about taking a different approach. The idea is to avoid adding more methods directly to the signalStore, in order to prevent “polluting” the API with keywords that might be needed for other use cases in the future:
#160 (review)

So yes, I’m in favour of deprecating clearStack — but instead of introducing new store methods, we should provide standalone functions that can be used within patchState.

[GregOnNet]

When I understand correctly clearing the undo-redo-stack could become something like this.

patchState(store, state => reset({ undoStack: [], redoStack: []}));

// or if the internal state gets adjusted

patchState(store, state => reset({ undoRedo: ??? }));

// or with some type-literal wizardry

patchState(store, state => reset('undoRedo'));

Am I correct?

I also experience challenges managing the visibility of certain mechanics in the store.
It feels like there is "internal state" and "public state".
In consequence there are methods internal to the store and public ones.

Nevertheless, I do not think that the example clearStack is "polluting" the API.
To me, it is the public API of the signalStoreFeature.
So I absolutely expect this method to be in my store to handle my use-cases.
Adding reset, causes an indirection, a developer might need to read about.
To me, it is not intuitive.
If I understood correctly, instead of "learning" about "clearStack" the API-user now needs to have knowledge about the shape of the state of the undo-redo-plugin.

Maybe it is worth to push the discussion about visibility to @ngrx/signal-store instead to reason about an overall solution for this topic.

UPDATE

What comes to my mind simplifying reset a bit.
We could provide an abstraction to hide the details of which parts of the state are reset.
I guess, undoStack & redoStack still need to get part of the state, then:

import { resetUndoRedo } from '@angular-architects/ngrx-toolkit'

patchState(store, state => resetUndoRedo(state)) 

// or shorthand


patchState(store, resetUndoRedo) 

// or making resetUndoRedo call patchState

resetUndoRedo(store) // But now, another indirection. The relation to patchState and how it is operating with the store is hidden.

[rainerhahnekamp]

Sorry for the huge Diff.

Do you think you can manage to do it without the reformatting?

I think it is better for the Git history (but also review), if we only see the real changes.

We can always do a big formatting PR afterwards, but that would then be separated from that one.

[GregOnNet]

Hi Rainer,

yes I can.
Nevertheless, we might want to reformat the whole project once.
I believe others who are willing to contribute encounter the same problem since formattig is done automatically while committing.

Currently, I guess, everyone has more effort to contribute if an unformatted file is touched.

[GregOnNet]

I pushed the changes withoud reformatting @rainerhahnekamp .

[rainerhahnekamp]

@GregOnNet, thanks, and a quick update from my side. I am currently on vacation and will return on Wednesday. You will hear from me then.

[GregOnNet]

Thanks for the update, Rainer.
I wish you a pleasant and relaxing trip!