Skip to content

Commit

Permalink
[BUGFIX beta] Avoid double throwing unhandled promise rejections.
Browse files Browse the repository at this point in the history
When used in Ember, `RSVP` is configured to settle within
Backburner's configured `actions` queue.

Prior to this change, any unhandled promise rejections would:

* `Ember.testing === true`
  * When `Ember.Test.adapter` was registered the `Test.adapter`'s
    `exception` method would be called, and the rejection would be logged
    to the console.
  * When `Ember.Test.adapter` was not registered, the `defaultDispatch`
    implementation would re-throw the error, and since RSVP settles in the
    run loop this means that the re-thrown error would be caught by the
    currently flushing Backburner queue's `invokeWithOnError` which sends
    any errors to `Ember.onerror` (if present). If `Ember.onerror` was not
    present, the exception would bubble up the "normal" unhandled
    exception system (and ultimately to `window.onerror`).
* `Ember.testing === false`
  * When `Ember.onerror` is present, it would be invoked with the
    rejection reason.
  * When `Ember.onerror` is not present, the rejection reason would be
    logged to the console.

After this change:

* When `Ember.Test.adapter` is present, its `exception` method is
  invoked with the rejection.
* Otherwise the rejection reason is rethrown.

The benefits of this are:

* It is now possible to debug rejected promises via "normal" JS
  exception debugging (e.g. break on uncaught exception).
* There are many fewer decision points, making it much easier to grok
  what is going on.
  • Loading branch information
rwjblue committed Nov 28, 2017
1 parent 80f7d6b commit 0f3f49c
Show file tree
Hide file tree
Showing 5 changed files with 9 additions and 45 deletions.
31 changes: 0 additions & 31 deletions packages/ember-metal/lib/error_handler.js
@@ -1,18 +1,6 @@
import Logger from 'ember-console';
import { isTesting } from 'ember-debug';

// To maintain stacktrace consistency across browsers
let getStack = error => {
let stack = error.stack;
let message = error.message;

if (stack && stack.indexOf(message) === -1) {
stack = `${message}\n${stack}`;
}

return stack;
};

let onerror;
export const onErrorTarget = {
get onerror() {
Expand All @@ -30,14 +18,6 @@ export function setOnerror(handler) {
}

let dispatchOverride;
// dispatch error
export function dispatchError(error) {
if (dispatchOverride) {
dispatchOverride(error);
} else {
defaultDispatch(error);
}
}

// allows testing adapter to override dispatch
export function getDispatchOverride() {
Expand All @@ -46,14 +26,3 @@ export function getDispatchOverride() {
export function setDispatchOverride(handler) {
dispatchOverride = handler;
}

function defaultDispatch(error) {
if (isTesting()) {
throw error;
}
if (onerror) {
onerror(error);
} else {
Logger.error(getStack(error));
}
}
1 change: 0 additions & 1 deletion packages/ember-metal/lib/index.js
Expand Up @@ -19,7 +19,6 @@ export {
export {
getOnerror,
setOnerror,
dispatchError,
setDispatchOverride,
getDispatchOverride
} from './error_handler';
Expand Down
9 changes: 7 additions & 2 deletions packages/ember-runtime/lib/ext/rsvp.js
@@ -1,7 +1,7 @@
import * as RSVP from 'rsvp';
import {
run,
dispatchError
getDispatchOverride
} from 'ember-metal';
import { assert } from 'ember-debug';

Expand All @@ -21,7 +21,12 @@ RSVP.on('error', onerrorDefault);
export function onerrorDefault(reason) {
let error = errorFor(reason);
if (error) {
dispatchError(error);
let overrideDispatch = getDispatchOverride();
if (overrideDispatch) {
overrideDispatch(error);
} else {
throw error;
}
}
}

Expand Down
4 changes: 1 addition & 3 deletions packages/ember-runtime/tests/ext/rsvp_test.js
Expand Up @@ -46,12 +46,10 @@ QUnit.test('Can reject with non-Error object', function(assert) {
try {
run(RSVP, 'reject', 'foo');
} catch (e) {
ok(false, 'should not throw');
equal(e, 'foo', 'should throw with rejection message');
} finally {
setTesting(wasEmberTesting);
}

ok(true);
});

QUnit.test('Can reject with no arguments', function(assert) {
Expand Down
9 changes: 1 addition & 8 deletions packages/ember/tests/error_handler_test.js
Expand Up @@ -361,14 +361,7 @@ function generateRSVPErrorHandlingTests(message, generatePromise, timeout = 10)
});

test(`${message} when Ember.onerror which does rethrow is present (Ember.testing = false) - rsvp`, function(assert) {
// this should be: `assert.expect(2);`, but currently non-testing
// Ember.onerror is called twice for errors in RSVP promises:
//
// 1) the default RSVP error handler calls `dispatchError` (which then calls
// `Ember.onerror`)
// 2) the run wrapping that is done by the RSVP + Backburner
// integration calls `dispatchError` (which then calls `Ember.onerror`)
assert.expect(3);
assert.expect(2);

Ember.testing = false;
let thrown = new Error('the error');
Expand Down

0 comments on commit 0f3f49c

Please sign in to comment.