UniRx icon indicating copy to clipboard operation
UniRx copied to clipboard

ReactiveProperty does not emit when setting same value

Open nikomikulicic opened this issue 5 years ago • 7 comments

Coming from RxSwift, I found this behaviour weird. I'd expect ReactiveProperty to emit value whenever a value is set (regardless of whether it's the same value as before).

I believe this especially makes sense since we're talking about Rx here. You might want to listen to OnNext events whenever the value is being set. If you want to listen to only distinct events, you can just add DistrinctUntilChanged onto your observable chain.

These are the lines of code (ReactiveProperty.cs) I'm refering to:

        public T Value
        {
            get
            {
                return value;
            }
            set
            {
                if (!EqualityComparer.Equals(this.value, value))
                {
                    SetValue(value);
                    if (isDisposed)
                        return;

                    RaiseOnNext(ref value);
                }
            }
        }

More specifically this one: if (!EqualityComparer.Equals(this.value, value))

I've encountered this issue with structs and solved it by making struct a class since EqualityComparer compares structs by value and objects by address. Not ideal, but solved my issues for now.

I wanted to primarily open this discussion and would like to hear your opinion. As mentioned, in RxSwift, a value is always emitted when set. Since this class is in UnityEngineBridge folder, I wasn't sure if this implementation was taken from original .NET rx library or is this something that you might be willing to change.

nikomikulicic avatar Jul 18 '19 09:07 nikomikulicic

use .SetValueAndForceNotify(...)

elhimp avatar Jul 19 '19 18:07 elhimp

@elhimp Thanks, that sure is a better workaround.

I'd still like to hear some thoughts on whether this equality comparison is actually extraneous. It basically reimplements DistinctUntilChanged which is very easy to add when needed.

nikomikulicic avatar Jul 22 '19 06:07 nikomikulicic

It seems extraneous that we should have to call "SetValueAndForceNotify" all over the place. It would be better if the default "Value" implementation was to notify regardless of whether the value has changed or not.

ajon542 avatar Aug 20 '19 19:08 ajon542

Very interesting topic indeed. I'm kinda split on this issue, but by the principle of maximum simplicity, ReactiveProperty should indeed trigger on every value update even if it's equal. But if you think about most use cases of ReactiveProperty, why would you ever need to react on every value instead of every unique value?

Designing your code around normalized models is beneficial. In this case you almost never want to react to values until the model is really changed. This is true at least for UI code.

phobos2077 avatar Aug 06 '20 06:08 phobos2077

I agree @phobos2077 that is the case in most use cases. However, I don't think this is a question of utility, but more of a question of consistency.

Regardless of what you need in most situations, no other Observable behaves in a way where it emits only when value is changed (except when DistinctUntilChanged is added). Any Subject, ReplaySubject, IObservable, etc. emits values regardless of whether the new value is different from previous one. One can argue that subjects don't hold state, and that is a valid argument, but you would still expect the Rx library to behave uniformly across all of their observables.

This way, it comes as a surprise when ReactiveProperty is treated as special and when it behaves differently from other observables. You don't expect such behaviour and you have to learn it the hard way. It adds complexity without adding utility (since utility is already there in DistinctUntilChanged).

nikomikulicic avatar Aug 06 '20 09:08 nikomikulicic

Well I'm not sure about that argument. ReactiveProperty has a state, as you mentioned, so it is inherently a "special case". No contract is broken when it's only emitting distinct values, as it's the whole point of this class. I think you should be able to write your own version of ReactiveProperty to work as you want w/o too much difficulty.

However is the case, it's too late to change this behavior now as introducing it will break all code that relies on current behavior.

phobos2077 avatar Aug 06 '20 09:08 phobos2077

Now that I'm familiar with this behaviour, it's not difficult to work around it. ReactiveProperty is a special case, but my point is that it doesn't have to be, or it can be less than it is without sacrificing function.

I agree that changing this behaviour would break code relying on it. If ever done, it should be done carefully.

nikomikulicic avatar Aug 06 '20 10:08 nikomikulicic