Cysharp / R3

The new future of dotnet/reactive and UniRx.
MIT License
2.32k stars 103 forks source link

Add `Reactiveproperty<T>.Subscribe(Observer<T> observer, bool riseOnSubscription = true)` overload #215

Open Antoshidza opened 6 months ago

Antoshidza commented 6 months ago

For new devs like me it is often confusing when callback rises immediately after subscription even there is now changes. It is even more confusing because when you use PairWise before Subscribe it will not rise callback right after subscription.

There is code in ReactiveProperty.cs which calls OnNext() without any condition.

// raise latest value on subscribe(before add observer to list)
observer.OnNext(currentValue);

There is some advices to use Skip(1) for such situations but again it may lead to unexpected behavior when PairWise used, so actual value change will be ignored 1 time in this case.

thejayman commented 5 months ago

It would be beneficial to add an argument to the ReactiveProperty constructor to determine if it should notify on subscribe. This would loosely resemble the behavior of the "ReactiveProperty" library at https://github.com/runceel/ReactiveProperty/tree/main.

Antoshidza commented 5 months ago

It would be beneficial to add an argument to the ReactiveProperty constructor to determine if it should notify on subscribe. This would loosely resemble the behavior of the "ReactiveProperty" library at https://github.com/runceel/ReactiveProperty/tree/main.

Is there any design reasons to put this inside constructor?

Imagine that you have some class which expose it's ReactiveProperty<int> as IReadOnlyReactiveProperty<int>. And you have 2 subscription points in your app. First point is in simulation layer and it wants to execute some code only when IReadOnlyReactiveProperty<int> actually changes and not execute code immediately. Second point is in presentation layer and it wants to display value of IReadOnlyReactiveProperty<int> on screen, so it actually wants react on changes but it is handy to also execute displaying code immediately with current value without accessing through IReadOnlyReactiveProperty<int>.CurrentValue.

Maybe it is more convenient when owner decides how owned Observable should behave through constructor. But it forces subscription points to take care of avoiding initial callback, because if ReactiveProperty<T> exposed as Observable<T> you can't tell would it callback immediately or not. Actually same with property exposed with IReadOnlyReactiveProperty<T> having constructor parameter. Today if I see IReadOnlyReactiveProperty<T> I'm sure I will get invoked on subscribe but with parameter in constructor now I can't guarantee anything and if it is crucial for my code to react only on changes I should now somehow detect if execution happens right after subscription or something which is opposite of what I want to have in R3.

thejayman commented 5 months ago

I get where you're coming from about losing the guarantee of a signal on subscription. I suggested it as an option, hoping that it could also be considered to help achieve the outcome you were after. However, it seems like that's not what you're looking for.

Antoshidza commented 5 months ago

I get where you're coming from about losing the guarantee of a signal on subscription. I suggested it as an option, hoping that it could also be considered to help achieve the outcome you were after. However, it seems like that's not what you're looking for.

I think that making overload have cons such as Subscribe method is an extension for Observable<T> AFAIK, so it is only possible to add Subscribe overload only for IReadonlyReactiveProperty<T>, and even with that method overload cases where property exposed with Observable<T> still not guarantee concrete subscribe behavior.

At the same time adding constructor parameter at least guarantee subscribe behaviour for owner. But the main problem remains: subscribers still must "know" what they subscribe on.

Last8Exile commented 2 months ago

Maybe it should not raise callback during subcribtion no matter how convenient it may be?

Antoshidza commented 2 months ago

Maybe it should not raise callback during subcribtion no matter how convenient it may be?

Agree, it is better to introduce extension method that will invoke callback with current value (if there is any)