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

Critical issue with ToReadOnlyReactiveProperty #263

Open
vg-swift opened this issue Oct 25, 2024 · 0 comments
Open

Critical issue with ToReadOnlyReactiveProperty #263

vg-swift opened this issue Oct 25, 2024 · 0 comments

Comments

@vg-swift
Copy link

vg-swift commented Oct 25, 2024

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

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

No branches or pull requests

1 participant