Skip to content

feat: Support unhandled rejections by default #1242

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

Merged
merged 1 commit into from
Mar 2, 2018

Conversation

kamilogorek
Copy link
Contributor

Fixes: #1070

There are more and more clients asking for this feature.

@kamilogorek kamilogorek requested a review from a team March 1, 2018 13:57
@MaxBittker MaxBittker self-assigned this Mar 1, 2018
Copy link
Contributor

@MaxBittker MaxBittker left a comment

Choose a reason for hiding this comment

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

🏅 🏅 🏅 🏅

awesome, I think this is a great change.
code, tests and docs look good.

we should be clear to specify in the release that when people update, this version may start catching errors that were silently occuring in the background.

docs/usage.rst Outdated
manually hook into such an event handler and call ``Raven.captureException`` or ``Raven.captureMessage``
directly.
Most Promise libraries however, have a global hook for capturing unhandled errors. You will need to disable default behaviour
by setting ``captureUnhandledRejections`` option to ``false`` and manually hook into such an event handler
Copy link
Contributor

@MaxBittker MaxBittker Mar 1, 2018

Choose a reason for hiding this comment

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

You will need to disable default behaviour

is there harm in leaving it?

Copy link
Contributor Author

@kamilogorek kamilogorek Mar 2, 2018

Choose a reason for hiding this comment

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

Not really, but it'll silence the error unless you set Raven.debug = true; to see the log.
Changed to "you may want to".

docs/usage.rst Outdated
window.onunhandledrejection = function(evt) {
Raven.captureException(evt.reason);
};
In this case, you don't need to do anything, we already got you covered.
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe mention captureUnhandledRejections here?

Copy link
Contributor

Choose a reason for hiding this comment

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

small nit, i would say "we've" not "we"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated. And added this note.

@kamilogorek kamilogorek force-pushed the unhandled-rejections branch from 3d4ee5b to 0cd9086 Compare March 2, 2018 10:34
@kamilogorek kamilogorek merged commit 0516d9e into master Mar 2, 2018
@kamilogorek kamilogorek deleted the unhandled-rejections branch March 2, 2018 10:34
@ohana54
Copy link

ohana54 commented Mar 2, 2018

@kamilogorek @MaxBittker hey, thanks for this feature!
Do you think it's ok to turn it on by default in a minor version?
We're capturing unhandled rejections manually in our app (like other apps I guess), does this mean the exceptions will be captured twice?

@ohana54
Copy link

ohana54 commented Mar 2, 2018

Related: #1243

@francois-codes
Copy link

This patch breaks our build too as window is not defined in our env :/

I think there are many users relying on this Library in environments where window is not defined - there should be a check for that, or we should be able to turn it off in Raven.config

@ohana54
Copy link

ohana54 commented Mar 2, 2018

@f-roland it's possible to turn it off, but it's still not enough since in the unregister function it doesn't check if you turned it off and always accesses the window.

@kamilogorek
Copy link
Contributor Author

@ohana54 @f-roland fixed and released patched version. I call it dontdeployonfriday thing :(

@ohana54
Copy link

ohana54 commented Mar 2, 2018

:) No worries, thanks for the quick fix!!

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.

4 participants