Skip to content

Commit

Permalink
fix(repeatWhen): support synchronous notifier
Browse files Browse the repository at this point in the history
* test(repeatWhen): rename to repeat in descriptions
* test(repeatWhen): align expectations with docs
* test(repeatWhen): add failing empty notifier test
* test(repeatWhen): add failing notifier test
* fix(repeatWhen): support synchronous notifier
  • Loading branch information
cartant committed Mar 25, 2018
1 parent 4cbd91c commit 8fe004c
Show file tree
Hide file tree
Showing 2 changed files with 62 additions and 4 deletions.
63 changes: 60 additions & 3 deletions spec/operators/repeatWhen-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ describe('Observable.prototype.repeatWhen', () => {
expectSubscriptions(source.subscriptions).toBe(subs);
});

it('should retry when notified via returned notifier on complete', (done: MochaDone) => {
it('should repeat when notified via returned notifier on complete', (done: MochaDone) => {
let retried = false;
const expected = [1, 2, 1, 2];
let i = 0;
Expand All @@ -64,22 +64,79 @@ describe('Observable.prototype.repeatWhen', () => {
});
});

it('should retry when notified and complete on returned completion', (done: MochaDone) => {
const expected = [1, 2, 1, 2];
it('should not repeat when applying an empty notifier', (done: MochaDone) => {
const expected = [1, 2];
const nexted: number[] = [];
Observable.of(1, 2)
.map((n: number) => {
return n;
})
.repeatWhen((notifications: any) => Observable.empty())
.subscribe((n: number) => {
expect(n).to.equal(expected.shift());
nexted.push(n);
}, (err: any) => {
done(new Error('should not be called'));
}, () => {
expect(nexted).to.deep.equal([1, 2]);
done();
});
});

it('should not error when applying an empty synchronous notifier', () => {
const errors: any[] = [];
// The current Subscriber.prototype.error implementation does nothing for
// stopped subscribers. This test was written to fail and expose a problem
// with synchronous notifiers. However, by the time the error occurs the
// subscriber is stopped, so the test logs errors by both patching the
// prototype and by using an error callback (for when/if the do-nothing-if-
// stopped behaviour is fixed).
const originalSubscribe = Observable.prototype.subscribe;
Observable.prototype.subscribe = function (...args: any[]): any {
let [subscriber] = args;
if (!(subscriber instanceof Rx.Subscriber)) {
subscriber = new Rx.Subscriber<any>(...args);
}
subscriber.error = function (err: any): void {
errors.push(err);
Rx.Subscriber.prototype.error.call(this, err);
};
return originalSubscribe.call(this, subscriber);
};
Observable.of(1, 2)
.repeatWhen((notifications: any) => Observable.empty())
.subscribe(undefined, err => errors.push(err));
Observable.prototype.subscribe = originalSubscribe;
expect(errors).to.deep.equal([]);
});

it('should not error when applying a non-empty synchronous notifier', () => {
const errors: any[] = [];
// The current Subscriber.prototype.error implementation does nothing for
// stopped subscribers. This test was written to fail and expose a problem
// with synchronous notifiers. However, by the time the error occurs the
// subscriber is stopped, so the test logs errors by both patching the
// prototype and by using an error callback (for when/if the do-nothing-if-
// stopped behaviour is fixed).
const originalSubscribe = Observable.prototype.subscribe;
Observable.prototype.subscribe = function (...args: any[]): any {
let [subscriber] = args;
if (!(subscriber instanceof Rx.Subscriber)) {
subscriber = new Rx.Subscriber<any>(...args);
}
subscriber.error = function (err: any): void {
errors.push(err);
Rx.Subscriber.prototype.error.call(this, err);
};
return originalSubscribe.call(this, subscriber);
};
Observable.of(1, 2)
.repeatWhen((notifications: any) => Observable.of(1))
.subscribe(undefined, err => errors.push(err));
Observable.prototype.subscribe = originalSubscribe;
expect(errors).to.deep.equal([]);
});

it('should apply an empty notifier on an empty source', () => {
const source = cold( '|');
const subs = '(^!)';
Expand Down
3 changes: 2 additions & 1 deletion src/operators/repeatWhen.ts
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ class RepeatWhenSubscriber<T, R> extends OuterSubscriber<T, R> {
if (!this.isStopped) {
if (!this.retries) {
this.subscribeToRetries();
} else if (this.retriesSubscription.closed) {
}
if (!this.retriesSubscription || this.retriesSubscription.closed) {
return super.complete();
}

Expand Down

0 comments on commit 8fe004c

Please sign in to comment.