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

[FEAT] Adds better logging for render errors #1169

Merged
merged 1 commit into from
Oct 13, 2020

Conversation

pzuraq
Copy link
Member

@pzuraq pzuraq commented Sep 30, 2020

Screen Shot 2020-09-29 at 6 23 58 PM

Copy link
Member

@rwjblue rwjblue left a comment

Choose a reason for hiding this comment

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

Chatted with @pzuraq offline, we should migrate this away from try/catch back to using try/finally. As a try/catch we somewhat fundamentally break developer ergonomics (when using "break on uncaught exceptions" it should stop in userland helper code, not our own internals).

The original need that caused this to be moved to try/catch was the deopts that were being triggered in that code that should only be ran when there is an actual exception. We can avoid that same issue by doing something akin to this instead:

let didError = true;
try {
  doSomething();
  didError = false;
} finally {
  if (didError) {
    console.log('error yo!')
  }
}

Doing this avoids the dev ergonomics issues and the deopt that was the original reason for moving to try/catch.


In this context, I think we should change this PR from assigning e.message to directly console.log'ing instead.

@rwjblue
Copy link
Member

rwjblue commented Oct 5, 2020

For those that are curious, here is a smallish codepen that demos the dev ergonomics issue I was referring to: https://codepen.io/rwjblue/full/PozYKqw.

When you run it, make sure to have DevTools open and "Pause on uncaught exceptions" set in the sources tab. The execution should pause in the someBadFunction implementation where throw new Error() is called, but it is actually pausing inside the catch block where we throw e.

@rwjblue
Copy link
Member

rwjblue commented Oct 13, 2020

#1172 addresses the removal of try/catch here, this will need a rebase after that lands

@rwjblue
Copy link
Member

rwjblue commented Oct 13, 2020

kk, ready for a rebase / rework now @pzuraq

@pzuraq pzuraq force-pushed the feat/better-render-error-logging branch from 46b6765 to 87e7488 Compare October 13, 2020 18:55
Adds an error log that provides context to the user when an error occurs during rendering.
@pzuraq pzuraq force-pushed the feat/better-render-error-logging branch from 87e7488 to 67df831 Compare October 13, 2020 19:06
@rwjblue rwjblue merged commit a872540 into master Oct 13, 2020
@rwjblue rwjblue deleted the feat/better-render-error-logging branch October 13, 2020 19:29
rwjblue added a commit to emberjs/ember.js that referenced this pull request Oct 17, 2020
Fixes two major issues:

* Ensure "pause on uncaught" exceptions pauses in correct location
  (glimmerjs/glimmer-vm#1172)
* Log template heirarchy to the console for any errors thrown during
  rendering (glimmerjs/glimmer-vm#1169)
kategengler pushed a commit to emberjs/ember.js that referenced this pull request Oct 19, 2020
Fixes two major issues:

* Ensure "pause on uncaught" exceptions pauses in correct location
  (glimmerjs/glimmer-vm#1172)
* Log template heirarchy to the console for any errors thrown during
  rendering (glimmerjs/glimmer-vm#1169)

(cherry picked from commit e8cfd08)
locks pushed a commit to emberjs/ember.js that referenced this pull request Oct 27, 2020
Fixes two major issues:

* Ensure "pause on uncaught" exceptions pauses in correct location
  (glimmerjs/glimmer-vm#1172)
* Log template heirarchy to the console for any errors thrown during
  rendering (glimmerjs/glimmer-vm#1169)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants