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.

Sign up for GitHub

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

Jump to bottom

Correct takeUntil comment - does not stop taking values if notifier completes without emitting #2418

Merged
merged 2 commits into from
Sep 19, 2017
Merged

Correct takeUntil comment - does not stop taking values if notifier completes without emitting #2418

merged 2 commits into from
Sep 19, 2017

Conversation

jeffbski
Copy link
Contributor

If this takeUntil test is correct then I believe the comment is wrong, it will not stop taking values if the notifier completes without emitting any values (empty).

https:/ReactiveX/rxjs/blob/master/spec/operators/takeUntil-spec.ts#L38-L48

@jeffbski
docs(Observable): correct takeUntil comment regarding empty notifier
85099de 
Assuming this empty notifier takeUntil test is correct then I believe
that one of the takeUntil comments is wrong. `takeUntil` will not stop
taking values if the notifier completes without emitting any values.
https:/ReactiveX/rxjs/blob/master/spec/operators/takeUntil-spec.ts#L38-L48
@kwonoj kwonoj added the docs Issues and PRs related to documentation label Mar 12, 2017
@kwonoj
Copy link
Member

kwonoj commented Mar 12, 2017

Not sure why but build failed by this change? 🤔

@jeffbski
Copy link
Contributor Author

It looks like maybe it was due to trailing whitespace in a comment line. I have fixed that line so hopefully it will build properly now.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.688% when pulling 8d0946e on jeffbski:patch-1 into 69d051b on ReactiveX:master.

1 similar comment
@coveralls
Copy link

coveralls commented Mar 13, 2017 •
edited
Loading

Coverage Status

Coverage remained the same at 97.688% when pulling 8d0946e on jeffbski:patch-1 into 69d051b on ReactiveX:master.

@DzmitryShylovich DzmitryShylovich mentioned this pull request May 3, 2017
Closed
@kwonoj kwonoj merged commit e5403ef into ReactiveX:master Sep 19, 2017
@aciccarello
Copy link
Contributor

For some reason I'm not seeing this change in the online docs. Has the website been updated?

@frederikprijck
Copy link

When is this change going to be visible on the docs so that #2160 can be closed (and the docs are correct)?

http://reactivex.io/rxjs/class/es6/Observable.js~Observable.html#instance-method-takeUntil still mentions

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.

@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.
Reviewers
No reviews
Assignees
No one assigned
Labels
docs Issues and PRs related to documentation
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jeffbski @kwonoj @coveralls @aciccarello @frederikprijck