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

Simplify takeUntil() unsubscription #121

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Conversation

domfarolino
Copy link
Collaborator

@domfarolino domfarolino commented Feb 20, 2024

This CL introduces no behavior change to the takeUntil() operator, it only implements a simplification. Prior to this PR, takeUntil() declared an AbortController which managed an AbortSignal that fed into the sourceObservable and notifier Observable subscriptions. This was so that the two Observables could trigger unsubscriptions to each other, if i.e., sourceObservable completes, or if notifier next()s or errors()s.

But I didn't realize that we don't actually have to do that. As long as those operations either "complete" or "error" the "outer" Subscriber (that is associated with the Observable returned from takeUntil()), then the outer Subscriber's signal will be aborted, and we can simply use that AbortSignal as the input to the subscription of sourceObservable and notifier and have the same effect, without needing to maintain our own AbortController.

I am merging this myself since I implemented this as part of https://crrev.com/c/5311097 and found that there was no observable (pun) behavior change, as expected.


Preview | Diff

@domfarolino domfarolino merged commit 4e61306 into master Feb 20, 2024
2 checks passed
@domfarolino domfarolino deleted the takeUntil-simplification branch February 20, 2024 22:42
github-actions bot added a commit that referenced this pull request Feb 20, 2024
SHA: 4e61306
Reason: push, by domfarolino

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

1 participant