angular-architects / ngrx-toolkit

Various Extensions for the NgRx Signal Store
MIT License
104 stars 17 forks source link

Undo Redo only works on singleton stores #6

Open kobi2294 opened 6 months ago

kobi2294 commented 6 months ago

The implementation adds an ainternal state, by design, by adding local variables in the withUndoRedo function. The problem is that this function is called only once when the type of the store is defined. That means that if two different components will provide the same type of signal store, they will have 2 instances, sharing the same undo and redo stacks.

For this to work we need to be able to maintain private data per instance of the store. One approach might be to "provide" a service with the store itself. So one Request For Feature may be for a feature that allows for the signal store to define a derived injector and add providers to that injector.

rainerhahnekamp commented 6 months ago

Thanks for raising this issue Kobi. Do you want to contribute a PR?

kobi2294 commented 6 months ago

I am not sure I have a solution for this (in fact, I am trying to deal with the same problem in one of the custom features I am writing for my project). It requires some additional feature from the @ngrx team

rainerhahnekamp commented 6 months ago

OK, then let me take a look. The uncommitted version of DevTools deals already with multiple instances of the same store.

kobi2294 commented 6 months ago

Ok, I found a solution in my project. I think it can work also on the withUndoRedo feature. When you create a new feature in the following structure:

function withXXX(): SignalStoreFeature {
      const x = something;

      return signalStoreFeature(
            withThis(), 
            withThat()
      );
}

you need to remember that the function is only executed once per "store type definition". So if the same store type is provided many times, x is still declared only once. So all instances of the store "share" the same x. This can be good for static data, like id counters, but bad for state that should be unique per store instance.

However, you can write the function a bit differently. The function is supposed to return a SignalStoreFeature, and the signalStoreFeature() function creates one for you. But when you examine the type, you see that it is just a function that takes a store and creates another store. So as long as your method returns a store => store function, your ok.

So, you could write the function like this:

function withXXX(): SignalStoreFeature {
      return store => {
              const x = something;
              const feature = signalStoreFeature(
                    withThis(), 
                    withThat()
              );
            return feature(store);
      }
}

now x is declared in a function that is executed once per store instance (it even receives a partial store instance which our custom feature is supposed to enhance). We still create the feature using signalStoreFeature and then apply it on the store. But now x is local and recreated each time we instantiate this store.

rainerhahnekamp commented 6 months ago

All your methods starting with with*will get the store object of the current instance. I think that is better, because in your version above, store is not aware of the functions or properties that your other with methods add to it.

So the store in withThat would have access to the things that withThis has already added.

kobi2294 commented 6 months ago

Yap, you are right. Each with* method takes the store before it and replaces it with a new and enhanced store. That's the idea behind custom feature functions. Typical functional programming at it's best.