-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Add Raven.showReportDialog (experimental) #456
Conversation
30f1caa
to
3745ae1
Compare
3745ae1
to
26e7a83
Compare
var encode = encodeURIComponent; | ||
var qs = ''; | ||
qs += '?eventId=' + encode(lastEventId); | ||
qs += '&dsn=' + encode(this._dsn || ''); |
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.
dsn needs to be configurable via options
|
||
var lastEventId = options.eventId || this.lastEventId(); | ||
if (!lastEventId) { | ||
throw new RavenConfigError('Missing eventId'); |
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 probably shouldn't throw errors like this since they'd then get caught and in turn reported to Sentry. Thoughts?
Or we have a rule to ignore RavenConfigError's. This wasn't an issue before because RavenConfigError was only ever thrown before raven was even hooked up. Now we'll be throwing errors that will be caught and logged.
Not sure if bad or not, just pointing it out.
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.
Wouldn't you want them reported to Sentry? It means that you misconfigured Raven (for dialog purposes) and you might never otherwise find out.
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.
Note that if you have Raven configured to accept an error, you will have set a DSN, and will thus never hit Missing DSN
.
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.
I could see validity in the Missing DSN case, since that's before Raven is configured, but for missing event id... I'm not sure. I feel like we could just return a true/false instead for success/failure?
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.
I feel it should be reported.
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.
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.
I think it's really hard to get into an infinite loop situation here, because when calling showReportDialog
:
-
If you don't have a DSN, then Raven wasn't installed and the global error handler isn't present – the exception is thrown but nothing catches it and control flow ends.
-
If you don't have an
eventId
(or there was nolastEventId
), then Raven will throw an exception, and this will be caught by the global error handler. Now ifshowReportDialog
is called a second time, now there is alastEventId
and the function will no longer error.
Also, if you have an onerror
handler, and inside that handler you throw another error (e.g. RavenConfigError
), onerror
doesn't get called again in an infinite loop – it just stops there:
http://jsbin.com/qoqoviduru/1/edit?html,js
The user would have to use a setTimeout
call to leave the onerror
stack to reach this infinite loop scenario:
http://jsbin.com/moxulabigi/1/edit?html,js,output (warning, this will freeze your browser)
But I mean, if they did this, any kind of bug in their code would trigger the infinite/setTimeout scenario – not strictly this RavenConfigError
. So altogether, I think this scenario is really unlikely, and would be their fault anyways.
I'm going to move ahead with merging the branch. It's flagged experimental anyways.
Add Raven.showReportDialog (experimental)
Hey, first off: Thanks for the implementation. Looks like this is going to help us a lot. I know this is a beta-feature, but I thought I'd give it a shot anyways: Is there a way to localize the error-embed? It did not look like setting a locale via |
* ci: Add node 9 and 10 to travis builds * test: Fix listeners test for Node >= 9
No description provided.