New ActionDispatched event would duplicate StateChanged

[cmeeren]

I'm working on a quick PR to add an ActionDispatched event to IStore and Store. I figured this was orthogonal to everything else on the store, and the intention was for this event to work as a backing for an extension method providing an IObservable of actions, as has been mentioned a few times (e.g. #47 (comment)).

While making this, it occurred to me that this event would be raised at exactly the same time as StateChanged. The only and critical difference is that ActionDispatched would pass on the action that was received (which is what allows us to build an IObservable out of it).

@GuillaumeSalles, @dcolthorp, @lilasquared, how do you think we should solve this? In order to be able to make an observable of actions that reach the reducer (not actions that are stopped by middleware), we either need to make this an event on the base Store class, or make a subclass of Store which overrides InnerDispatch (which needs to be made virtual as per PR #60).

I'd like the former, because then we could use an extension method to observe the actions, and wouldn't need a subclass (which could be troublesome if we want to implement other stuff that needs subclasses but are orthogonal to this). For example, the ObservableActionStore in PR #62 could be dropped entirely.

[dcolthorp]

It seems reasonable for ActionDispatched to be on the core Store to me. Honestly, I think of it as, if anything, even more fundamental than the StateChanged event. State updates and publication of StateChanged could in principle be just a default handler for ActionDispatched. You could also consider decoupling the two at a lower level, allowing e.g. the implementation of alternative state management models – such as the struct-based approach I outline in my blog post and example gist.

I'm not necessarily advocating for the latter, but that it could afford such flexibility recommends including it at a foundational level IMO.

[cmeeren]

I agree that ActionDispatched is more fundamental than StateChanged. The state, after all, changes in response to an action dispatched to the reducers.

It is also true that using ActionDispatched would allow for other state management models. However, considering that core idea of Redux is that 1) the state is encapsulated by the store and 2) the only way to change the state is to dispatch actions to the store, then IMHO we should make that particular design easy to use and the default choice.

There are a few "simple" alternatives:

1. Use the existing StateChanged but define it as Action<object> instead of Action and pass the action to the handlers. The semantics of this is "The state was possibly changed, in response to this particular action". The downside is that all state changed handlers are forced to accept a parameter most of them won't ever use, and which semantically isn't really needed in a StateChanged event.

2. Only have an ActionDispatched event. This makes it more semantically correct to pass the action, but the problem still remains that most handlers (that are only interested in updating stuff when the state changes) get a parameter they'll never need.

3. Have both StateChanged and ActionDispatched, with the downside being that the store is less orthogonal.

4. Don't implement ActionDispatched at all. No lost orthogonality and no unneeded event handler parameters, but we'd need a subclass or wrapper/decorator to implement ActionDispatched, which has the problems described at the top of this thread.

Of these, I'm leaning towards 2 or 3. If you or anyone else has other concrete suggestions, shoot!

State updates and publication of StateChanged could in principle be just a default handler for ActionDispatched.

You'd still have to define StateChanged on the store for this to work, right? See point 3 regarding the lost orthogonality...

[dcolthorp]

I'd vote for #3. It adds real power to the system, but its use is rather specialized – e.g. for extending the store.

Your point about orthogonality is well taken, but I think there are significant conceptual differences. ActionDispatch is for tapping into the event stream, StateChanged is for reacting to changes in the state. Sure, there's a causal relationship between them, but they're two different phases which should be kept distinct – keeping them that way ensures use of the store is consistent with the underlying paradigm.

[cmeeren]

I agree that having both StateChanged and ActionDispatched seems the best solution – the former for reacting to state changes, and the latter as a powerful way of extending the store. @GuillaumeSalles, do you agree? I can have a PR up pretty quickly, it's almost done.

[lilasquared]

I don't understand what this does that a middleware couldn't do. Middleware receive all actions and have the ability to do something with them before they reach the store. Middleware and stew creators are used to extend the store so I'm not sure I'm understanding the value of more events on the store itself

[lilasquared]

Store creators* damn mobile

[cmeeren]

ActionDispatched is a fairly low level concept that makes the store more extensible. It serves as the basis for an observable of actions that allows registering sagas using Rx, see #62 for details (particularly the unit tests for the registration syntax). Using Rx to subscribe sagas is more powerful than using any old middleware, as it's trivial to support throttling, strong typing of actions (no need to check in the sagas), etc. Furthermore, this way of subscribing sagas also makes it easier to have a new kind of store keep track of ongoing async operations, so they can be awaited when doing integration tests (again, see #62). AFAIK this would be much more cumbersome using a middleware. @dcolthorp may have some insights regarding this, and other salient comments on why this event is better than middleware and allows more features.

[cmeeren]

It's important to note though that this does not replace middleware. This is for doing stuff with actions that have already reached the store and been dispatched. This is not possible with middleware. While middleware can stop actions, which is orthogonal to what this event is for.

[powerdude]

I'm a new commenter, but i think you should just have StateChanged, but go back to the standard way of using EventHandler for the event. You will get the best of both worlds and T could be an object that would include the action. There's no reason to raise two events and you should follow the standard .NET event pattern.

[cmeeren]

It's true that we could pass some kind of StateChangedEventArgs that could include the action, but that's really then just ActionChanged under another name.

you should follow the standard .NET event pattern.

I disagree. The dispatched action shouldn't be needed for most purposes on a state-changed event in a redux architecture (it's mainly for more easily extending the store), so including the action could cause confusion among users and at worst lead to poor practices where it's used in unwise manners just because it's there. Even the updated state has been removed from the StateChanged event (PR #58) as per some discussions in e.g. #51 where you'll find a to a lenghty redux.js discussion on that topic (in short, the state should be retrieved directly from the store; sending the state in the event is slightly more convenient, but duplicates functionality and makes it harder to extend the store). StateChanged is now simply an Action.

In general, if we send objects in the event args that the handler shouldn't use, then at best, it just increases noise as it requires devs to accept parameters they'll never use, and at worst, it drives devs towards unfortunate design decisions.

In short, an ActionDispatched event as described is an easy way making the store extensible without cluttering the rest of the API (no-one has to use it, after all).

Of course, if you then prefer to program against an API that raises a StateChanged with StateChangedEventArgs, then it's trivial to write your own store that wraps the Redux.NET store and exposes whatever API you choose. I think the Redux.NET store should both be easily extensible and be elegant and promote best practices when used directly.

[cmeeren]

Created a PR for this at #64 so we have some code to look at.