-
Notifications
You must be signed in to change notification settings - Fork 3k
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
fix(mergeAll): add source subscription to composite before actually subscribing #2479
fix(mergeAll): add source subscription to composite before actually subscribing #2479
Conversation
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.
Just some clarification around a comment.
spec/operators/mergeAll-spec.ts
Outdated
() => results.push('GOOSE!') | ||
); | ||
|
||
// never even get here |
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.
This comment is confusing. Why wouldn't it get here?
@benlesh done. |
…ubscribing Add subscriptions for source Observables to mergeAll composite subscription before actually subscribing to any of these Observables, so that if source Observable emits synchronously and consumer of mergeAll unsubscribes at that moment (for example `take` operator), subscription to source is unsubscribed as well and Observable stops emitting. Closes #2476
@benlesh I've rebased and refactored this PR. |
Add subscriptions for source Observables to mergeAll composite subscription before actually subscribing to any of these Observables, so that if source Observable emits synchronously and consumer of mergeAll unsubscribes at that moment (for example
take
operator), subscription to source is unsubscribed as well and Observable stops emitting.Closes #2476