From 8fe004c2126325d8b27e13d44b1b55a8b6bc8628 Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 25 Mar 2018 04:59:01 +1000 Subject: [PATCH 1/2] fix(repeatWhen): support synchronous notifier * 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 --- spec/operators/repeatWhen-spec.ts | 63 +++++++++++++++++++++++++++++-- src/operators/repeatWhen.ts | 3 +- 2 files changed, 62 insertions(+), 4 deletions(-) diff --git a/spec/operators/repeatWhen-spec.ts b/spec/operators/repeatWhen-spec.ts index fb233d8fbd..59593c8fc0 100644 --- a/spec/operators/repeatWhen-spec.ts +++ b/spec/operators/repeatWhen-spec.ts @@ -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; @@ -64,8 +64,9 @@ 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; @@ -73,13 +74,69 @@ describe('Observable.prototype.repeatWhen', () => { .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(...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(...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 = '(^!)'; diff --git a/src/operators/repeatWhen.ts b/src/operators/repeatWhen.ts index daa4fdaa20..85775447ab 100644 --- a/src/operators/repeatWhen.ts +++ b/src/operators/repeatWhen.ts @@ -76,7 +76,8 @@ class RepeatWhenSubscriber extends OuterSubscriber { if (!this.isStopped) { if (!this.retries) { this.subscribeToRetries(); - } else if (this.retriesSubscription.closed) { + } + if (!this.retriesSubscription || this.retriesSubscription.closed) { return super.complete(); } From 3c86f32761071953a59da899b58353bb4d11233a Mon Sep 17 00:00:00 2001 From: Nicholas Jamieson Date: Sun, 25 Mar 2018 17:06:04 +1000 Subject: [PATCH 2/2] test(repeatWhen): fix TypeScript 2.0 issue --- spec/operators/repeatWhen-spec.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/operators/repeatWhen-spec.ts b/spec/operators/repeatWhen-spec.ts index 59593c8fc0..a6e2fd0944 100644 --- a/spec/operators/repeatWhen-spec.ts +++ b/spec/operators/repeatWhen-spec.ts @@ -95,7 +95,7 @@ describe('Observable.prototype.repeatWhen', () => { Observable.prototype.subscribe = function (...args: any[]): any { let [subscriber] = args; if (!(subscriber instanceof Rx.Subscriber)) { - subscriber = new Rx.Subscriber(...args); + subscriber = new Rx.Subscriber(args[0], args[1], args[2]); } subscriber.error = function (err: any): void { errors.push(err); @@ -122,7 +122,7 @@ describe('Observable.prototype.repeatWhen', () => { Observable.prototype.subscribe = function (...args: any[]): any { let [subscriber] = args; if (!(subscriber instanceof Rx.Subscriber)) { - subscriber = new Rx.Subscriber(...args); + subscriber = new Rx.Subscriber(args[0], args[1], args[2]); } subscriber.error = function (err: any): void { errors.push(err);