search

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

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

Closed GuillaumeSalles closed 7 years ago

GuillaumeSalles commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

@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 commented 7 years ago

@GuillaumeSalles based on the discussions in https://github.com/GuillaumeSalles/redux.NET/issues/51 do you think we should create a interface store class that exposes the events / observable that decorates the IStore?

cmeeren commented 7 years ago

And now I went ahead and pulled the whole event thing into doubt again in #https://github.com/GuillaumeSalles/redux.NET/issues/51#issuecomment-290417443... :-)

cmeeren commented 7 years ago

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 commented 7 years ago

@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