-
Notifications
You must be signed in to change notification settings - Fork 3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
fix(takeUntil): unsubscribe notifier when it completes
Fix operator takeUntil to automatically unsubscribe the notifier when it completes. This is to conform RxJS Next with RxJS 4. Somewhat related to issue #577.
- Loading branch information
Showing
2 changed files
with
20 additions
and
13 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
9415196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a test for this commit? TakeUntil does not do what is supposed to when notifier completes, but does so when notifier emits on version 5.0.0-beta.12 which is greater than this commit which was included 5.0.0-alpha-7
9415196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, see these commits https://github.com/ReactiveX/rxjs/pull/586/commits
9415196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But these tests in the commits you are referencing are all for skipUntil not takeUntil.
I think a test like the following should be passing as well:
but it is not. TakeUntil states that
But there is no test for what happens when the notifier completes, which is what I am seeing not working in some sample code I am running trying out takeUntil.
Here is a jsbin demonstrating my case:
http://jsbin.com/lagoquz/1/edit?js,console
9415196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you please open an issue to discuss this? Not in a commit message
9415196
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure here you go #2304