dotnet / reactive

The Reactive Extensions for .NET
http://reactivex.io
MIT License
6.74k stars 751 forks source link

BehaviorSubject implements IBehaviorObservable #1961

Closed Phoder1 closed 1 year ago

Phoder1 commented 1 year ago

What is the nature of your contribution?

Enhancement

IBehaviorObservable is used to expose the subject's Value property as readonly, in order to restrict public write access.

Phoder1 commented 1 year ago

@dotnet-policy-service agree

akarnokd commented 1 year ago

Please describe why you want to add this change.

Phoder1 commented 1 year ago

Please describe why you want to add this change.

Sorry, done.

akarnokd commented 1 year ago

Please describe the why, not the what. What's the reason you want to add that misspelled interface? Also where is this IReadOnlyBehavior? In addition, there are style requirements, such as the license header that is missing.

Phoder1 commented 1 year ago

Please describe the why, not the what. What's the reason you want to add that misspelled interface? Also where is this IReadOnlyBehavior? In addition, there are style requirements, such as the license header that is missing.

Sorry about the misspelling, I want to contribute, do whatever you want.

akarnokd commented 1 year ago

Your contribution makes no sense. The problem with fluent APIs such as Rx is that any specific type/interface is pretty much lost immediately after an operator.

var bs = new BehaviorSubject<int>();

var cs = bs.Select(v => v * v);

Here cs is no longer a BehaviorSubject but only an IObservable so IBehaviorObservable is not accessible. The only place IBehaviorObservable would be usable is right at bs, which is already a BehaviorSubject thus Value is accessible anyway.

There is currently one Behavior-Subject implementation, an implementation that makes most sense. The question is, what other Behavior you want to implement that would be significantly different yet you want to mix it with the standard BehaviorSubject through the IBehaviorObservable interface?

Phoder1 commented 1 year ago

Your contribution makes no sense. The problem with fluent APIs such as Rx is that any specific type/interface is pretty much lost immediately after an operator.

var bs = new BehaviorSubject<int>();

var cs = bs.Select(v => v * v);

Here cs is no longer a BehaviorSubject but only an IObservable so IBehaviorObservable is not accessible. The only place IBehaviorObservable would be usable is right at bs, which is already a BehaviorSubject thus Value is accessible anyway.

There is currently one Behavior-Subject implementation, an implementation that makes most sense. The question is, what other Behavior you want to implement that would be significantly different yet you want to mix it with the standard BehaviorSubject through the IBehaviorSubject interface?

But it's still useful to expose a BehaviorSubject to create a reactive property.

for example:

public class UserData
{
    public BehaviorSubject<string> UserName;
}

Is useful to allow both access the user's current username and listen to changes, but it forces you to give public write access. The current alternative for such use case if you want a read-only reactive property is:

public class UserData
{
    private BehaviorSubject<string> _userName;

    public string UserName => _userName;
    public IObservable<string> OnUserNameChange => _userName;
}

Also I like to subscribe the behavior subject to other observables in order to "cache" the last value, like:

OnUserNameChangeComplete.Subscribe(_userName);

So of course, when someone will use the username subject like an event he will lose the option to access the Value property and that obviously makes sense, but I don't think it makes the interface useless or makes no sense.

akarnokd commented 1 year ago

Yes, use composition or educate the users of your code to not call the OnXXX methods.

Phoder1 commented 1 year ago

"Educate users of your code"? That makes no sense.

akarnokd commented 1 year ago

Do you have other developers who would use the raw BehaviorSubject in a way you don't want them to do? If yes, just tell them not to call no matter how much urge they feel. If it's just you, then remember not to call them (other than the places where you want to update the subject of course).

GalBNx commented 1 year ago

@akarnokd Coming from a big corporate with a lot of users on a codebase - it is not possible to educate developers to not use certain utilities that are practically available. Clean code means safe code. Protecting the user from themselves. It's like saying "why use private functions/properties? just educate your user not to use them.".

akarnokd commented 1 year ago

If education is not viable, then use composition the OP already recognized as the straightforward solution. No Rx library change needed, the users will know little difference and no one has to run back later to Rx to ask yet another interface for whatever method combination to restrict the Rx types to.

GalBNx commented 1 year ago

Well, I see your point - the change is not "a must" since there is a way to achieve a readonly approach today (like OP described). But then again - what are the cons of adding this interface? This approach is harmless, optional, eliminates boilerplate for users that do want a readonly BehaviorObservable and IMO encourages users to practice a safer design.

akarnokd commented 1 year ago

Sounds like the underlying convenience issue would be best solved on the language level. There is a proposal for C#, Roles & Extensions, of which the section Adaptation to Interfaces does look like what the OP would like.