Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ObserveState extension #57

Merged
merged 1 commit into from
Apr 2, 2017
Merged

ObserveState extension #57

merged 1 commit into from
Apr 2, 2017

Conversation

GuillaumeSalles
Copy link
Owner

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
Copy link

@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
Copy link
Owner Author

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
Copy link

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
Copy link
Owner Author

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
Copy link

cmeeren commented Apr 1, 2017

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
Copy link
Owner Author

@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
Copy link

cmeeren commented Apr 1, 2017

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
Copy link
Owner Author

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

@GuillaumeSalles GuillaumeSalles merged commit 419db1d into master Apr 2, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants