Deferred: Give better stack diagnostics on exceptions #3113
Conversation
Ref jquerygh-2736 The exception stack has the name of the immediately outer function where the exception occurred, which can be very handy for tracing errors. Since we already have the exception object we might as well use it.
|
By analyzing the blame information on this pull request, we identified @mgol, @gibson042 and @markelog to be potential reviewers |
| window.console.warn = oldWarn; | ||
| done(); | ||
| } ); | ||
|
|
||
| defer.resolve(); | ||
| } ); | ||
|
|
||
| // Test fails in IE9 but is skipped there because console is not active |
There was a problem hiding this comment.
Does it fail with DevTools open? If so, why?
There was a problem hiding this comment.
IE9 doesn't have error.stack.
There was a problem hiding this comment.
Could you add a check for (new Error()).stack here so that the tests work even with IE9 DevTools enabled?
There was a problem hiding this comment.
Oh wait, I just remembered:
Different browsers set this value at different times. For example, Firefox sets it when creating an Error object, while PhantomJS sets it only when throwing the Error, and MSDN docs also seem to match the PhantomJS implementation. -- https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/Stack
So the test would need to be more elaborate. I think the current check is less likely to skip by accident at least? Everyone but IE9 has a console.
There was a problem hiding this comment.
Yeah, I thought it'd be simpler. 😅 It's OK then.
|
So, the idea is that this stack trace is nowhere near that useful than the original one which would be passed by your jquery-deferred-reporter plugin but it still provides some useful info? |
| @@ -11,10 +11,10 @@ var rerrorNames = /^(Eval|Internal|Range|Reference|Syntax|Type|URI)Error$/; | |||
|
|
|||
| jQuery.Deferred.exceptionHook = function( error, stack ) { | |||
|
|
|||
| // Support: IE 8 - 9 only | |||
There was a problem hiding this comment.
Let's not change this comment. We've decided some time ago to not remove info about workarounds for now non-supported browsers as long as we still need the workaround for other, supported ones.
There was a problem hiding this comment.
OK, for some reason I thought we were doing the opposite.
|
I don't see the name in the stack trace for the handler named It shows |
|
Firefox, on the other hand, show just the first error (Chrome shows both, with stack traces with your patch): with no stack trace. I'm doing the testing with this code: var defer = jQuery.Deferred();
jQuery.when(
defer.then( function() {
jQuery.barf();
} ).then( null, jQuery.noop ),
defer.then( function() {
throw new Error( "Make me a sandwich" );
} ).then( null, jQuery.noop )
).then( function barf( ) {
jQuery.thisDiesToo();
} );
defer.resolve(); |
|
Interesting; even with: jQuery.Deferred.exceptionHook = function( error, stack ) {
console.log("jQuery.Deferred exception: " + error.message, error.name, error.stack, stack );
};I see 3 errors in Chrome with this test but only one error message in Firefox and the Firefox one doesn't seem to come from jQuery as it doesn't begin with |
|
It all works fine for me in Firefox beta or newer so perhaps only the current version has problems. |
The exceptionHook with stack test also needed to see all args, since the arg order was changed.
|
|
||
| var done = assert.async(), | ||
| defer = jQuery.Deferred(), | ||
| oldWarn = window.console.warn; | ||
|
|
||
| window.console.warn = function( msg ) { | ||
| window.console.warn = function() { | ||
|
|
||
| // Support: Chrome <=41 only |
There was a problem hiding this comment.
We've dropped Yandex now, but I guess this is still needed for Android 5.0?
There was a problem hiding this comment.
Yeah, although technically that shouldn't matter as Android 5.0 has both auto-updatable browser (Chrome) & WebView. It's just that BrowserStack doesn't update Chrome in Android images (I'm not sure if that's even possible in emulators). The solution would be to move to Android 6.0 and drop 5.0 as we're really interested in testing Chrome for Android, not specific Android versions (it used to be different as Android Browser versions were tied to Android versions as is still the case on iOS) but last time I checked local testing didn't work on Android 6.0 on BrowserStack. I'll need to re-check, if it worked now we'd be able to drop these workarounds.
|
Closed by 07c11c0 |
Summary
Ref gh-2736
The exception stack has the name of the immediately outer function where the
exception occurred, which can be very handy for tracing errors. Since we already
have the exception object we might as well use it.
I was debugging some code that had errors inside a document ready function and this would have been really useful to have.
Checklist
Mark an
[x]for completed items, if you're not sure leave them unchecked and we can assist.If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.comThanks! Bots and humans will be around shortly to check it out.