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

Make Tracekit report correctly angularJS errors #238

Merged
merged 2 commits into from
Aug 11, 2014

Conversation

vperron
Copy link
Contributor

@vperron vperron commented Aug 6, 2014

Hi,

I'm pretty sure there are some issues in the way this commit is done (no version bump, etc), sorry in advance.
I noticed the stack frames corresponding to something like:

ReferenceError: xxx is not defined
    at new <anonymous> (http://127.0.0.1:3333/js/app.js:12354:19)
    at invoke (http://127.0.0.1:3333/js/vendor.js:29215:17)
    at Object.instantiate (http://127.0.0.1:3333/js/vendor.js:29226:23)
    at http://127.0.0.1:3333/js/vendor.js:32625:28

were silently skipped in Tracekit, because of the two words "new" and "" next to each other in the first frame.

This patch enhances (and in the end, simplifies) the regular expression that was used to decode the stack frames in Chrome. It was checking for 0-1 occurrences of [object], then some non-space chars, then 0-1 occurrences of [as xxx], but included everything in the same capture group.
I just capture what's before the first opening parenthesis.

Other methods (gecko-based detection) seem fine. Test has been added.

@mattrobenolt
Copy link
Contributor

Can you back out the changes made to the dist/ folder please?

@vperron
Copy link
Contributor Author

vperron commented Aug 6, 2014

Sure. I read that part of the contributing/ docs a little quick, sorry.

EDIT: and didn't get the notification for your lightning-speed answer.

@mattrobenolt
Copy link
Contributor

Thanks. :) Lemme review this later and I'll get back with you. At first glance, this seems reasonable though.

@robinwassen
Copy link
Contributor

I would advice against using this patch this way, the reason is that it is a local patch on the included TraceKit file instead of into the TraceKit project.

Send this patch to TraceKit instead and update the TraceKit version used by raven-js when the pull request has been merged.

@mattrobenolt
Copy link
Contributor

@robinwassen TraceKit is unmaintained, so it's purely vendorized into our project. We already have a ton of modifications to it.

mattrobenolt added a commit that referenced this pull request Aug 11, 2014
Make Tracekit report correctly angularJS errors
@mattrobenolt mattrobenolt merged commit b959b9e into getsentry:master Aug 11, 2014
@mattrobenolt
Copy link
Contributor

@vperron Thanks. :) 🍰

@robinwassen
Copy link
Contributor

@mattrobenolt Fair enough! Then it makes more sense :)

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.

3 participants