Cysharp / R3

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

Critical issue with ToReadOnlyReactiveProperty #263

Open vg-swift opened 3 weeks ago

vg-swift commented 3 weeks ago

The issue is when the method returns the original ReadOnlyReactiveProperty. Depending on the path it takes, it leads to two completely different behaviours. ConnectedReactiveProperty creates a new subscription, so you would always add it to a Disposable. But in the case where it returns the original property, you would not want to add it to Disposable, as it was probably already added. I would expect ToReadOnlyReactiveProperty to always create a new property. There should be an extension that makes sure of that. It can lead to situation where you accidentally dispose the original property (especially critical if it comes from a higher context)

image

For the context, I'm migrating from UniRx to R3, and this is the biggest issue I have right now. Of course I could check if Observable is ReadOnlyReactiveProperty property beforehand and handle 2 cases separately, but the codebase is too big (over 300 usages). And I can't add my own extension bcs ConnectedReactiveProperty is internal