From fd1e039be4c83af3b65722ffc4de076afd11c7e8 Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Tue, 25 Aug 2020 13:39:19 -0500 Subject: [PATCH] refactor(Subscription): Get rid of _unsubscribe weirdness (#5670) * refactor(Subscription): Get rid of _unsubscribe weirdness This is just another step toward getting what we're doing with inheritance to be a little more sane, ultimately, this may be refactored further, as I am not happy with some of these subscribers adding their own teardowns, but this is a start. The goal was to get rid of the weirdness around `_unsubscribe` in our code base. - Moves to calling `_unsubscribe` a new internal name in `Subscription`: `initialTeardown`, because it's slightly more descriptive - Removes `_unsubscribe` usage from the entire codebase. - Formatted a few files just for my sanity. * chore: address comments --- api_guard/dist/types/index.d.ts | 2 +- src/internal/Subscriber.ts | 4 +-- src/internal/Subscription.ts | 29 ++++--------------- .../observable/ConnectableObservable.ts | 12 ++++---- src/internal/operators/bufferTime.ts | 4 +-- src/internal/operators/bufferWhen.ts | 4 +-- src/internal/operators/groupBy.ts | 8 ++--- src/internal/operators/refCount.ts | 3 +- src/internal/operators/switchMap.ts | 5 +--- src/internal/operators/window.ts | 4 +-- src/internal/operators/windowCount.ts | 3 +- src/internal/operators/windowToggle.ts | 4 +-- src/internal/scheduler/AsyncAction.ts | 4 +-- 13 files changed, 35 insertions(+), 51 deletions(-) diff --git a/api_guard/dist/types/index.d.ts b/api_guard/dist/types/index.d.ts index 13ec6686cc..e649c59f1a 100644 --- a/api_guard/dist/types/index.d.ts +++ b/api_guard/dist/types/index.d.ts @@ -548,7 +548,7 @@ export declare class Subscriber extends Subscription implements Observer { export declare class Subscription implements SubscriptionLike { closed: boolean; - constructor(unsubscribe?: () => void); + constructor(initialTeardown?: (() => void) | undefined); add(teardown: TeardownLogic): void; remove(teardown: Exclude): void; unsubscribe(): void; diff --git a/src/internal/Subscriber.ts b/src/internal/Subscriber.ts index 8f5de1be8a..c371050630 100644 --- a/src/internal/Subscriber.ts +++ b/src/internal/Subscriber.ts @@ -158,6 +158,7 @@ export class SafeSubscriber extends Subscriber { error?: ((e?: any) => void) | null, complete?: (() => void) | null) { super(); + this.add(this._teardown); let next: ((value: T) => void) | undefined; @@ -281,8 +282,7 @@ export class SafeSubscriber extends Subscriber { return false; } - /** @internal This is an internal implementation detail, do not use. */ - _unsubscribe(): void { + private _teardown = () => { const { _parentSubscriber } = this; this._parentSubscriber = null!; _parentSubscriber.unsubscribe(); diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 9c4cd4de3a..a9fd3ba72e 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -40,15 +40,10 @@ export class Subscription implements SubscriptionLike { private _teardowns: Exclude[] | null = null; /** - * @param {function(): void} [unsubscribe] A function describing how to - * perform the disposal of resources when the `unsubscribe` method is called. + * @param initialTeardown A function executed first as part of the teardown + * process that is kicked off when {@link unsubscribe} is called. */ - constructor(unsubscribe?: () => void) { - if (unsubscribe) { - (this as any)._ctorUnsubscribe = true; - (this as any)._unsubscribe = unsubscribe; - } - } + constructor(private initialTeardown?: () => void) {} /** * Disposes the resources held by the subscription. May, for instance, cancel @@ -76,22 +71,10 @@ export class Subscription implements SubscriptionLike { } } - const _unsubscribe = (this as any)._unsubscribe; - if (isFunction(_unsubscribe)) { - // It's only possible to null _unsubscribe - to release the reference to - // any teardown function passed in the constructor - if the property was - // actually assigned in the constructor, as there are some classes that - // are derived from Subscriber (which derives from Subscription) that - // implement an _unsubscribe method as a mechanism for obtaining - // unsubscription notifications and some of those subscribers are - // recycled. Also, in some of those subscribers, _unsubscribe switches - // from a prototype method to an instance property - see notifyNext in - // RetryWhenSubscriber. - if ((this as any)._ctorUnsubscribe) { - (this as any)._unsubscribe = undefined; - } + const { initialTeardown } = this; + if (isFunction(initialTeardown)) { try { - _unsubscribe.call(this); + initialTeardown(); } catch (e) { errors = e instanceof UnsubscriptionError ? e.errors : [e]; } diff --git a/src/internal/observable/ConnectableObservable.ts b/src/internal/observable/ConnectableObservable.ts index 1613fc8634..431618a06d 100644 --- a/src/internal/observable/ConnectableObservable.ts +++ b/src/internal/observable/ConnectableObservable.ts @@ -74,18 +74,19 @@ class ConnectableSubscriber extends Subscriber { constructor(protected destination: Subject, private connectable: ConnectableObservable) { super(); + this.add(this._teardown); } protected _error(err: any): void { - this._unsubscribe(); + this._teardown(); super._error(err); } protected _complete(): void { this.connectable._isComplete = true; - this._unsubscribe(); + this._teardown(); super._complete(); } - protected _unsubscribe() { - const connectable = this.connectable; + private _teardown = () => { + const connectable = this.connectable as any; if (connectable) { this.connectable = null!; const connection = connectable._connection; @@ -125,9 +126,10 @@ class RefCountSubscriber extends Subscriber { constructor(destination: Subscriber, private connectable: ConnectableObservable) { super(destination); + this.add(this._teardown); } - protected _unsubscribe() { + private _teardown = () => { const { connectable } = this; if (!connectable) { diff --git a/src/internal/operators/bufferTime.ts b/src/internal/operators/bufferTime.ts index a53126edc3..b9bd828d68 100644 --- a/src/internal/operators/bufferTime.ts +++ b/src/internal/operators/bufferTime.ts @@ -139,6 +139,7 @@ class BufferTimeSubscriber extends Subscriber { private maxBufferSize: number, private scheduler: SchedulerLike) { super(destination); + this.add(this._teardown); const context = this.openContext(); this.timespanOnly = bufferCreationInterval == null || bufferCreationInterval < 0; if (this.timespanOnly) { @@ -184,8 +185,7 @@ class BufferTimeSubscriber extends Subscriber { super._complete(); } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { this.contexts = null!; } diff --git a/src/internal/operators/bufferWhen.ts b/src/internal/operators/bufferWhen.ts index 9279c38ebb..20adc764dc 100644 --- a/src/internal/operators/bufferWhen.ts +++ b/src/internal/operators/bufferWhen.ts @@ -75,6 +75,7 @@ class BufferWhenSubscriber extends SimpleOuterSubscriber { constructor(destination: Subscriber, private closingSelector: () => Observable) { super(destination); + this.add(this._teardown); this.openBuffer(); } @@ -90,8 +91,7 @@ class BufferWhenSubscriber extends SimpleOuterSubscriber { super._complete(); } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { this.buffer = null!; this.subscribing = false; } diff --git a/src/internal/operators/groupBy.ts b/src/internal/operators/groupBy.ts index c8b17f4ec5..12668a7306 100644 --- a/src/internal/operators/groupBy.ts +++ b/src/internal/operators/groupBy.ts @@ -251,17 +251,17 @@ class GroupBySubscriber extends Subscriber implements RefCountSubscr */ class GroupDurationSubscriber extends Subscriber { constructor(private key: K, - private group: Subject, + group: Subject, private parent: GroupBySubscriber) { super(group); + this.add(this._teardown); } - protected _next(value: T): void { + protected _next(): void { this.complete(); } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { const { parent, key } = this; this.key = this.parent = null!; if (parent) { diff --git a/src/internal/operators/refCount.ts b/src/internal/operators/refCount.ts index b7baf6932c..4f66a8592a 100644 --- a/src/internal/operators/refCount.ts +++ b/src/internal/operators/refCount.ts @@ -89,9 +89,10 @@ class RefCountSubscriber extends Subscriber { constructor(destination: Subscriber, private connectable: ConnectableObservable) { super(destination); + this.add(this._teardown); } - protected _unsubscribe() { + private _teardown = () => { const { connectable } = this; if (!connectable) { diff --git a/src/internal/operators/switchMap.ts b/src/internal/operators/switchMap.ts index 3fd4bc90b4..b0b873f228 100644 --- a/src/internal/operators/switchMap.ts +++ b/src/internal/operators/switchMap.ts @@ -137,11 +137,8 @@ class SwitchMapSubscriber extends SimpleOuterSubscriber { if (!innerSubscription || innerSubscription.closed) { super._complete(); } - this.unsubscribe(); - } - - protected _unsubscribe() { this.innerSubscription = undefined; + this.unsubscribe(); } notifyComplete(): void { diff --git a/src/internal/operators/window.ts b/src/internal/operators/window.ts index d0f6e89ff1..50cc99017c 100644 --- a/src/internal/operators/window.ts +++ b/src/internal/operators/window.ts @@ -80,6 +80,7 @@ class WindowSubscriber extends SimpleOuterSubscriber { constructor(destination: Subscriber>) { super(destination); + this.add(this._teardown); destination.next(this.window); } @@ -109,8 +110,7 @@ class WindowSubscriber extends SimpleOuterSubscriber { this.destination.complete(); } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { this.window = null!; } diff --git a/src/internal/operators/windowCount.ts b/src/internal/operators/windowCount.ts index ff670ba0dd..b200644921 100644 --- a/src/internal/operators/windowCount.ts +++ b/src/internal/operators/windowCount.ts @@ -98,6 +98,7 @@ class WindowCountSubscriber extends Subscriber { private windowSize: number, private startWindowEvery: number) { super(destination); + this.add(this._teardown); destination.next(this.windows[0]); } @@ -142,7 +143,7 @@ class WindowCountSubscriber extends Subscriber { this.destination.complete(); } - protected _unsubscribe() { + private _teardown = () => { this.count = 0; this.windows = null!; } diff --git a/src/internal/operators/windowToggle.ts b/src/internal/operators/windowToggle.ts index 588c0617f0..d4f30b4011 100644 --- a/src/internal/operators/windowToggle.ts +++ b/src/internal/operators/windowToggle.ts @@ -90,6 +90,7 @@ class WindowToggleSubscriber extends ComplexOuterSubscriber { private openings: Observable, private closingSelector: (openValue: O) => Observable) { super(destination); + this.add(this._teardown); this.add(this.openSubscription = innerSubscribe(openings, new ComplexInnerSubscriber(this, openings, 0))); } @@ -137,8 +138,7 @@ class WindowToggleSubscriber extends ComplexOuterSubscriber { super._complete(); } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { const { contexts } = this; this.contexts = null!; if (contexts) { diff --git a/src/internal/scheduler/AsyncAction.ts b/src/internal/scheduler/AsyncAction.ts index e819b74853..72ac7117af 100644 --- a/src/internal/scheduler/AsyncAction.ts +++ b/src/internal/scheduler/AsyncAction.ts @@ -20,6 +20,7 @@ export class AsyncAction extends Action { constructor(protected scheduler: AsyncScheduler, protected work: (this: SchedulerAction, state?: T) => void) { super(scheduler, work); + this.add(this._teardown); } public schedule(state?: T, delay: number = 0): Subscription { @@ -132,8 +133,7 @@ export class AsyncAction extends Action { } } - /** @deprecated This is an internal implementation detail, do not use. */ - _unsubscribe() { + private _teardown = () => { const id = this.id; const scheduler = this.scheduler;