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.

Sign up for GitHub

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

Jump to bottom

feat(Subscription): add no longer returns unnecessary Subscription … #5656

Merged
merged 1 commit into from
Aug 19, 2020
Merged

feat(Subscription): add no longer returns unnecessary Subscription … #5656

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion api_guard/dist/types/index.d.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -548,7 +548,7 @@ export declare class Subscription implements SubscriptionLike {
protected _parentOrParents: Subscription | Subscription[] | null;
closed: boolean;
constructor(unsubscribe?: () => void);
add(teardown: TeardownLogic): Subscription;
add(teardown: TeardownLogic): void;
remove(subscription: Subscription): void;
unsubscribe(): void;
static EMPTY: Subscription;
Expand Down
85 changes: 25 additions & 60 deletions spec/Subscription-spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,85 +4,50 @@ import { Observable, UnsubscriptionError, Subscription, merge } from 'rxjs';
/** @test {Subscription} */
describe('Subscription', () => {
describe('Subscription.add()', () => {
it('Should return self if the self is passed', () => {
const sub = new Subscription();
const ret = sub.add(sub);

expect(ret).to.equal(sub);
});

it('Should return Subscription.EMPTY if it is passed', () => {
const sub = new Subscription();
const ret = sub.add(Subscription.EMPTY);

expect(ret).to.equal(Subscription.EMPTY);
});

it('Should return Subscription.EMPTY if it is called with `void` value', () => {
const sub = new Subscription();
const ret = sub.add(undefined);
expect(ret).to.equal(Subscription.EMPTY);
});

it('Should return a new Subscription created with teardown function if it is passed a function', () => {
const sub = new Subscription();

it('should unsubscribe child subscriptions', () => {
const main = new Subscription();

let isCalled = false;
const ret = sub.add(function() {
const child = new Subscription(() => {
isCalled = true;
});
ret.unsubscribe();
main.add(child);
main.unsubscribe();

expect(isCalled).to.equal(true);
});

it('Should wrap the AnonymousSubscription and return a subscription that unsubscribes and removes it when unsubbed', () => {
const sub: any = new Subscription();
let called = false;
const arg = {
unsubscribe: () => called = true,
};
const ret = sub.add(arg);

expect(called).to.equal(false);
expect(sub._subscriptions.length).to.equal(1);
ret.unsubscribe();
expect(called).to.equal(true);
expect(sub._subscriptions.length).to.equal(0);
});

it('Should return the passed one if passed a AnonymousSubscription having not function `unsubscribe` member', () => {
const sub = new Subscription();
const arg = {
isUnsubscribed: false,
unsubscribe: undefined as any,
};
const ret = sub.add(arg as any);

expect(ret).to.equal(arg);
});

it('Should return the passed one if the self has been unsubscribed', () => {
it('should unsubscribe child subscriptions if it has already been unsubscribed', () => {
const main = new Subscription();
main.unsubscribe();

const child = new Subscription();
const ret = main.add(child);
let isCalled = false;
const child = new Subscription(() => {
isCalled = true;
});
main.add(child);

expect(ret).to.equal(child);
expect(isCalled).to.equal(true);
});

it('Should unsubscribe the passed one if the self has been unsubscribed', () => {
it('should unsubscribe a teardown function that was passed', () => {
let isCalled = false;
const main = new Subscription();
main.add(() => {
isCalled = true;
});
main.unsubscribe();
expect(isCalled).to.be.true;
});

it('should unsubscribe a teardown function that was passed immediately if it has been unsubscribed', () => {
let isCalled = false;
const child = new Subscription(() => {
const main = new Subscription();
main.unsubscribe();
main.add(() => {
isCalled = true;
});
main.add(child);

expect(isCalled).to.equal(true);
expect(isCalled).to.be.true;
});
});

Expand Down
15 changes: 7 additions & 8 deletions src/internal/Subscription.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,11 @@ export class Subscription implements SubscriptionLike {
* `remove()` to remove the passed teardown logic from the inner subscriptions
* list.
*/
add(teardown: TeardownLogic): Subscription {
add(teardown: TeardownLogic): void {
let subscription = (<Subscription>teardown);

if (!teardown) {
return Subscription.EMPTY;
return;
}

switch (typeof teardown) {
Expand All @@ -153,10 +153,9 @@ export class Subscription implements SubscriptionLike {
case 'object':
if (subscription === this || subscription.closed || typeof subscription.unsubscribe !== 'function') {
// This also covers the case where `subscription` is `Subscription.EMPTY`, which is always in `closed` state.
return subscription;
return;
} else if (this.closed) {
subscription.unsubscribe();
return subscription;
} else if (!(subscription instanceof Subscription)) {
const tmp = subscription;
subscription = new Subscription();
Expand All @@ -170,14 +169,14 @@ export class Subscription implements SubscriptionLike {

// Add `this` as parent of `subscription` if that's not already the case.
let { _parentOrParents } = subscription;
if (_parentOrParents === null) {
if (_parentOrParents == null) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

thought: Yikes. I wonder if there's a lint rule to catch this sort of thing?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is intentional. == null matches null or undefined.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah. I know, your change fixes a potential bug. I'm wondering if comparisons like this could be elsewhere. I know there is a lint rule that relaxes === when comparing null, but IDK that there's one that enforces/recommends the use of == when comparing against null. With the varied use of null and undefined in this codebase - callers can pass either - it would be interesting - to me, anyway - to see if this occurs elsewhere. A search could find it, but a lint rule could enforce it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems that there are about half a dozen other uses of either === null or === undefined throughout the codebase (ignoring the docs app).

// If we don't have a parent, then set `subscription._parents` to
// the `this`, which is the common case that we optimize for.
subscription._parentOrParents = this;
} else if (_parentOrParents instanceof Subscription) {
if (_parentOrParents === this) {
// The `subscription` already has `this` as a parent.
return subscription;
return;
}
// If there's already one parent, but not multiple, allocate an
// Array to store the rest of the parent Subscriptions.
Expand All @@ -187,7 +186,7 @@ export class Subscription implements SubscriptionLike {
_parentOrParents.push(this);
} else {
// The `subscription` already has `this` as a parent.
return subscription;
return;
}

// Optimize for the common case when adding the first subscription.
Expand All @@ -198,7 +197,7 @@ export class Subscription implements SubscriptionLike {
subscriptions.push(subscription);
}

return subscription;
return;
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/internal/operators/exhaust.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ class SwitchFirstSubscriber<T> extends SimpleOuterSubscriber<T, T> {

protected _next(value: T): void {
if (!this.innerSubscription) {
this.innerSubscription = this.add(innerSubscribe(value, new SimpleInnerSubscriber(this)));
this.add(this.innerSubscription = innerSubscribe(value, new SimpleInnerSubscriber(this)));
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/internal/operators/subscribeOn.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ export interface DispatchArg<T> {

class SubscribeOnObservable<T> extends Observable<T> {
/** @nocollapse */
static dispatch<T>(this: SchedulerAction<T>, arg: DispatchArg<T>): Subscription {
static dispatch<T>(this: SchedulerAction<T>, arg: DispatchArg<T>) {
const { source, subscriber } = arg;
return this.add(source.subscribe(subscriber));
this.add(source.subscribe(subscriber));
}

constructor(
Expand Down