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

fix: chain subscriptions from interop observables #5059

Merged
merged 5 commits into from
Dec 6, 2019

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Oct 5, 2019

Description:

This PR fixes a bug in which subscriptions returned from interop observables are ignored - instead of being composed into the subscription chain. Prior to this fix, when an interop observable was returned to a flattening operator - like mergeMap or switchMap - the interop observable would not be unsubscribed upon explicit unsubscription from the flattening operator.

The bug was introduced in the changes made in #2479 and #4037.

This PR:

  • adds a test helper for simulating an interop observable;
  • copies existing, chaining-related tests and uses an interop observable within them;
  • ensures subscriptions returned from interop observables are added to the subscription chain;
  • fixes another bug in which subscribeToResult was passed outer values and indices when they should have been passed to the InnerSubscriber (the bug would have manifested itself in the now-deprecated result selectors for the flattening operators); and
  • tweaks the subscribeToResult signature to make it safer (if an InnerSubscriber is passed, value and index must be undefined and must be passed via the InnerSubscriber instead).

Related issue (if exists): #5051

Copy link
Member

@benlesh benlesh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me... However, I'd love it if we could add comments to explain why things are structured the way the are in the changed areas. Also, a brief comment in the test about what an "interop" is, just explain the scenario. :)

Thanks, @cartant!

src/internal/operators/switchMap.ts Show resolved Hide resolved
src/internal/operators/skipUntil.ts Show resolved Hide resolved
src/internal/operators/mergeScan.ts Show resolved Hide resolved
src/internal/operators/mergeMap.ts Show resolved Hide resolved
@cartant
Copy link
Collaborator Author

cartant commented Nov 27, 2019

Added comments.

@benlesh benlesh merged commit d7f7078 into ReactiveX:master Dec 6, 2019
@benlesh
Copy link
Member

benlesh commented Dec 6, 2019

@cartant: Unfortunately, the patch branch will need a separate PR. Can you tackle that? The problem is that the tests have changed between the two branches.

@lock lock bot locked as resolved and limited conversation to collaborators Jan 5, 2020
@cartant cartant deleted the issue-5051 branch February 29, 2020 21:56
@cartant cartant restored the issue-5051 branch February 29, 2020 21:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants