Skip to content

Commit

Permalink
[BUGFIX release] Mark handled errors and don't reraise
Browse files Browse the repository at this point in the history
Ember 2.1 introduced a regression where all errors during a transition
were thrown, even when those errors had been handled in a route's error
action. This PR adds a test to identify the regression, and adds some
logic and state to identify errors that have been handled, and not
reraise them, while maintaining the desired behaviour of throwing errors
that aren't handled.

This is especially valuable in any application that tests error
handling. Since all errors were being thrown regardless of handling,
failed assertions were being registered by qunit, and it became
impossible to test application flow that entered the error hook.

Further details about this issue are here:
#12547

Remove leftover debugger

This addresses an issue where handled errors are still thrown

The added test passes on Ember 2.0. It fails on Ember 2.1 because even though
the error is not bubbled, the application throws the handled error.

This is contrary to what the documentation indicates.

Full details about this issue are here: #12547

This PR also adds a very naive implementation of how to flag an error as having
been handled and prevents reraising, while retaining the functionality of throwing errors
that aren't caught, or are bubbled from the application route

Make error checking methods a part of the router instance

Attach error caching methods to EmberRouter, and use guids for errors

Errors used as keys were just getting `.toString()`ed, so multiple
`Error`s or hashes would all have the same key. This update uses ember
`guidFor` to uniquely identify errors. Thanks for the help @rwjblue!

When creating a new promise, resolve it. Don't fork the promise

Tests expecting errors to bubble up, should bubble errors
  • Loading branch information
Mina Smart authored and rwjblue committed Dec 27, 2015
1 parent 2d622f1 commit ef7108a
Show file tree
Hide file tree
Showing 3 changed files with 72 additions and 2 deletions.
38 changes: 36 additions & 2 deletions packages/ember-routing/lib/system/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@ import {
stashParamNames,
calculateCacheKey
} from 'ember-routing/utils';
import { guidFor } from 'ember-metal/utils';
import RouterState from './router_state';
import { getOwner } from 'container/owner';

Expand All @@ -33,6 +34,7 @@ function K() { return this; }

var slice = [].slice;


/**
The `Ember.Router` class manages the application state and URLs. Refer to
the [routing guide](http://emberjs.com/guides/routing/) for documentation.
Expand Down Expand Up @@ -299,8 +301,7 @@ var EmberRouter = EmberObject.extend(Evented, {

_doURLTransition(routerJsMethod, url) {
var transition = this.router[routerJsMethod](url || '/');
didBeginTransition(transition, this);
return transition;
return didBeginTransition(transition, this);
},

transitionTo(...args) {
Expand Down Expand Up @@ -724,6 +725,24 @@ var EmberRouter = EmberObject.extend(Evented, {
run.cancel(this._slowTransitionTimer);
}
this._slowTransitionTimer = null;
},

_handledErrors: computed(function() {
return {};
}),

// These three helper functions are used to ensure errors aren't
// re-raised if they're handled in a route's error action.
_markErrorAsHandled(error) {
this.get('_handledErrors')[error] = true;
},

_isErrorHandled(error) {
return this.get('_handledErrors')[error];
},

_clearHandledError(error) {
delete this.get('_handledErrors')[error];
}
});

Expand Down Expand Up @@ -884,6 +903,11 @@ export function triggerEvent(handlerInfos, ignoreFailure, args) {
if (handler.actions[name].apply(handler, args) === true) {
eventWasHandled = true;
} else {
// Should only hit here if a non-bubbling error action is triggered on a route.
if (name === 'error') {
var errorId = guidFor(args[0]);
handler.router._markErrorAsHandled(errorId);
}
return;
}
}
Expand Down Expand Up @@ -1047,6 +1071,16 @@ function didBeginTransition(transition, router) {
router.set('currentState', routerState);
}
router.set('targetState', routerState);

return transition.catch(function(error) {
var errorId = guidFor(error);

if (router._isErrorHandled(errorId)) {
router._clearHandledError(errorId);
} else {
throw error;
}
});
}

function resemblesURL(str) {
Expand Down
2 changes: 2 additions & 0 deletions packages/ember/tests/routing/basic_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -1022,6 +1022,7 @@ QUnit.test('The Special page returning an error invokes SpecialRoute\'s error ha
actions: {
error(reason) {
equal(reason, 'Setup error', 'SpecialRoute#error received the error thrown from setup');
return true;
}
}
});
Expand Down Expand Up @@ -1059,6 +1060,7 @@ function testOverridableErrorHandler(handlersName) {
attrs[handlersName] = {
error(reason) {
equal(reason, 'Setup error', 'error was correctly passed to custom ApplicationRoute handler');
return true;
}
};

Expand Down
34 changes: 34 additions & 0 deletions packages/ember/tests/routing/substates_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -415,6 +415,40 @@ QUnit.test('Default error event moves into nested route', function() {
equal(appController.get('currentPath'), 'grandma.error', 'Initial route fully loaded');
});

QUnit.test('Error events that aren\'t bubbled don\t throw application assertions', function() {
expect(2);

templates['grandma'] = 'GRANDMA {{outlet}}';

Router.map(function() {
this.route('grandma', function() {
this.route('mom', { resetNamespace: true }, function() {
this.route('sally');
});
});
});

App.ApplicationController = Ember.Controller.extend();

App.MomSallyRoute = Ember.Route.extend({
model() {
step(1, 'MomSallyRoute#model');

return Ember.RSVP.reject({
msg: 'did it broke?'
});
},
actions: {
error(err) {
equal(err.msg, 'did it broke?');
return false;
}
}
});

bootApplication('/grandma/mom/sally');
});

QUnit.test('Setting a query param during a slow transition should work', function() {
var deferred = RSVP.defer();

Expand Down

0 comments on commit ef7108a

Please sign in to comment.