Skip to content

Commit

Permalink
feat(Observable): unhandled errors are now reported to HostReportErro…
Browse files Browse the repository at this point in the history
…rs (#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
  • Loading branch information
benlesh authored Nov 10, 2017
1 parent ad143f1 commit cd9626a
Show file tree
Hide file tree
Showing 15 changed files with 217 additions and 506 deletions.
322 changes: 117 additions & 205 deletions spec/Observable-spec.ts

Large diffs are not rendered by default.

26 changes: 0 additions & 26 deletions spec/Subscriber-spec.ts
Original file line number Diff line number Diff line change
@@ -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;

Expand Down
28 changes: 16 additions & 12 deletions spec/observables/dom/ajax-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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;

Expand All @@ -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', () => {
Expand Down Expand Up @@ -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;

Expand All @@ -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', () => {
Expand Down Expand Up @@ -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) {
Expand All @@ -825,7 +821,11 @@ describe('Observable.ajax', () => {
};

Rx.Observable.ajax(ajaxRequest)
.subscribe();
.subscribe({
error(err) {
/* expected, ignore */
}
});

const request = MockXMLHttpRequest.mostRecent;
try {
Expand Down Expand Up @@ -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) {
Expand All @@ -899,7 +899,11 @@ describe('Observable.ajax', () => {
Rx.Observable.ajax({
url: '/flibbertyJibbet'
})
.subscribe();
.subscribe({
error(err) {
/* expected */
}
});

const request = MockXMLHttpRequest.mostRecent;

Expand Down
54 changes: 1 addition & 53 deletions spec/observables/from-promise-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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'));
});
});
}
});
});
5 changes: 3 additions & 2 deletions spec/observables/fromEvent-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down
38 changes: 0 additions & 38 deletions spec/operators/catch-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
Expand Down
9 changes: 5 additions & 4 deletions spec/operators/do-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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--|';
Expand Down Expand Up @@ -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) => {
Expand All @@ -85,7 +86,7 @@ describe('Observable.prototype.do', () => {
});

Observable.of(1, 2, 3)
.do(<Rx.Observer<number>>subject)
.do(subject)
.subscribe();
});

Expand Down
18 changes: 1 addition & 17 deletions spec/operators/map-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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 = '(^!)';
Expand Down Expand Up @@ -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);
Expand Down
15 changes: 1 addition & 14 deletions spec/operators/mapTo-spec.ts
Original file line number Diff line number Diff line change
@@ -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

Expand All @@ -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', () => {
Expand Down Expand Up @@ -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 = '(^!)';
Expand Down
14 changes: 1 addition & 13 deletions spec/util/subscribeToResult-spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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', () => {
Expand Down Expand Up @@ -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();
});
});
});
Loading

0 comments on commit cd9626a

Please sign in to comment.