From 6c79ef43aca53f1f0715dbec618aff4f94a20ddc Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Wed, 12 May 2021 11:52:31 -0500 Subject: [PATCH 1/2] feat(Subscription): idempotent add and remove of teardowns - Adding the same instance of a teardown more than once is the same as adding it once - Removing a teardown with `Subscription.prototype.remove` will remove it much faster BREAKING CHANGE: Adding the same function instance to a subscription as a teardown multiple times will now result in that function being executed only once on teardown. This brings us inline with the behavior of EventTarget, and also makes removing teardowns faster. The workaround is to make sure you are adding a new function instance to the Subscription each time if you need the same effect. Resolves #6400 --- spec/operators/delay-spec.ts | 2 +- spec/operators/switchAll-spec.ts | 6 +++--- src/internal/Subscription.ts | 8 +++++--- 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/spec/operators/delay-spec.ts b/spec/operators/delay-spec.ts index bb82933820..197be83233 100644 --- a/spec/operators/delay-spec.ts +++ b/spec/operators/delay-spec.ts @@ -245,7 +245,7 @@ describe('delay', () => { tap({ next() { const [[subscriber]] = subscribeSpy.args; - counts.push(subscriber._teardowns.length); + counts.push(subscriber._teardowns.size); }, complete() { expect(counts).to.deep.equal([1, 1]); diff --git a/spec/operators/switchAll-spec.ts b/spec/operators/switchAll-spec.ts index e182c0bddb..5f9422e3df 100644 --- a/spec/operators/switchAll-spec.ts +++ b/spec/operators/switchAll-spec.ts @@ -318,7 +318,7 @@ describe('switchAll', () => { }); // Expect one child of switchAll(): The oStream - expect((sub as any)._teardowns?.[0]._teardowns?.length).to.equal(1); + expect([...(sub as any)._teardowns?.values()][0]._teardowns?.size).to.equal(1); sub.unsubscribe(); }); @@ -333,10 +333,10 @@ describe('switchAll', () => { oStreamControl.next(n); // creates inner }); // Expect one child of switchAll(): The oStream - expect((sub as any)._teardowns?.[0]._teardowns?.length).to.equal(1); + expect([...((sub as any)._teardowns?.values() ?? [])][0]._teardowns?.size).to.equal(1); // Expect two children of subscribe(): The destination and the first inner // See #4106 - inner subscriptions are now added to destinations - expect((sub as any)._teardowns?.length).to.equal(2); + expect((sub as any)._teardowns?.size).to.equal(2); sub.unsubscribe(); }); diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 9ff58e72da..79eae70cdf 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -34,7 +34,7 @@ export class Subscription implements SubscriptionLike { * The list of registered teardowns to execute upon unsubscription. Adding and removing from this * list occurs in the {@link #add} and {@link #remove} methods. */ - private _teardowns: Exclude[] | null = null; + private _teardowns: Set> | null = null; /** * @param initialTeardown A function executed first as part of the teardown @@ -134,7 +134,7 @@ export class Subscription implements SubscriptionLike { } teardown._addParent(this); } - (this._teardowns = this._teardowns ?? []).push(teardown); + (this._teardowns = this._teardowns ?? new Set()).add(teardown); } } } @@ -190,7 +190,9 @@ export class Subscription implements SubscriptionLike { */ remove(teardown: Exclude): void { const { _teardowns } = this; - _teardowns && arrRemove(_teardowns, teardown); + if (_teardowns) { + _teardowns.delete(teardown); + } if (teardown instanceof Subscription) { teardown._removeParent(this); From 2f5944aef9344abc255df4505c4018336a566a7b Mon Sep 17 00:00:00 2001 From: Ben Lesh Date: Sat, 18 Sep 2021 18:56:10 -0500 Subject: [PATCH 2/2] refactor: use optional chaining --- src/internal/Subscription.ts | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/internal/Subscription.ts b/src/internal/Subscription.ts index 79eae70cdf..1dcbd50199 100644 --- a/src/internal/Subscription.ts +++ b/src/internal/Subscription.ts @@ -189,10 +189,7 @@ export class Subscription implements SubscriptionLike { * @param teardown The teardown to remove from this subscription */ remove(teardown: Exclude): void { - const { _teardowns } = this; - if (_teardowns) { - _teardowns.delete(teardown); - } + this._teardowns?.delete(teardown); if (teardown instanceof Subscription) { teardown._removeParent(this);