Skip to content

Fix parameters to TraceKit.rethrow #315

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
Closed
wants to merge 1 commit into from
Closed

Fix parameters to TraceKit.rethrow #315

wants to merge 1 commit into from

Conversation

tgr
Copy link

@tgr tgr commented Feb 8, 2015

  • Do not pass options array. TraceKit expects a boolean so it
    doesn't do anything useful.
  • Pass false to disable rethrowing errors. Raven would catch them
    again so it is pointless, and it can pollute the stack trace
    (since TraceKit doesn't extract information from it unnecessarily).
  • Remove the try-catch that was made unnecessary by the change.

This should fix #300.

@mattrobenolt
Copy link
Contributor

wtf I remember doing this specifically for a reason... and now it doesn't make sense. Going to look into this. Maybe I was just crazy when I did that or something, but at first glance this seems legit.

@tgr
Copy link
Author

tgr commented Feb 12, 2015

It looks like the intent was to pass the options object to handleStackInfo. I don't think it works though; it seems extra options just get lost.

@tgr
Fix parameters to TraceKit.rethrow
177b845 
* Remove exception rethrowing from TraceKit.report. Raven would catch
  them again so it is pointless, and it can pollute the stack trace
  (since TraceKit doesn't extract information from it immediately,
  only after the rethrow has happened, adding another call to the
  top of the stack).
* Update jsdoc TraceKit.repoer to make clear that additional arguments
  are passed to the handlers.
* Remove the try-catch in Raven.captureException that was made
  unnecessary by the change.

Fixes getsentry#300.
@tgr
Copy link
Author

tgr commented Feb 24, 2015

The options are passed via lastArgs; I didn't notice that.

Updated the patch to keep the options argument but remove unnecessary exception handling. TraceKit.report was only used in two places: in Raven.captureException, where the exception was caught anyway, and in TraceKit.wrap, where it was rethrown anyway, so this change should make no difference (apart from fixing #300).

@tgr
Copy link
Author

tgr commented Feb 24, 2015

The patch broke a bunch of tests; I'll rewrite it.

Is it intentional that the tests are not integrated into GitHub via Travis or something similar?

@mattrobenolt
Copy link
Contributor

The tests do run on Travis. For some reason, it doesn't appear to be running for pull requests though...

@dcramer dcramer mentioned this pull request Jul 25, 2015
Closed
@mattrobenolt
Copy link
Contributor

This should be irrelevant now since we don't call TraceKit.rethrow anymore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Rethrow logic adds an extra stacktrace line which causes culprit to be set to raven.js
2 participants
@tgr @mattrobenolt