Add optional global error trap #94

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants
Collaborator

ef4 commented Jul 29, 2012

Sometimes window.onerror is just not good enough. For example, under
Chrome you lose your stack trace.

This exposes a new function on Q that lets you set a global error
handler, which is called whenever Q.end() would have otherwise generated an
unhandled exception. Example usage:

Q.onError(function(err){
  // Post your error to the server...
});

Note that if you still want the unhandled exception too (which is helpful in the console), you can just make your onError handle rethrow.

Add optional global error trap
Sometimes window.onerror is just not good enough. For example, under
Chrome you lose your stack trace.

This exposes a new function on Q that lets you set a global error
handler, for whenever Q.end() would have otherwise generated an
unhandled exception.
Collaborator

domenic commented Jul 29, 2012

I like this, and it certainly would make testing easier. Let's get some buy-in from @kriskowal before merging, but I'm sure it'll get in in some form.

The only question I have is whether there should be a single global error handler, which multiple users of Q can overwrite, or whether it should be an event-dispatching system where multiple calls to Q.onError result in multiple listeners being registered. I can see arguments either way.

Collaborator

ef4 commented Jul 29, 2012

Yeah, I considered supporting multiple listeners but couldn't come up with a practical reason I'd ever need that.

Owner

kriskowal commented Aug 23, 2012

I will entertain a Q.onerror = handler API for this, but not this patch as-is.

@kriskowal kriskowal closed this Aug 23, 2012

Collaborator

ef4 commented Aug 25, 2012

But doesn't defend(exports) rule out that kind of API?

As far as I can see (in Chrome), I'm unable to set properties on Q from outside the Q module itself.

@kriskowal kriskowal reopened this Aug 25, 2012

Owner

kriskowal commented Aug 25, 2012

@e4f, ah right. Will have to think more on it.

ef4 added a commit to ef4/q that referenced this pull request Oct 10, 2012

Don't mangle stack traces when an exception is seen twice
It's possible for the same exception to hit two different "end()"
points, causing it to pass through the stack trace formatter twice.

On the second pass, error.stack is already a string, and the formatter
mangles it by inserting newlines after every character.

The unit test included in this pull request depends on my earlier pull
request #94. That global error trap is pretty helpful for tests like
this.
Collaborator

domenic commented Oct 10, 2012

@kriskowal, I'd really like something like this. One more consideration: #111 calls for something like Q.stackJumpLimit = 5.

In light of these two, I see two options:

  1. Un-defend exports, perhaps selectively making all existing properties non-writable and non-configurable. This allows us to align with existing APIs a bit better, viz. window.onerror and Error.stackTraceLimit as settable properties.
  2. Introduce a Q.configureErrors({ uncaughtTrap, stackJumpLimit }) call.
Owner

kriskowal commented Oct 10, 2012

@domenic Let’s remove the defense of exports and add onerror and longStackTraceLimit.

@domenic domenic closed this in cdd8198 Oct 13, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment