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

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

Closed
Antoshidza opened this issue May 29, 2024 · 9 comments

Comments

@Antoshidza
Copy link

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

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

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

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

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

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

@Antoshidza
Copy link
Author

Antoshidza commented Sep 24, 2024

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)

MarquisMc added a commit to MarquisMc/R3 that referenced this issue Oct 22, 2024

Verified

This commit was signed with the committer’s verified signature.
snyk-bot Snyk bot
Fixes Cysharp#215

Add `Subscribe(Observer<T> observer, bool riseOnSubscription = true)` overload to `ReactiveProperty<T>` class.

* Add a new `Subscribe` method overload with `riseOnSubscription` parameter to `ReactiveProperty<T>` class in `src/R3/ReactiveProperty.cs`.
* Update `SubscribeCore` method in `ReactiveProperty<T>` class to conditionally call `observer.OnNext(currentValue)` based on `riseOnSubscription` parameter.
* Add new `Subscribe` method overloads with `riseOnSubscription` parameter to `ObservableSubscribeExtensions` class in `src/R3/ObservableSubscribeExtensions.cs`.
* Add unit tests for the new `Subscribe` method overload with `riseOnSubscription` parameter in `ReactiveProperty<T>` class in `tests/R3.Tests/ReactivePropertyTest.cs`.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/Cysharp/R3/issues/215?shareId=XXXX-XXXX-XXXX-XXXX).
@neuecc neuecc closed this as completed Feb 14, 2025
@Antoshidza
Copy link
Author

PR closed because of AI generated code but why this issue is also closed?

@neuecc
Copy link
Member

neuecc commented Feb 18, 2025

Since Subscribe is also an interface method, I think it would be undesirable from a UX perspective to add a special overload here.

@Antoshidza
Copy link
Author

Antoshidza commented Feb 19, 2025

Ok, but what should we do in the next case:
We have IFoo interface that provides Observable<int> which could be both ReactiveProperty<int> or Subject<int>. The interface (IFoo) clearly tells us that we can only subscribe to "push" of a value. If I would need to give a possibility to subscribe AND read current value I'd rather expose ReadOnlyReactiveProperty<int>. In the end subscriber can't know what behavior actually will happen: just subscribe or subscribe and instant push of current value. I think it breaks L from SOLID, because I can't now just use any Observable<int> safely.

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 a pull request may close this issue.

4 participants