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

fix(takeWhile): force unsubscribe when it completes or errors #2470

Merged
merged 1 commit into from
Jun 14, 2017
Merged

fix(takeWhile): force unsubscribe when it completes or errors #2470

merged 1 commit into from
Jun 14, 2017

Conversation

mpodlasin
Copy link
Contributor

Force unsubscribe on takeWhile when it completes or errors, even when used with operator that does not unsubscribe reliably, so that source Observable is never subscribed longer than it needs to.

Related issues:

takeWhile suffered from the same problem as first: #2455
It is a concrete case of more general problem: #2459

@mpodlasin
fix(takeWhile): force unsubscribe when it completes or errors
964fb6c 
Force unsubscribe on `takeWhile` when it completes or errors,
even when used with operator that does not unsubscribe reliably,
so that source Observable is never subscribed longer than it
needs to.
@mpodlasin mpodlasin mentioned this pull request Mar 16, 2017
Closed
@coveralls
Copy link

coveralls commented Mar 16, 2017 •
edited
Loading

Coverage Status

Coverage increased (+0.0007%) to 97.69% when pulling 964fb6c on Podlas29:take-while-with-switch-map-fix into a88ac2a on ReactiveX:master.

@benlesh
Copy link
Member

benlesh commented Mar 21, 2017

Like #2463, I worry that this will be a breaking change for some people, and we might have to release this with the next major (which we should probably do pretty soon)... @staltz? @trxcllnt?

@trxcllnt
Copy link
Member

@benlesh wouldn't all bug fixes count as breaking changes then?

@staltz
Copy link
Member

staltz commented Mar 23, 2017

wouldn't all bug fixes count as breaking changes then?

Yup, and there's no shame in that. If people are unknowingly relying on buggy behavior, when the bug gets fixed, their app will behave differently, and who knows, that new behavior might look like a bug to their business logic.

@kwonoj
Copy link
Member

kwonoj commented Mar 23, 2017

Interesting, to clarify, I don't object to make this breaking changes for major but just for curiosity - if some behavior was unintended / undocumented and ppl was using it by just found it, still we consider fixing it has major breaking changes? (this one looks in those way to me)

@mpodlasin mpodlasin mentioned this pull request Mar 29, 2017
Merged
@mpodlasin mpodlasin mentioned this pull request Apr 12, 2017
Merged
@benlesh benlesh changed the base branch from master to next June 14, 2017 20:56
@benlesh benlesh merged commit 2b4a96c into ReactiveX:next Jun 14, 2017
@mpodlasin mpodlasin mentioned this pull request Jun 16, 2017
Merged
@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
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@mpodlasin @coveralls @benlesh @trxcllnt @staltz @kwonoj