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 docs wrong. #2160

Closed
Dorus opened this issue Nov 29, 2016 · 24 comments
Closed

takeUntil docs wrong. #2160

Dorus opened this issue Nov 29, 2016 · 24 comments

Comments

@Dorus
Copy link

Dorus commented Nov 29, 2016

EDIT: (blesh) We need to update the docs

The docs should describe the behavior when the notifier passed to takeUntil does not emit, and instead completes. (for example, Observable.empty() or Observable.timer(100).ignoreElements())


Original Post

takeUntil on reactivex.io states

The TakeUntil subscribes and begins mirroring the source Observable. It also monitors a second Observable that you provide. If this second Observable emits an item or sends a termination notification, the Observable returned by TakeUntil stops mirroring the source Observable and terminates.

(emphasise is mine)

Similar, the docs state:

takeUntil subscribes and begins mirroring the source Observable. It also monitors a second Observable, notifier that you provide. If the notifier emits a value or a complete notification, the output Observable stops mirroring the source Observable and completes.

(emphasise is mine)

However, in the specs:

it('should take all values when notifier is empty', function () {

I think the correct behaviour would be to terminate when the notifier terminates, however at the very least the docs should be in line with the specs.

@frederikprijck
Copy link

frederikprijck commented Nov 29, 2016

Just for completion, a little code sample to reproduce:

Calling complete doesn't terminate.

const onDestroy$: Subject<any> = new Subject();

Rx.Observable.interval()
    .takeUntil(onDestroy$)
    .subscribe(console.log, undefined, () => console.log("complete"));

setTimeout(() => {
    onDestroy$.complete();
}, 5000);

Calling next does terminate.

const onDestroy$: Subject<any> = new Subject();

Rx.Observable.interval()
    .takeUntil(onDestroy$)
    .subscribe(console.log, undefined, () => console.log("complete"));

setTimeout(() => {
    onDestroy$.next();
}, 5000);

Edit:
For anyone having the need to make it work using complete, you can concate the onDestroy subject using making it terminate:

.takeUntil(onDestroy$.concat(Observable.of(0)))

@staltz staltz added the bug Confirmed bug label Nov 29, 2016
@staltz
Copy link
Member

staltz commented Nov 29, 2016

Seems legit. I mean, I believe it should behave like you described. That said, RxJS v5 behaves just like RxJS v4 in this regard. So it has been like this for a very long time.

@Dorus
Copy link
Author

Dorus commented Nov 29, 2016

For what it is worth. Rx.Net does not terminate on empty. RxJava does terminate on empty. That might explain why reactivex.io is confused on the subject.

@staltz
Copy link
Member

staltz commented Nov 29, 2016

And... FWIW, xstream equivalent to takeUntil is endWhen, and it completes the result stream when the notifier stream completes.

@frederikprijck
Copy link

frederikprijck commented Nov 30, 2016

And FWIW³: I found this issue on ReactiveCocoa.
Even tho I don't expect rxjs to be identical on an operator level, I did like the little conversation over there.

ReactiveCocoa/ReactiveCocoa#1431

@jayphelps
Copy link
Member

jayphelps commented Dec 5, 2016

Hey! We discussed this today at our meeting, notes here. We'd like to keep the status quo as far as behavior, but certainly the docs could call out this behavior explicitly, to prevent any confusion.

@jayphelps jayphelps removed the bug Confirmed bug label Dec 5, 2016
@benlesh benlesh changed the title takeUntil specs (or docs) wrong. takeUntil docs wrong. Dec 5, 2016
@benlesh
Copy link
Member

benlesh commented Dec 5, 2016

I've updated the title of this issue and edited the original post to include the intention of what we need to do to resolve the issue.

@Karasuni
Copy link

#2369

@ghost
Copy link

ghost commented Oct 28, 2017

I ran into this as well. Please update the docs to match the behavior, or update the behavior to match the docs.

@jayphelps
Copy link
Member

@errorx666 would you be interested in contributing?

@OliverJAsh
Copy link
Contributor

Just stumbled into this, and then found this issue and this PR to fix the docs: #3035.

Unfortunately the docs don't seem to be updated yet. I guess this is waiting on the next release?

@insidewhy
Copy link

Just got burned by this also. Wonder how many other developers' time it's going to waste before the documentation finally gets updated ;)

@jayphelps
Copy link
Member

@ohjames I don’t know, would you like to update it?

@OliverJAsh
Copy link
Contributor

I just checked the docs at http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#instance-method-takeUntil, and they don't seem to be updated with the fix in #3035.

The docs on the site read:

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

However, the PR changed this to:

If the notifier emits a value, the output Observable stops mirroring the source Observable and completes. If the notifier doesn't emit any value and completes then takeUntil will pass all values.

martinsik@a82121a#diff-61e6203a9881d218587e80e948e9aef1R23

@jayphelps Do you know why the site is not up-to-date with the docs in the code? I don't know how they're generated or updated.

@Karasuni
Copy link

@jayphelps I've asked last year how we can go about updating the docs. Just like @OliverJAsh says we don't know how these are generated or maintained. The docs have been completely incorrect wasting hours of developer's time for over a year now.

@benlesh
Copy link
Member

benlesh commented Jan 24, 2018

@OliverJAsh @Karasuni ... The documentation build script has been broken for some time, it seems :( I manually updated the docs this morning and all is well.

Going forward, we have a much better documentation effort going on over at github.com/reactivex/rxjs-docs, lead by @ladyleet. They're still in-development, but the goal is to never have this problem again. It's all volunteer work, so if you feel passionately about the documentation for RxJS, I strongly encourage you to spent some time with those fine and talented folks.

It seems like we can close this one now.

@benlesh benlesh closed this as completed Jan 24, 2018
@Dorus
Copy link
Author

Dorus commented Jan 24, 2018

Possibly (hopefully) i'm jumping on this too soon, but http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#instance-method-takeUntil has not yet updated. Did the script already complete?

@benlesh benlesh reopened this Jan 25, 2018
@benlesh
Copy link
Member

benlesh commented Jan 25, 2018

@Dorus what's up there reflects what we have in stable. Any additional changes we'll need to do another PR.

@benlesh benlesh closed this as completed Jan 25, 2018
@frederikprijck
Copy link

@benlesh this issue is about the takeuntil docs being wrong, not out of sync with the stable.

Sounds like a good idea to keep this open until the extra pr has been made + merged, as the take until docs are still wrong.

I can try to take a stab at it any time soon, if others want, feel free to do so.

@Karasuni
Copy link

@ReactiveX/rxjs-docs doesn't appear to be hosted anywhere. Are the docs being maintained separate of code then? I'd expect docs to be generated from the annotated code

@ladyleet
Copy link
Member

@Karasuni it'll be a combination of the two once the docs project is complete - but yes the docs will be generated from the annotated code.

@benlesh benlesh reopened this Jan 25, 2018
@enerdgumen
Copy link

Recently I've observed the behaviour of an expression like empty().pipe(takeUntil(empty())) using rxjs v6 (illustrated in https://codepen.io/maurocchi/pen/MGROKQ?editors=0012), where the resulting observable does not complete.

Reading this ticket I've not understood if the missing completion event is expected or not, however in my opinion it should be considered as a bug.

@benlesh
Copy link
Member

benlesh commented May 24, 2018

@maurocchi ... an empty observable doesn't emit a value, therefore it doesn't "notify". It's like subscribing to an event and never getting an event.

@benlesh
Copy link
Member

benlesh commented May 24, 2018

I think we can close this issue, as the docs have moved and been updated since this was filed forever ago.

@benlesh benlesh closed this as completed May 24, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jun 23, 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

10 participants