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.

Sign up for GitHub

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

Jump to bottom

Improve exception handling #368

Open
wants to merge 6 commits into
base: release-3.0
Choose a base branch
from
Open

Improve exception handling #368

wants to merge 6 commits into from

Conversation

jankapunkt
Copy link
Collaborator

@jankapunkt jankapunkt commented Apr 14, 2022 •
edited
Loading

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):

Blaze.setExceptionHandler(console.error)

Before:

meteor.js?hash=b9ec8cf25b6fc794ae6b825f30e06c3c35c50e7c:1064 Exception in template helper: Error: Must be attached
    at Blaze._DOMRange.DOMRange.firstNode (http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:462:29)
    at Blaze.View.view.templateInstance (http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:3346:39)
    at Function.Template.instance (http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:3755:60)
    at Function.getState (http://localhost:3030/packages/jkuester_template-states.js?hash=fe7d328b82b92a4f4a1cce203c7c6c0dbba59039:120:29)
    at Object.isCurrent (/imports/ui/layout/submenu/submenu.js:96:33)
    at http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:2899:16
    at http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:1582:16
    at http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:2952:66
    at Function.Template._withTemplateInstanceFunc (http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:3680:14)
    at http://localhost:3030/packages/blaze.js?hash=3ff2ed001c044f824549b078a8305bcad15811f0:2951:27

After:

exceptions.js:43 Exception in Template.submenu isCurrent: Error: Must be attached
    at Blaze._DOMRange.DOMRange.firstNode (domrange.js:191:11)
    at Blaze.View.view.templateInstance (template.js:176:39)
    at Function.Template.instance (template.js:560:17)
    at Function.getState (lib.js:59:29)
    at Object.isCurrent (submenu.js:59:33)
    at lookup.js:30:16
    at exceptions.js:67:16
    at lookup.js:72:53
    at Function.Template._withTemplateInstanceFunc (template.js:493:14)
    at lookup.js:71:27

Note, that due to this I was able to provide the PR #366

@StorytellerCZ StorytellerCZ added this to the Blaze 3.0 milestone Apr 14, 2022
@StorytellerCZ
Copy link
Collaborator

I'm thinking this for Blaze 3, but it could also go into 2.6.1 if it is needed in relation to #366.

@jankapunkt
Copy link
Collaborator Author

#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.

@StorytellerCZ StorytellerCZ changed the base branch from master to release-3.0 April 15, 2022 13:48
harryadel
harryadel suggested changes Jun 20, 2022
View reviewed changes
packages/blaze/exceptions.js Show resolved Hide resolved
packages/blaze/lookup.js
isKnownOldStyleHelper = true;
} else if (helper != null) {
return wrapHelper(bindDataContext(helper), tmplInstanceFunc);
throw new Meteor.Error("not-suported", "We removed support for old style templates");
Copy link
Contributor

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?

Copy link
Collaborator Author

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

packages/spacebars-tests/template_tests.js
@@ -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 () {
Copy link
Contributor

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!

Copy link
Collaborator Author

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

@jankapunkt jankapunkt mentioned this pull request Jul 13, 2022
Merged
@jankapunkt jankapunkt mentioned this pull request Jul 22, 2022
Open
@StorytellerCZ StorytellerCZ linked an issue Jul 28, 2022 that may be closed by this pull request
Improve Exception Reporting #291
Open
@jankapunkt jankapunkt self-assigned this Sep 4, 2022
@StorytellerCZ StorytellerCZ self-requested a review September 26, 2022 07:02
StorytellerCZ
StorytellerCZ requested changes Sep 26, 2022
View reviewed changes
Copy link
Collaborator

@StorytellerCZ StorytellerCZ left a 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.

@zodern zodern self-requested a review September 26, 2022 15:26
@zodern
Copy link
Collaborator

zodern commented Mar 29, 2023

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:

  1. Another package sets the exception handler first, but it is overridden by the agent
  2. The app sets its own exception handler, which prevents the agent from tracking the errors. The agent could work around this by wrapping Blaze.setExceptionHandler, but it would be preferable if it didn't need to, and it wouldn't fix the previous scenario.

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 Blaze._exceptionHandlers, or add a function to get how many exception handlers there are. Another option would be for the exception handler to indicate if Blaze should still call Meteor._debug, maybe by returning false.

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 Blaze._throwNextException instead of disabling the exception handler.

@StorytellerCZ StorytellerCZ linked an issue Mar 31, 2023 that may be closed by this pull request
Template exceptions are using console.log not console.error #314
Open
@jankapunkt jankapunkt mentioned this pull request Jan 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@harryadel harryadel harryadel requested changes

@StorytellerCZ StorytellerCZ StorytellerCZ requested changes

@zodern zodern Awaiting requested review from zodern

Assignees

@jankapunkt jankapunkt

Labels
None yet
Projects
None yet
Milestone
Blaze 3.0
Development

Successfully merging this pull request may close these issues.

Template exceptions are using console.log not console.error Improve Exception Reporting
4 participants
@jankapunkt @StorytellerCZ @zodern @harryadel