GuillaumeSalles / redux.NET

Redux.NET is a predictable state container for .NET apps. Inspired by https://github.com/reactjs/redux.
712 stars 86 forks source link

ObserveState extension #57

Closed GuillaumeSalles closed 7 years ago

GuillaumeSalles commented 7 years ago

I propose the solution 2 explained in this comment

I think it's the cleaner approach.

@cmeeren, you quoted this earlier :

The Create method is also preferred over creating custom types that implement the IObservable interface. There really is no need to implement the observer/observable interfaces yourself. Rx tackles the intricacies that you may not think of such as thread safety of notifications and subscriptions.

I agree but for the solution 1, you don't have other choice if you want to have other method on your custom observable.

Also, Observable.FromEvent is okay for our use case (link)

(I read this website in 2011 and it's still the best Rx reference 👌 )

What do you think ?

lilasquared commented 7 years ago

@GuillaumeSalles I have been researching observables all day partly for use within redux extensions and partly for a project I am working on, but I am not that familiar with them yet. It looks like a very simple extension that attaches to the state changed event, however I am still questioning the validity of the state changed event vs just subscribing. If the event wasn't part of the store would this still be possible?

GuillaumeSalles commented 7 years ago

When you say : " event vs just subscribing", are you referring to C# event vs Observable or C# event vs Subscribe method like in redux.js ?

I'm not sure to understand this :

If the event wasn't part of the store would this still be possible?

lilasquared commented 7 years ago

Yeah what I meant was - If the store was implemented as redux.js, eg

store.Subscribe(() => doStuff())

could the observable extension still work? And If so, could we implement the store as redux.js and have both an event-based extension and a observable based extension so that store was as low level as redux.js

GuillaumeSalles commented 7 years ago

If the store was created exactly like redux.js, the observable could be build as an extension method (with Observable.Create instead of Observable.FromEvent), but the Event couldn't be built as an extension method. It would be possible as an adapter (Adapter pattern).

IMHO, C# event are as low level as redux.js subscribe method. StateChanged event and subscribe have exactly the same external properties. C# event are just more pratical because internally, you don't have to keep a list of subscribers. I'm confident that redux.js creators would have use a language construct like C# events if they had exist in javascript.

cmeeren commented 7 years ago

So if I understand correctly, one can get state updates by:

  1. Subscribing to the Store.StateChanged event, which passes the updated state. (Is it really a good idea to pass the state, considering the redux.js discussion mentioned in this comment?)

  2. Subscribing to the IObservable returned by Store.ObserveState(). Store.ObserveState().Subscribe() will after merging work exactly like the previous Store.Subscribe(), right?

Why have you called the MockObserver member for StateChangedHandler? Aren't you mixing events and Rx now?

Another note: I think we should consider implementing support for a kind of store only surfacing the state through Store.Project() and Store.Observe() methods described in other discussions here. I have refactored my app to work with a store wrapper (I've called it ProjectingStore) that does this (I can share it in a gist), and it forces you to into a nice separation of concerns and pushes you towards keeping your domain logic in your domain layer and keeping your state subscribers simple. I really like it. My point in this context being, we should keep that in mind in case it's relevant for this PR.

@dcolthorp, wanna take a look at this PR?

GuillaumeSalles commented 7 years ago

@lilasquared, @cmeeren : wow, I'm so confused. I don't know how could I have misread your comments for so long. I worked with redux.js for 2 years and the idea that subscribe listener takes the state as parameter was so deeply integrated in my mind that my brain somewhat obfuscated your arguments. My mistake. Thanks a lot to have insisted on this point. I will make another PR after this one to implement it without parameter (to finally be as orthogonal as the js version :)). Or maybe one of you want to take a try?

Subscribing to the IObservable returned by Store.ObserveState(). Store.ObserveState().Subscribe() will after merging work exactly like the previous Store.Subscribe(), right?

Yeah.

Another note: I think we should consider implementing support for a kind of store only surfacing the state through Store.Project() and Store.Observe() methods described in other discussions here. I have refactored my app to work with a store wrapper (I've called it ProjectingStore) that does this (I can share it in a gist), and it forces you to into a nice separation of concerns and pushes you towards keeping your domain logic in your domain layer and keeping your state subscribers simple. I really like it. My point in this context being, we should keep that in mind in case it's relevant for this PR.

My goal with v3 will be to enable opinionated extensions ,like your ProjectingStore or the @dcolthorp 's one, to be built on top of redux. With this architecture, you will still be able to use other open source Middlewares or the devtools for free. I'll try to make a PR soon to handle your extensions.

cmeeren commented 7 years ago

My goal with v3 will be to enable opinionated extensions ,like your ProjectingStore or the @dcolthorp 's one, to be built on top of redux. With this architecture, you will still be able to use other open source Middlewares or the devtools for free. I'll try to make a PR soon to handle your extensions.

Sure, sounds fine. We'll just have to make sure that the extensions are (as far as possible) orthogonal and compatible. :) I've lost track of the exact number of extensions we've talked about and their specifics.

GuillaumeSalles commented 7 years ago

Or maybe one of you want to take a try?

The PR was a little bit heavier than expected so I did it. Sorry if one of you worked on it... See #58

Why have you called the MockObserver member for StateChangedHandler? Aren't you mixing events and Rx now?

Just a bad refactoring after the Rx version. Fixed in #58