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

Add asyncNotifyException method (for one-level deep async stacktraces) #242

Closed

Conversation

jmshal
Copy link

@jmshal jmshal commented Jun 3, 2017

See #241 (comment) for more context & example.

@jmshal
Copy link
Author

jmshal commented Jun 3, 2017

Tested in Chrome, Safari and Firefox.

Copy link
Contributor

@bengourley bengourley left a comment

Choose a reason for hiding this comment

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

Hey @jmshal, this looks like a cool little hack until such time where we support long/async stack traces!

I'm on the fence about whether it is worthwhile adding as a feature, just because I think it will be confusing to explain/document, and also needs manual intervention to every async action to get set up.

Let me know if you're still keen to get this merged and if so, we could perhaps support it but not widely advertise/document it.

Couple of nit picks left as comments and also this would definitely need a test or two.

@@ -66,6 +78,36 @@
// ### Manual error notification (public methods)
//

// #### Bugsnag.autoNotifyException
Copy link
Contributor

Choose a reason for hiding this comment

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

this should say asyncNotifyException

try {
throw Error('');
} catch(err) {
stacktraceName = (
Copy link
Contributor

Choose a reason for hiding this comment

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

Some comment on which browsers use which error property for the stack trace would be nice.

@bengourley bengourley added the awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks. label Aug 29, 2017
@jmshal
Copy link
Author

jmshal commented Aug 30, 2017

Thanks @bengourley! I have my own doubts about this actually making it into the notifier. I feel at this point that it would be better if it was detailed within the docs (as a snippet), simply because of it's small size & very specific use-case. Although I do see this as a hack, I also feel like this is the best we're going to get, at least for a while - I spent a lot of time, and tried a bunch of different methods, but I settled on this as it was the least "involved". I usually like to stay optimistic about these things, but I don't think I've really seen this area of JS evolve much.

@bengourley
Copy link
Contributor

I feel at this point that it would be better if it was detailed within the docs (as a snippet)

I very much agree with this.

I'll close this PR but thanks your efforts in this area! Async tracing is definitely an area JS needs to catch up in… Node's async hooks are a start but it sounds like TC39 are (or will be) working on something for native JS, which means we're probably in for a rocky ride of incompatibility/competing standards in the future!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaiting feedback Awaiting a response from a customer. Will be automatically closed after approximately 2 weeks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants