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

Unsubscribe when take* completes #43

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

veyh
Copy link

@veyh veyh commented Jun 30, 2019

In RxJS, take* unsubscribes after onCompleted/onError. In RxLua, it doesn't. I'm not sure if that's intentional or not, but in case it isn't, here's a fix.

@4O4
Copy link
Contributor

4O4 commented Sep 30, 2019

I've been digging into this as I wasn't exactly optimistic that this PR is gonna solve all issues for me, and now I can say that it is indeed not enough to properly fix the issue, because the problem is actually a bit deeper.
When the observable emits complete or error it should automatically unsubscribe... at least it does so in rxjs implementation where everything works in a predictable way, but not in RxLua.

I've spent a lot of time comparing these two implementations and I've managed to fix the underlaying issue. I'm gonna send PR in a couple days when I polish things up as I was debugging multiple problems and messed the code up a lot :) But the end result will be proper automatic unsubscription in general (not only for take* operators) which will also work properly with cold observables which complete instantly in a synchronous way. This is important because Observables differ from Promises in that they don't have to be asynchronous and it can be leveraged in quite a lot of use cases.

Anyway thank you for your work on this @veyh because even though it's not a full fix, it helped me a bit to find a right direction while researching this issue :)

@4O4
Copy link
Contributor

4O4 commented Oct 17, 2020

Hello, I don't want to send spam but since this is directly related I'll just leave a link to my comment in another issue: #22 (comment)

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 this pull request may close these issues.

2 participants