Handle IStore subscription like redux.js

[GuillaumeSalles]

#57 should be close before this one

Thanks again @lilasquared and @cmeeren to have insisted. The API is now much cleaner.

[cmeeren]

Wait, I don't get it:

Wasn't the intention to use a standard event (StateChanged) and then have extension methods for an Rx subscribe method? Like you described in #51 (comment), point 2 (which you refer to in #57). (The alternative to a standard event is just using Rx as the only way to subscribe.)

This PR seems like it's neither – no event, and a Subscribe() method that's not Rx. If that's correct, then I don't think that's a good idea, since it's non-standard (as opposed to events or Rx). See #51 (comment) for details, specifically:

• Use a simple StateChanged event on the store for subscribing to state changes. Events are very common in C# and should be well known to most .NET devs. This has benefits in edge cases e.g. for multithreading/concurrency and exceptions, where the events and handlers will behave as expected to anyone familiar with events (e.g. throwing an exception from a handler will make the subsequent handlers not get called. etc.). With a custom Subscribe() implementation, these things would be more up in the air and require more investment on the part of the developer to read documentation, not to mention put more work on us to be thorough in explaining how the code behaves in all edge cases (an impossible task since we can't foresee them all, and bound to result in bug reports).

Note that the above quote was written under the misunderstanding that the 2.0.0 Subscribe method was non-standard, which I later realized was false (it's an implementation of IObservable). So the IObservable.Subscribe method is technically fine.

[cmeeren]

To be clear: Personally, I would be satisfied with Rx only, but I think we should do either a standard event (which calls delegates on subscription) with Rx as extension methods or similar, or Rx only.

[lilasquared]

I think there are reasons that redux behaves the way it does for predictability and extensions and such. It has specific tests for ensuring that subscribers have their callbacks caled at the right time and not during certain execution cycles. You can check out the redux.js source code and unit tests to see it. It should be pretty simple to have a decorator or store enhancer that exposes the events if we want to do that but since they would use the underlying implementation of subscribe and getstste, then the behavior would still adhere to the current redux spec.

[cmeeren]

@lilasquared Could you be specific as to what would be unsatisfactory with events or IObservable.Subscribe, that is fixed when using the Subscribe method from this PR?

[lilasquared]

@cmeeren nothing that's fixed with this PR because of the todo comment. I think the test from redux.js though deals with subscribe and unsubscribe. The tests are here I can't link to the lines cause I'm on mobile but a few of the subscribe and unsubscribes don't pass unless you manage the listeners internally like redux.js does. Whether it's satisfactory or not, I don't know enough about all of the extensions to say. I am more just bringing it up because of the current redux.js spec.

[lilasquared]

I pulled this down and it won't build here - @GuillaumeSalles does it build for you?

[GuillaumeSalles]

Oh... I forgot to save a file before commit.

[lilasquared]

I wonder why the build didn't fail for the CI

[GuillaumeSalles]

True. It's weird. I'll a take a look later.

[GuillaumeSalles]

When I was working on this PR, it felt weird to have an event without parameter. But honestly, I don't have any preferences between Action Subscribe(Action) and event Action StateChanged. Maybe it's because I do more javascript than C# recently. If you feel that StateChanged event is better, I can correct it right away.

However, IMHO IObservable.Subscribe is not correct (even if IObservable is an interface in System) for those reasons :

• It take an IObserver as parameter and OnError and OnCompleted will never be fired in our case.
• It force us to do : public interface IStore<T> : IObservable<T>, hence we need to reimplement IObservable (not recommended by introtorx.com.
• Since it's less orthogonal, it force us to do weird things when we lift the state in a StoreEnhancer (See TimeMachineStore)

[cmeeren]

@lilasquared So what we need to do is to decide how we want Redux.NET to work, which may not be exactly the same as redux.js in all cases. This may be purely design decisions on our part, or it may be differences due to the fact that C# is a very different language than JS with different syntax, features, and conventions.

I've skimmed through the redux.js tests you linked to, and I don't immediately see any tests concerning subscribe/unsubscribe that are 1) represents a good idea in .NET, and 2) not easily attainable using either plain events or Rx. In fact, several of the tests we don't need because we get them for free with events/Rx, such as supports multiple subscriptions, only removes listener once when unsubscribe is called, or only removes relevant listener when unsubscribe is called.

---

There are a few features that I'm not sure really is a good idea (please let me know if/how I'm wrong). When reading the below, keep in mind that like all good libraries, we should strive to push our users towards sensible design decisions.

Specifically, it concerns the delays unsubscribe until the end of current dispatch and delays subscribe until the end of current dispatch features (which I guess is the features you talk about that require managing subscriptions manually): AFAICS this is relevant if some subscribers subscribe/unsubscribe other handlers, and ensures that all handlers will get called.

In a .NET sense, one is never guaranteed the order in which standard event handlers will get called. Thus, unsubscribing handler A from handler B would be asking for trouble if you really need to be sure that handler A also runs for the same dispatch where you unsubscribe it from B. AFAIK these "asking for trouble, don't do it" semantics are the ones .NET devs are used to, and it seems to me that having this as a feature would be just to cater to some unwise design decisions on the part of our users. (Of course, please let me know if you have a solid use case for this that can't be catered to in a more robust manner.)

The point is, .NET has well-defined semantics for subscribing and unsubscribing event handlers (and IObservable listeners for those familiar with Rx). This is not directly transferable to/from JS.

---

Again, I don't think it should be a goal in and of itself to make Redux.NET work 100% the same as redux.js. I think we should strive first and foremost to make the redux architecture easy to use in .NET. (Though of course, we shouldn't create significant differences without good reason.)

[cmeeren]

@GuillaumeSalles

If you feel that StateChanged event is better, I can correct it right away.

I certainly do at the moment, though this may be due to limitations of my knowledge. @lilasquared may have valid rebuttals to my previous comment.

However, IMHO IObservable.Subscribe is not correct (even if IObservable is an interface in System) for those reasons :

• It take an IObserver as parameter and OnError and OnCompleted will never be fired in our case.

When using Rx, the Subscribe method has plenty of overloads. I hadn't really thought about it, but if not using Rx means you will have to implement IObservable for all subscribers, then yes, that won't fly. (Unless we decide to force users to depend on Rx - then it's just as easy as adding an event handler.)

But yes, I agree that in the "base" store a StateChanged event is fine, and that Rx can be implemented using extension methods.

[lilasquared]

@cmeeren It would take me some time to get a good example of something that might not work with events. In the meantime I looked at the store api docs on redux.js:

You may call dispatch() from a change listener, with the following caveats:

1. The listener should only call dispatch() either in response to user actions or under specific conditions (e. g. dispatching an action when the store has a specific field). Calling dispatch() without any conditions is technically possible, however it leads to an infinite loop as every dispatch() call usually triggers the listener again.
2. The subscriptions are snapshotted just before every dispatch() call. If you subscribe or unsubscribe while the listeners are being invoked, this will not have any effect on the dispatch() that is currently in progress. However, the next dispatch() call, whether nested or not, will use a more recent snapshot of the subscription list.
3. The listener should not expect to see all state changes, as the state might have been updated multiple times during a nested dispatch() before the listener is called. It is, however, guaranteed that all subscribers registered before the dispatch() started will be called with the latest state by the time it exits.

It is a low-level API. Most likely, instead of using it directly, you'll use React (or other) bindings. If you commonly use the callback as a hook to react to state changes, you might want to write a custom observeStore utility. The Store is also an Observable, so you can subscribe to changes with libraries like RxJS.

if we decide to go with events, Do you see any issues with supporting nested dispatch as they do here?

[cmeeren]

1. The listener should only call dispatch() either in response to user actions or under specific conditions (e. g. dispatching an action when the store has a specific field). Calling dispatch() without any conditions is technically possible, however it leads to an infinite loop as every dispatch() call usually triggers the listener again.

There is no problem calling Store.Dispatch() from a StateChanged event handler (with the same warnings and limitations as regards infinite looping). However, this seems to be bad design to me, and I can't come up with any use cases which are not better solved with more robust methods.

2. The subscriptions are snapshotted just before every dispatch() call. If you subscribe or unsubscribe while the listeners are being invoked, this will not have any effect on the dispatch() that is currently in progress. However, the next dispatch() call, whether nested or not, will use a more recent snapshot of the subscription list.

This works out of the box, because multicast delegates (e.g. event handlers) are immutable, so any unsubscription will not affect the current invocation of the delegates. I wrote a gist that tests this and passes. I also wrote an SO question before I found out about the immutability of multicast delegates.

Anyhow, all the sentiments I can find online about the invocation order of .NET event handlers indicates that if you rely on a specific order, then it is an indication of a (possibly serious) design problem, and I would agree. It will also indicate a design problem even if we use a custom Subscribe() method, because this method is semantically equivalent to a normal event subscription.

3. The listener should not expect to see all state changes, as the state might have been updated multiple times during a nested dispatch() before the listener is called. It is, however, guaranteed that all subscribers registered before the dispatch() started will be called with the latest state by the time it exits.

This does not work out of the box, as this gist shows. Though again, this seems to assume some ordering of invoked handlers, which as mentioned seems to indicate design problems in the .NET world. If we really want to support it, I'm sure there's some way we can do that with a normal event. I think the logic would lie in the InnerDispatch method. But again, we should think very carefully if this is something we want to support, and therefore also something we implicitly communicate to our users that "this is a perfectly valid way to do it."

[GuillaumeSalles]

Thanks @cmeeren and @lilasquared for your feedback/analysis.

Store now use event Action StateChanged.

@lilasquared Regarding nested dispatch, every cases are handle in this PR.
@cmeeren Supporting the 3rd case is a consequence of removing the parameter of the StateChanged event.

If I update your gist like this, everything work.

    public class EventSource
    {
        private int i;

        public int GetState()
        {
            return i;
        }

        public event Action Event;

        public void TriggerEvent()
        {
            i++;
            this.Event?.Invoke();
        }
    }

    public class EventSourceTests
    {
        [Test]
        public void When_InvokingFromHandler_Should_InvokeAllWithLatestState()
        {
            // Arrange
            var listener1InvokedState = 0;
            var listener2InvokedState = 0;
            var listener3InvokedState = 0;

            var sut = new EventSource();

            Action listener1 = null;
            Action listener2 = null;
            Action listener3 = null;

            listener1 = () => listener1InvokedState = sut.GetState();
            listener2 = () =>
            {
                listener2InvokedState = sut.GetState();
                if (sut.GetState() == 1) sut.TriggerEvent();
            };

            listener3 = () => listener3InvokedState = sut.GetState();

            sut.Event += listener1;
            sut.Event += listener2;
            sut.Event += listener3;

            // Act
            sut.TriggerEvent();

            // Assert
            Assert.That(listener1InvokedState, Is.EqualTo(2));
            Assert.That(listener2InvokedState, Is.EqualTo(2));
            Assert.That(listener3InvokedState, Is.EqualTo(2));
        }
    }

I ignore possible multi-thread issues right now to focus on the shape of the API and the internal behavior.

This behavior will be removed in another PR. Help is welcome :)

I think that every concerns is resolved so I merge this PR.

Feel free to comment if I missed something.

[cmeeren]

Supporting the 3rd case is a consequence of removing the parameter of the StateChanged event.

Yes, of course, that would work. Then AFAICS listener3 will be called twice and fetch state = 2 both times, and this will make the test pass.

The PR looks good to me.

[cmeeren]

This behavior will be removed in another PR. Help is welcome :)

What behavior?