Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[BUGFIX release] Do not throw uncaught errors mid-transition. #5166

Merged
merged 1 commit into from
Jul 15, 2014

Conversation

rwjblue
Copy link
Member

@rwjblue rwjblue commented Jul 15, 2014

The primary intent of this was to rethrow the error when it wasn't a known (ignorable) type, and the default RSVP error handler would ensure that it was printed to the console.

This worked wonderfully, except that it caused an error to occur (an Ember.assert throws an error) during the rendering of the error route (or template).

This changes from throwing the error to just logging it, but the userland result is the same: useful console error messages showing when bad things happen in route hook promises.

Closes #5148.

1.6.1 will need to be released after this has been merged.

@rwjblue
Copy link
Member Author

rwjblue commented Jul 15, 2014

The correct thing in general would be to throw the error (which was being done before this), but that leads to an issue when transitioning into the error route because the RSVP.onerrorDefault calls Ember.assert during the run-loop with the new transition. This causes that transition to be completely ignored (as the Ember.assert within RSVP.onerrorDefault just throws an error and stops processing).

@rwjblue
Copy link
Member Author

rwjblue commented Jul 15, 2014

@machty - Can you review and +1/-1 ?

@rwjblue
Copy link
Member Author

rwjblue commented Jul 15, 2014

Assuming all is well, this will be pulled into the stable branch, and released as 1.6.1.

@@ -833,7 +833,7 @@ function listenForTransitionErrors(transition) {
} else if (error.name === 'TransitionAborted') {
// just ignore TransitionAborted here
} else {
throw error;
Ember.Logger.error(error.stack);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this behave on older browsers that don't have Error#stack?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not provide meaningful information (error.stack is undefined), but it does not throw an error.

Just updated to share the error logging logic from the default error action handler.

The primary intent of this was to rethrow the error when it wasn't a
known (ignorable) type, and the default RSVP error handler would ensure
that it was printed to the console.

This worked wonderfully, except that it caused an error to occur (an
`Ember.assert` throws an error) during the rendering of the `error`
route (or template).

This changes from `throw`ing the error to just logging it, but the
userland result is the same: useful console error messages showing when
bad things happen in route hook promises.
@machty
Copy link
Contributor

machty commented Jul 15, 2014

LGTM

rwjblue added a commit that referenced this pull request Jul 15, 2014
[BUGFIX release] Do not throw uncaught errors mid-transition.
@rwjblue rwjblue merged commit 1b565b3 into emberjs:master Jul 15, 2014
@rwjblue rwjblue deleted the do-not-throw-in-error-action branch July 15, 2014 13:41
@chriseldredge
Copy link

Can anyone comment on if this is related?

http://emberjs.jsbin.com/degag/3/edit

Case 4 (throwing a string) transitions to the error route but the model is null, using Ember 1.6.1.

@machty
Copy link
Contributor

machty commented Jul 19, 2014

I think it's because it internally converted into transitionTo('error', "your string") and strings get treated as params rather than the object passed to a route. It probably shouldn't behave that way internally, but then again you should probably be throwing a new Error("your string")

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: error template does not render
4 participants