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

Breaking change: Store<T> use events instead of implementing IObservable<T>. #55

Closed
wants to merge 3 commits into from

Conversation

GuillaumeSalles
Copy link
Owner

Motivation : #51

Store is now event based. The code is now really closed to Yarni!

I created a ObservableStore that can wrap an IStore instance to expose it as observable. Not sure this breaking change is acceptable...

Ignore the time TimeMachine changes. It should be refactored to use store enhancer.

@GuillaumeSalles
Copy link
Owner Author

ObservableStore could be replace by this extensions method

public static IObservable<TState> ObserveState(this IStore<TState> store)
{
    return Observable.FromEvent<TState>(
        h => store.StateChanged += h,
        h => store.StateChanged -= h);
}

pros : Less library code, wrapper not needed to instanciate the store.
cons : More breaking changes.

@lilasquared
Copy link

@GuillaumeSalles I have some questions about this one because i've done the same for NRedux. I have more tests in mine that I copied right from redux test cases. Once i did this a bunch of them started failing. I'm wondering if it should still have the same behavior of redux even though it's dotnet or if the behavior we see with the events is acceptable. I can explain in detail and show examples or submit my on PR here that has the tests and that they are breaking. What do you think?

@lilasquared
Copy link

@GuillaumeSalles based on the discussions in #51 do you think we should create a interface store class that exposes the events / observable that decorates the IStore?

@cmeeren
Copy link

cmeeren commented Mar 30, 2017

And now I went ahead and pulled the whole event thing into doubt again in ##51 (comment)... :-)

@cmeeren
Copy link

cmeeren commented Mar 30, 2017

And as I said - I wouldn't worry about breaking changes, since it's a major upgrade. Let's make it as mature, solid, and consistent as we can.

@GuillaumeSalles
Copy link
Owner Author

@GuillaumeSalles I have some questions about this one because i've done the same for NRedux. I have more tests in mine that I copied right from redux test cases. Once i did this a bunch of them started failing. I'm wondering if it should still have the same behavior of redux even though it's dotnet or if the behavior we see with the events is acceptable. I can explain in detail and show examples or submit my on PR here that has the tests and that they are breaking. What do you think?

@lilasquared Sorry to have not answer sooner. You are totally right about the behaviors differences. I talk a little bit about it here. Did you see other differences ? PR are always welcome :)

@GuillaumeSalles based on the discussions in #51 do you think we should create a interface store class that exposes the events / observable that decorates the IStore?

And now I went ahead and pulled the whole event thing into doubt again in ##51 (comment)... :-)

I'm closing this PR.
I merged another PR that contains only the event based version : #56 (Feel free to add comments if you have an issue with the code.)
I opened another PR to talk specifically about the Store exposed as observable : #57

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