feat(Observable): unhandled errors are now reported to HostReportErrors (#3062)

BREAKING CHANGE: Unhandled errors are no longer caught and rethrown, rather they are caught and scheduled to be thrown, which causes them to be reported to window.onerror or process.on('error'), depending on the environment. Consequently, teardown after a synchronous, unhandled, error will no longer occur, as the teardown would not exist, and producer interference cannot occur

Author: benlesh

spec/Observable-spec.ts

@@ -1,36 +1,10 @@
 import { expect } from 'chai';
-import * as sinon from 'sinon';
 import * as Rx from '../src/Rx';
 
 const Subscriber = Rx.Subscriber;
 
 /** @test {Subscriber} */
 describe('Subscriber', () => {
-  describe('when created through create()', () => {
-    it('should not call error() if next() handler throws an error', () => {
-      const errorSpy = sinon.spy();
-      const completeSpy = sinon.spy();
-
-      const subscriber = Subscriber.create(
-        (value: any) => {
-          if (value === 2) {
-            throw 'error!';
-          }
-        },
-        errorSpy,
-        completeSpy
-      );
-
-      subscriber.next(1);
-      expect(() => {
-        subscriber.next(2);
-      }).to.throw('error!');
-
-      expect(errorSpy).not.have.been.called;
-      expect(completeSpy).not.have.been.called;
-    });
-  });
-
   it('should ignore next messages after unsubscription', () => {
     let times = 0;
 

spec/Subscriber-spec.ts

@@ -57,7 +57,7 @@ describe('Observable.ajax', () => {
     expect(axObjectStub).to.have.been.called;
   });
 
-  it('should throw if not able to create XMLHttpRequest', () => {
+  it('should raise an error if not able to create XMLHttpRequest', () => {
     root.XMLHttpRequest = null;
     root.ActiveXObject = null;
 
@@ -66,9 +66,7 @@ describe('Observable.ajax', () => {
       method: ''
     };
 
-    expect(() => {
-      Rx.Observable.ajax(obj).subscribe();
-    }).to.throw();
+    Rx.Observable.ajax(obj).subscribe(null, err => expect(err).to.exist);
   });
 
   it('should create XMLHttpRequest for CORS', () => {
@@ -100,7 +98,7 @@ describe('Observable.ajax', () => {
     expect(xDomainStub).to.have.been.called;
   });
 
-  it('should throw if not able to create CORS request', () => {
+  it('should raise an error if not able to create CORS request', () => {
     root.XMLHttpRequest = null;
     root.XDomainRequest = null;
 
@@ -111,9 +109,7 @@ describe('Observable.ajax', () => {
       withCredentials: true
     };
 
-    expect(() => {
-      Rx.Observable.ajax(obj).subscribe();
-    }).to.throw();
+    Rx.Observable.ajax(obj).subscribe(null, err => expect(err).to.exist);
   });
 
   it('should set headers', () => {
@@ -805,7 +801,7 @@ describe('Observable.ajax', () => {
 
   it('should work fine when XMLHttpRequest ontimeout property is monkey patched', function() {
     Object.defineProperty(root.XMLHttpRequest.prototype, 'ontimeout', {
-      set: function (fn: (e: ProgressEvent) => any) {
+      set(fn: (e: ProgressEvent) => any) {
         const wrapFn = (ev: ProgressEvent) => {
           const result = fn.call(this, ev);
           if (result === false) {
@@ -825,7 +821,11 @@ describe('Observable.ajax', () => {
     };
 
     Rx.Observable.ajax(ajaxRequest)
-      .subscribe();
+      .subscribe({
+        error(err) {
+          /* expected, ignore */
+        }
+      });
 
     const request = MockXMLHttpRequest.mostRecent;
     try {
@@ -881,7 +881,7 @@ describe('Observable.ajax', () => {
 
   it('should work fine when XMLHttpRequest onerror property is monkey patched', function() {
     Object.defineProperty(root.XMLHttpRequest.prototype, 'onerror', {
-      set: function (fn: (e: ProgressEvent) => any) {
+      set(fn: (e: ProgressEvent) => any) {
         const wrapFn = (ev: ProgressEvent) => {
           const result = fn.call(this, ev);
           if (result === false) {
@@ -899,7 +899,11 @@ describe('Observable.ajax', () => {
     Rx.Observable.ajax({
       url: '/flibbertyJibbet'
     })
-      .subscribe();
+      .subscribe({
+        error(err) {
+          /* expected */
+        }
+      });
 
     const request = MockXMLHttpRequest.mostRecent;
 

spec/observables/dom/ajax-spec.ts

@@ -155,56 +155,4 @@ describe('Observable.fromPromise', () => {
       done();
     });
   });
-
-  if (typeof process === 'object' && Object.prototype.toString.call(process) === '[object process]') {
-    it('should globally throw unhandled errors on process', (done: MochaDone) => {
-        const originalException = process.listeners('uncaughtException');
-        process.removeAllListeners('uncaughtException');
-        process.once('uncaughtException', function (error) {
-            expect(error).to.be.an('error', 'fail');
-            originalException.forEach(l => process.addListener('uncaughtException', l));
-            done();
-        });
-
-        Observable.fromPromise(Promise.reject('bad'))
-          .subscribe(
-            (x: any) => { done(new Error('should not be called')); },
-            (e: any) => {
-              expect(e).to.equal('bad');
-              throw new Error('fail');
-            }, () => {
-              done(new Error('should not be called'));
-            });
-    });
-  } else if (typeof window === 'object' &&
-    (Object.prototype.toString.call(window) === '[object global]' || Object.prototype.toString.call(window) === '[object Window]')) {
-    it('should globally throw unhandled errors on window', (done: MochaDone) => {
-      const expected = ['Uncaught fail', 'fail', 'Script error.', 'uncaught exception: fail'];
-      const current = window.onerror;
-      window.onerror = null;
-
-      let invoked = false;
-      function onException(e) {
-        if (invoked) {
-          return;
-        }
-        invoked = true;
-        expect(expected).to.contain(e);
-        window.onerror = current;
-        done();
-      }
-
-      window.onerror = onException;
-
-      Observable.fromPromise(Promise.reject('bad'))
-        .subscribe(
-          (x: any) => { done(new Error('should not be called')); },
-          (e: any) => {
-            expect(e).to.equal('bad');
-            throw 'fail';
-          }, () => {
-            done(new Error('should not be called'));
-          });
-    });
-  }
-});
+});

spec/observables/from-promise-spec.ts

@@ -125,8 +125,9 @@ describe('Observable.fromEvent', () => {
       }
     };
 
-    const subscribe = () => Observable.fromEvent(<any>obj, 'click').subscribe();
-    expect(subscribe).to.throw(TypeError, 'Invalid event target');
+    Observable.fromEvent(obj as any, 'click').subscribe({
+      error(err) { expect(err).to.deep.equal(new TypeError('Invalid event target')); }
+    });
   });
 
   it('should pass through options to addEventListener', () => {

spec/observables/fromEvent-spec.ts

@@ -297,44 +297,6 @@ describe('Observable.prototype.catch', () => {
       sandbox.restore();
     });
 
-    it('should chain a throw from a promise using throw', (done: MochaDone) => {
-      const subscribeSpy = sinon.spy();
-      const testError = new Error('BROKEN PROMISE');
-      Observable.fromPromise(Promise.reject(testError)).catch(err => {
-        throw new Error('BROKEN THROW');
-      }).subscribe(subscribeSpy);
-
-      trueSetTimeout(() => {
-        try {
-          timers.tick(1);
-        } catch (e) {
-          expect(subscribeSpy).not.to.be.called;
-          expect(e.message).to.equal('BROKEN THROW');
-          return done();
-        }
-        done(new Error('This should have thrown an error'));
-      }, 0);
-    });
-
-    it('should chain a throw from a promise using Observable.throw', (done: MochaDone) => {
-      const subscribeSpy = sinon.spy();
-      const testError = new Error('BROKEN PROMISE');
-      Observable.fromPromise(Promise.reject(testError)).catch(err =>
-        Observable.throw(new Error('BROKEN THROW'))
-      ).subscribe(subscribeSpy);
-
-      trueSetTimeout(() => {
-        try {
-          timers.tick(1);
-        } catch (e) {
-          expect(subscribeSpy).not.to.be.called;
-          expect(e.message).to.equal('BROKEN THROW');
-          return done();
-        }
-        done(new Error('This should have thrown an error'));
-      }, 0);
-    });
-
     it('should chain a throw from a promise using Observable.throw', (done: MochaDone) => {
       const subscribeSpy = sinon.spy();
       const errorSpy = sinon.spy();

spec/operators/catch-spec.ts

@@ -13,7 +13,8 @@ const Subject = Rx.Subject;
 
 /** @test {do} */
 describe('Observable.prototype.do', () => {
-  asDiagram('do(x => console.log(x))')('should mirror multiple values and complete', () => {
+  asDiagram('do(x => console.log(x))')
+  ('should mirror multiple values and complete', () => {
     const e1 =  cold('--1--2--3--|');
     const e1subs =   '^          !';
     const expected = '--1--2--3--|';
@@ -68,8 +69,8 @@ describe('Observable.prototype.do', () => {
 
   it('should handle everything with a Subject', (done: MochaDone) => {
     const expected = [1, 2, 3];
-    const results = [];
-    const subject = new Subject();
+    const results: number[] = [];
+    const subject = new Subject<number>();
 
     subject.subscribe({
       next: (x: any) => {
@@ -85,7 +86,7 @@ describe('Observable.prototype.do', () => {
     });
 
     Observable.of(1, 2, 3)
-      .do(<Rx.Observer<number>>subject)
+      .do(subject)
       .subscribe();
   });
 

spec/operators/do-spec.ts

@@ -13,7 +13,6 @@ const Observable = Rx.Observable;
 // function shortcuts
 const addDrama = function (x) { return x + '!'; };
 const identity = function (x) { return x; };
-const throwError = function () { throw new Error(); };
 
 /** @test {map} */
 describe('Observable.prototype.map', () => {
@@ -89,16 +88,6 @@ describe('Observable.prototype.map', () => {
     expectSubscriptions(a.subscriptions).toBe(asubs);
   });
 
-  it('should propagate errors from subscribe', () => {
-    const r = () => {
-      Observable.of(1)
-        .map(identity)
-        .subscribe(throwError);
-    };
-
-    expect(r).to.throw();
-  });
-
   it('should not map an empty observable', () => {
     const a =   cold('|');
     const asubs =    '(^!)';
@@ -187,19 +176,14 @@ describe('Observable.prototype.map', () => {
     const expected = '--a--b---c----d--|';
     const values = {a: 5, b: 14, c: 23, d: 32};
 
-    let invoked = 0;
     const foo = {
       value: 42
     };
     const r = a
       .map(function (x: string, index: number) {
-        invoked++;
         expect(this).to.equal(foo);
         return (parseInt(x) + 1) + (index * 10);
-      }, foo)
-      .do(null, null, () => {
-        expect(invoked).to.equal(4);
-      });
+      }, foo);
 
     expectObservable(r).toBe(expected, values);
     expectSubscriptions(a.subscriptions).toBe(asubs);

spec/operators/map-spec.ts

@@ -1,4 +1,4 @@
-import { expect } from 'chai';
+
 import * as Rx from '../../src/Rx';
 import marbleTestingSignature = require('../helpers/marble-testing'); // tslint:disable-line:no-require-imports
 
@@ -10,9 +10,6 @@ declare const expectSubscriptions: typeof marbleTestingSignature.expectSubscript
 
 const Observable = Rx.Observable;
 
-// function shortcuts
-const throwError = function () { throw new Error(); };
-
 /** @test {mapTo} */
 describe('Observable.prototype.mapTo', () => {
   asDiagram('mapTo(\'a\')')('should map multiple values', () => {
@@ -61,16 +58,6 @@ describe('Observable.prototype.mapTo', () => {
     expectSubscriptions(a.subscriptions).toBe(asubs);
   });
 
-  it('should propagate errors from subscribe', () => {
-    const r = () => {
-      Observable.of(1)
-        .mapTo(-1)
-        .subscribe(throwError);
-    };
-
-    expect(r).to.throw();
-  });
-
   it('should not map an empty observable', () => {
     const a =   cold('|');
     const asubs =    '(^!)';

spec/operators/mapTo-spec.ts

@@ -4,8 +4,6 @@ import { subscribeToResult } from '../../src/util/subscribeToResult';
 import { OuterSubscriber } from '../../src/OuterSubscriber';
 import { $$iterator } from '../../src/symbol/iterator';
 import $$symbolObservable from 'symbol-observable';
-import { Observable } from '../../src/Observable';
-import { Subject } from '../../src/Subject';
 
 describe('subscribeToResult', () => {
   it('should synchronously complete when subscribe to scalarObservable', () => {
@@ -180,14 +178,4 @@ describe('subscribeToResult', () => {
 
     subscribeToResult(subscriber, null);
   });
-
-  it('should not swallow exception in inner subscriber', () => {
-    const source = new Subject();
-
-    source.mergeMapTo(Observable.of(1, 2, 3)).subscribe(() => {
-      throw new Error('meh');
-    });
-
-    expect(() => source.next()).to.throw();
-  });
-});
+});