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

TakeUntil does not stop source stream when notifier completes #2304

Closed
masimplo opened this issue Jan 20, 2017 · 3 comments
Closed

TakeUntil does not stop source stream when notifier completes #2304

masimplo opened this issue Jan 20, 2017 · 3 comments

Comments

@masimplo
Copy link

masimplo commented Jan 20, 2017

RxJS version:
v5.0.0-beta.12

Code to reproduce:

Observable.interval(1000)
  .takeUntil(notifier2)
  .subscribe((i)=>console.log('notifier 2: ' + i), null, ()=>console.log('complete'));

setTimeout(()=>{
  console.log('Notifier2 will now complete, original stream should complete')
  notifier2.complete();
}, 4100);

Expected behavior:
Console should print
"notifier 2: 0"
"notifier 2: 1"
"notifier 2: 2"
"notifier 2: 3"
"Notifier2 will now complete, original stream should complete"
"complete"

Actual behavior:
Console prints
"notifier 2: 0"
"notifier 2: 1"
"notifier 2: 2"
"notifier 2: 3"
"Notifier2 will now complete, original stream should complete"
"notifier 2: 4"
"notifier 2: 5"
and so on...

Additional information:
I think a test like the following should be passing as well:

  it('should take values until notifier completes', () => {
    const e1 =     hot('--a--b--c--d--e--f--g--|');
    const e1subs =     '^            !          ';
    const e2 =     hot('-------------|          ');
    const e2subs =     '^            !          ';
    const expected =   '--a--b--c--d-|          ';

    expectObservable(e1.takeUntil(e2)).toBe(expected);
    expectSubscriptions(e1.subscriptions).toBe(e1subs);
    expectSubscriptions(e2.subscriptions).toBe(e2subs);
  });

but it is not. TakeUntil states that the source Observable should stop if the notifier emits a complete notification

If the notifier emits a value or a complete notification, the output Observable stops mirroring the source Observable and completes.

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/2/edit?js,console

masimplo referenced this issue Jan 20, 2017
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.
@Dorus
Copy link

Dorus commented Jan 23, 2017

Possibly related? #2160

@jayphelps
Copy link
Member

Indeed this is expected behavior, at least for now. See #2160, which the outcome of was that we need to update the docs to call this out specifically and ask that the reactivex website clarify that not all Rx implementations stop taking on notifier complete (e.g. Rx.NET doesn't either). We brought this up again and decided to keep things as-is for now: meeting notes

People can make arguments for and against either side, but the number one reason we chose to keep the existing behavior is because it's always been that way in prev RxJS versions. It certainly wasn't the only reason, but it was a major factor. Hope everyone understands!

(thanks @Dorus!)

@lock
Copy link

lock bot commented Jun 6, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Jun 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants