From 0f3f49cd6c27ee4d4d41033f16a438c2b69d9220 Mon Sep 17 00:00:00 2001 From: Robert Jackson Date: Mon, 27 Nov 2017 14:49:39 -0500 Subject: [PATCH] [BUGFIX beta] Avoid double throwing unhandled promise rejections. 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. --- packages/ember-metal/lib/error_handler.js | 31 ------------------- packages/ember-metal/lib/index.js | 1 - packages/ember-runtime/lib/ext/rsvp.js | 9 ++++-- packages/ember-runtime/tests/ext/rsvp_test.js | 4 +-- packages/ember/tests/error_handler_test.js | 9 +----- 5 files changed, 9 insertions(+), 45 deletions(-) diff --git a/packages/ember-metal/lib/error_handler.js b/packages/ember-metal/lib/error_handler.js index 72ce2c47412..6921fda95f4 100644 --- a/packages/ember-metal/lib/error_handler.js +++ b/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() { @@ -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() { @@ -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)); - } -} diff --git a/packages/ember-metal/lib/index.js b/packages/ember-metal/lib/index.js index 01288ef1d06..1ed903f78e5 100644 --- a/packages/ember-metal/lib/index.js +++ b/packages/ember-metal/lib/index.js @@ -19,7 +19,6 @@ export { export { getOnerror, setOnerror, - dispatchError, setDispatchOverride, getDispatchOverride } from './error_handler'; diff --git a/packages/ember-runtime/lib/ext/rsvp.js b/packages/ember-runtime/lib/ext/rsvp.js index ca6537e616d..42cabd313e2 100644 --- a/packages/ember-runtime/lib/ext/rsvp.js +++ b/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'; @@ -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; + } } } diff --git a/packages/ember-runtime/tests/ext/rsvp_test.js b/packages/ember-runtime/tests/ext/rsvp_test.js index bc264418327..804b6e3da01 100644 --- a/packages/ember-runtime/tests/ext/rsvp_test.js +++ b/packages/ember-runtime/tests/ext/rsvp_test.js @@ -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) { diff --git a/packages/ember/tests/error_handler_test.js b/packages/ember/tests/error_handler_test.js index 8a7898f1e88..34caf115def 100644 --- a/packages/ember/tests/error_handler_test.js +++ b/packages/ember/tests/error_handler_test.js @@ -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');