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

refactor(subscribeToObservable): use subscribeWith util #5333

Merged
merged 4 commits into from
Apr 22, 2020

Conversation

cartant
Copy link
Collaborator

@cartant cartant commented Mar 1, 2020

Description:

This PR refactors the fix to #5051 that was made in #5059.

Instead of testing for interop subscribers/subscriptions in operator implementations, this refactor moves the test into subscribeToObservable - which is where the interop subscription takes place - and ensures that function behaves like subscribe calls made to this package's observables.

That is, it ensures that the subscriber that's passed in is what's returned. If the subscription that's received is not the subscriber, said subscription is added to the subscriber so that unsubscription chains correctly.

The code is much simpler and all of the tests that were added in #5059 pass.


I've made some changes to this PR, as it can be used to resolve the problem that #5239 and #5243 sought to address.

It's explained in this comment:

/**
* Subscribes a subscriber to an observable and ensures the subscriber is
* returned as the subscription.
*
* When an instance of a subscriber from within this package is passed to the
* subscribe method of an observable from within this package, the returned
* subscription is the subscriber instance.
*
* Operator implementations depend upon this behaviour, so it's important
* that interop subscribers and observables behave in a similar manner. If
* they do not, unsubscription chains can be broken.
*
* This function ensures that if the subscription returned from the subscribe
* call is not the subscriber itself, the subscription is added to the
* subscriber and the subscriber is returned. Doing so will ensure that the
* unsubscription chain is not broken.
*
* This function needs to be used wherever an interop observable or
* subscriber could be encountered. There are two such places:
* - within `subscribeToObservable`; and
* - within the `call` method of each operator's `Operator` class.
*
* Within `subscribeToObservable` the observables are almost always going to
* be interop - as they're obtained via the `Symbol.observable` property.
*
* Within the `call` method, the operator's subscriber will be interop -
* relative to the source observable - if the operator is imported from a
* package that uses a different version of RxJS.
*
* @param observable the observable to subscribe to
* @param subscriber the subscriber to be subscribed
* @returns the passed-in subscriber (as the subscription)
*/

Essentially, the subscribeWith util can be used to ensure that subscription chains are not broken - by interop observalbes - in subscribeToObservable or in any operator's call method.

This PR does not make any changes to operator call implementations. I'll do that in a separate PR, if this PR passes review.

If you look at the change made in #5239, it should be clear that the implementation of subscribeWith will solve the problem in a similar manner to the change that's in that PR:

const finallySubscriber = new FinallySubscriber(subscriber, this.callback);
const subscription = source.subscribe(finallySubscriber);
if (subscription !== finallySubscriber) {
subscription.add(finallySubscriber);
}
return subscription;

Related issue: #5051

@cartant cartant changed the title refactor(subscribeToObservable): #5051 fix refactor(subscribeToObservable): use subscribeWith util Mar 1, 2020
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.

Nice work!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants