-
-
Notifications
You must be signed in to change notification settings - Fork 115
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
Improve exception handling #368
base: release-3.0
Are you sure you want to change the base?
Improve exception handling #368
Conversation
I'm thinking this for Blaze 3, but it could also go into 2.6.1 if it is needed in relation to #366. |
#366 is good without this one. However, I think this one is important for further work towards 3.0 in order to get comprehensible errors during development. |
isKnownOldStyleHelper = true; | ||
} else if (helper != null) { | ||
return wrapHelper(bindDataContext(helper), tmplInstanceFunc); | ||
throw new Meteor.Error("not-suported", "We removed support for old style templates"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pardon me, but what does improving exceptions or allowing setting a custom handler function has to do with disabling old template helpers?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you read a bit further in the code it will basically raise a deprecated message. @StorytellerCZ should I restore this? We can drop old style helpers in 3.0 but then we should target 2.6.2 with this PR
@@ -3782,20 +3782,26 @@ Tinytest.add( | |||
|
|||
// Test old-style helper | |||
tmpl.foo = 'hello'; | |||
var div = renderToDiv(tmpl); | |||
test.equal(canonicalizeHtml(div.innerHTML), 'hello'); | |||
test.throws(function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to add tests for the new custom handler. Tests are only modified to check for the throw of /We removed support for old style templates/
but nothing to account for the new changes!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@harryadel thanks for pointing this out I will add tests to it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to address @harryadel comments.
Having a better way to handle exceptions than wrapping Meteor._debug will be very nice. However, from the perspective of someone who maintains an error tracking service for Meteor (Monti APM), I think the api will need some adjustments to be useful. One of the goals of montiapm:agent, and I assume other packages for error tracking is to instrument the app without affecting how the app works. The first problem is that only one exception handler can be set. This causes two potential problems for the agent:
An easy fix for this is to allow more than one exception handler. The second problem is setting an exception handler causes it to no longer call Meteor._debug. Most of the time that is desired. However, some apps wrap Meteor._debug to do something with the errors, and the agent adding an exception handler would break that. If there was some way for the agent to know how many exception handlers there were, it could call Meteor._debug itself if there is only one. A simple way to support this is adding an underscore prefixed property to Blaze that has the exception handlers, such as I'm also a wondering if https://github.com/meteor/blaze/pull/368/files#diff-d67f5296473b382dff76b06ab16ec9eabd7e690615a7c3d20ad47b6c57bc4b09R69 would prevent packages from testing their exception handlers. Blaze's tests could probably use |
Based on the issue from @lynchem #291 I updated to the code accordingly.
Additionally I used the chance to add a custom exception handler, which I think is crucuial to get decent debug traces in dev mode (related #314):
Before:
After:
Note, that due to this I was able to provide the PR #366