Open cmeeren opened 7 years ago
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.
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:
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.
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.
Have both StateChanged
and ActionDispatched
, with the downside being that the store is less orthogonal.
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 forActionDispatched
.
You'd still have to define StateChanged
on the store for this to work, right? See point 3 regarding the lost orthogonality...
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.
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.
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
Store creators* damn mobile
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.
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.
I'm a new commenter, but i think you should just have StateChanged, but go back to the standard way of using EventHandler
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.
Created a PR for this at #64 so we have some code to look at.
I'm working on a quick PR to add an
ActionDispatched
event toIStore
andStore
. 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. https://github.com/GuillaumeSalles/redux.NET/issues/47#issuecomment-290694783).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 thatActionDispatched
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 ofStore
which overridesInnerDispatch
(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.