Deferred: Warn on exceptions that are likely programming errors #2737

Closed
wants to merge 7 commits into
from

Conversation

Projects
None yet
10 participants
@dmethvin
Member

dmethvin commented Nov 23, 2015

Fixes gh-2736

Still a WIP with some details to be worked out. I'm not really happy with the stack trace that results, it's only showing the immediate prior call. The gist, however, is that nearly all exceptions of these types are programming mistakes and should be reported ASAP. If devs don't like being warned about them they can set jQuery.Deferred.exceptionHook = null and not be bothered, or define a custom hook that filters out the noisy false positives.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Nov 23, 2015

Member

Yea, I wonder if error.stack is a more appropriate stack trace than console.trace().

Member

timmywil commented Nov 23, 2015

Yea, I wonder if error.stack is a more appropriate stack trace than console.trace().

src/deferred.js
+ // These usually indicate a programmer mistake during development,
+ // warn about them ASAP rather than swallowing them by default.
+
+ var rerrorNames = /^(Eval|Internal|Range|Reference|Syntax|Type|URI)Error$/;

This comment has been minimized.

@gibson042

gibson042 Nov 23, 2015

Member

I still don't see this as library code. The whole mess is more appropriate for userland.

@gibson042

gibson042 Nov 23, 2015

Member

I still don't see this as library code. The whole mess is more appropriate for userland.

This comment has been minimized.

@dmethvin

dmethvin Nov 23, 2015

Member

I'd agree if the developer ergonomics weren't so horrific. If the default is to swallow but we just add the ability for a hook, people won't know enough to add a hook. Like I said in the meeting, devs need a solution really bad, so here is a really bad solution.

@dmethvin

dmethvin Nov 23, 2015

Member

I'd agree if the developer ergonomics weren't so horrific. If the default is to swallow but we just add the ability for a hook, people won't know enough to add a hook. Like I said in the meeting, devs need a solution really bad, so here is a really bad solution.

This comment has been minimized.

@staabm

staabm Nov 25, 2015

Contributor

What about pluging in this "error-handler" via a separate AMD Module...? That way one can build jQuery without it (for production) and with it for development etc

@staabm

staabm Nov 25, 2015

Contributor

What about pluging in this "error-handler" via a separate AMD Module...? That way one can build jQuery without it (for production) and with it for development etc

This comment has been minimized.

@timmywil

timmywil Nov 30, 2015

Member

@staabm not a bad idea

@timmywil

timmywil Nov 30, 2015

Member

@staabm not a bad idea

src/deferred.js
@@ -157,6 +157,10 @@ jQuery.extend( {
mightThrow();
} catch ( e ) {
+ if ( jQuery.Deferred.exceptionHook ) {
+ jQuery.Deferred.exceptionHook( e );

This comment has been minimized.

@gibson042

gibson042 Nov 23, 2015

Member

Calling it exceptionHook doesn't change the fact that it is invoked for every Promise-like rejection.

@gibson042

gibson042 Nov 23, 2015

Member

Calling it exceptionHook doesn't change the fact that it is invoked for every Promise-like rejection.

This comment has been minimized.

@gibson042

gibson042 Nov 23, 2015

Member

If we're comfortable with the above, though, a hook here seems like an interesting approach. Since only the exception is passed, there's no access to a relevant thenable and therefore no value for anything other than debugging.

@gibson042

gibson042 Nov 23, 2015

Member

If we're comfortable with the above, though, a hook here seems like an interesting approach. Since only the exception is passed, there's no access to a relevant thenable and therefore no value for anything other than debugging.

This comment has been minimized.

@dmethvin

dmethvin Nov 23, 2015

Member

Agreed, perhaps rejectionHook instead?

@dmethvin

dmethvin Nov 23, 2015

Member

Agreed, perhaps rejectionHook instead?

src/deferred.js
@@ -359,5 +363,22 @@ jQuery.extend( {
}
} );
+if ( window.console && window.console.warn ) {
+
+ // These usually indicate a programmer mistake during development,

This comment has been minimized.

@gibson042

gibson042 Nov 23, 2015

Member

How true is this? An obvious and common example is JSON.parse throwing SyntaxError, but there are also cases like Date.prototype.toISOString throwing RangeError. Promise examples often encourage developers to optimize for non-exceptional execution, and this could get noisy when they do something like:

get( … ).then( assertSuccess ).then( JSON.parse ).then( handleResponse )
    .catch( handleError )
    .then( finish );
@gibson042

gibson042 Nov 23, 2015

Member

How true is this? An obvious and common example is JSON.parse throwing SyntaxError, but there are also cases like Date.prototype.toISOString throwing RangeError. Promise examples often encourage developers to optimize for non-exceptional execution, and this could get noisy when they do something like:

get( … ).then( assertSuccess ).then( JSON.parse ).then( handleResponse )
    .catch( handleError )
    .then( finish );

This comment has been minimized.

@dmethvin

dmethvin Nov 23, 2015

Member

You've probably caught me editorializing a bit there, I admit. 😄 I think exceptions (vs programmatic rejections) are much more likely to be programming errors but I don't have data to back up that assertion. For that case it would seem really unusual for a server to deliver malformed JSON, but if it did occasionally do so the worst we've done is said that on the console.

@dmethvin

dmethvin Nov 23, 2015

Member

You've probably caught me editorializing a bit there, I admit. 😄 I think exceptions (vs programmatic rejections) are much more likely to be programming errors but I don't have data to back up that assertion. For that case it would seem really unusual for a server to deliver malformed JSON, but if it did occasionally do so the worst we've done is said that on the console.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Nov 23, 2015

Member

Is my understanding correct that this:

  • Warns on all exceptions, not just unhandled exceptions
  • Only listens for thrown exceptions and not explicitly rejected promises?
Member

scottgonzalez commented Nov 23, 2015

Is my understanding correct that this:

  • Warns on all exceptions, not just unhandled exceptions
  • Only listens for thrown exceptions and not explicitly rejected promises?
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 23, 2015

Member

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

Only listens for thrown exceptions and not explicitly rejected promises?

Yes, non-exception rejections (not an exception or explicitly thrown error) don't go through this path. jQuery Deferred hasn't ever warned on unhandled explicit rejections (such as an ajax 404) so I don't feel bad about excluding them.

Member

dmethvin commented Nov 23, 2015

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

Only listens for thrown exceptions and not explicitly rejected promises?

Yes, non-exception rejections (not an exception or explicitly thrown error) don't go through this path. jQuery Deferred hasn't ever warned on unhandled explicit rejections (such as an ajax 404) so I don't feel bad about excluding them.

@scottgonzalez

This comment has been minimized.

Show comment
Hide comment
@scottgonzalez

scottgonzalez Nov 24, 2015

Member

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

Member

scottgonzalez commented Nov 24, 2015

Warns on all exceptions, not just unhandled exceptions

No, it only warns on the derivative Error types listed there.

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Nov 24, 2015

Member

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

Oh, sorry, yes. Exceptions like SyntaxError and explicitly thrown errors will pass through here.

Member

dmethvin commented Nov 24, 2015

What I mean is that this isn't a hook for unhandled exceptions. It's a hook for all exceptions.

Oh, sorry, yes. Exceptions like SyntaxError and explicitly thrown errors will pass through here.

src/deferred.js
+ if ( error && rerrorNames.test( error.name ) ) {
+ window.console.warn( "jQuery.Deferred exception: " + error.message );
+ if ( window.console.trace ) {
+ window.console.trace();

This comment has been minimized.

@timmywil

timmywil Nov 25, 2015

Member

Could we do error.stack here?

@timmywil

timmywil Nov 25, 2015

Member

Could we do error.stack here?

src/deferred.js
+
+ jQuery.Deferred.exceptionHook = function( error ) {
+ if ( error && rerrorNames.test( error.name ) ) {
+ window.console.warn( "jQuery.Deferred exception: " + error.message );

This comment has been minimized.

@mgol

mgol Nov 25, 2015

Member

What if console doesn't exist?

@mgol

mgol Nov 25, 2015

Member

What if console doesn't exist?

This comment has been minimized.

@mgol

mgol Nov 25, 2015

Member

Specifically, that's the case in IE 9 without open Dev Tools.

@mgol

mgol Nov 25, 2015

Member

Specifically, that's the case in IE 9 without open Dev Tools.

This comment has been minimized.

@scottgonzalez

scottgonzalez Nov 25, 2015

Member

See L366.

@scottgonzalez

scottgonzalez Nov 25, 2015

Member

See L366.

This comment has been minimized.

@dmethvin

dmethvin Nov 25, 2015

Member

There's a guard above for that case but you're right, it needs to be in the function itself to prevent a really nasty Heisenbug where it would crash, but only if you didn't have Dev Tools open. 😈

In environments without the console my intent was that you just wouldn't get any output, which is basically no worse than the current default. I think escalating to something that is still there like window.alert would be too intrusive, but someone could add that as a hook if they were trying to debug an IE9-specific problem.

@dmethvin

dmethvin Nov 25, 2015

Member

There's a guard above for that case but you're right, it needs to be in the function itself to prevent a really nasty Heisenbug where it would crash, but only if you didn't have Dev Tools open. 😈

In environments without the console my intent was that you just wouldn't get any output, which is basically no worse than the current default. I think escalating to something that is still there like window.alert would be too intrusive, but someone could add that as a hook if they were trying to debug an IE9-specific problem.

This comment has been minimized.

@mgol

mgol Nov 25, 2015

Member

Ah, OK. I think if you open the site with DevTools open and then close it the console object is still there but I'd have to check.

@mgol

mgol Nov 25, 2015

Member

Ah, OK. I think if you open the site with DevTools open and then close it the console object is still there but I'd have to check.

test/unit/deferred.js
+ defer = jQuery.Deferred(),
+ oldWarn = console.warn;
+
+ console.warn = function( s ) {

This comment has been minimized.

@markelog

markelog Nov 28, 2015

Member

We have sinon in deps btw

@markelog

markelog Nov 28, 2015

Member

We have sinon in deps btw

This comment has been minimized.

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 1, 2015

Member

Pushed a few changes here. I tried sinon.spy but was getting an error from its guts and didn't really need it anyway.

Member

dmethvin commented Dec 1, 2015

Pushed a few changes here. I tried sinon.spy but was getting an error from its guts and didn't really need it anyway.

src/deferred/exceptionHook.js
+ var warn = window.console && window.console.warn;
+
+ if ( error && rerrorNames.test( error.name ) ) {
+ warn( "jQuery.Deferred exception: " + error.message );

This comment has been minimized.

@dmethvin

dmethvin Dec 1, 2015

Member

Because the Deferred is resolved through a setTimeout it loses the stack. We could use some chicanery to save a stack beforehand but that would impact size and performance for the comparatively rare case where an exception occurs. So I pulled out the stack trace entirely since it only refers to things inside jQuery itself.

@dmethvin

dmethvin Dec 1, 2015

Member

Because the Deferred is resolved through a setTimeout it loses the stack. We could use some chicanery to save a stack beforehand but that would impact size and performance for the comparatively rare case where an exception occurs. So I pulled out the stack trace entirely since it only refers to things inside jQuery itself.

This comment has been minimized.

@dcherman

dcherman Dec 14, 2015

Contributor

Does simply accessing the stack property of an Error have a perf impact? You suggested this in the meeting, but you could also provide jquery.longStackTraces.js as a plugin or something to get functionality similar to Bluebird as long as the appropriate hooks exist in core

@dcherman

dcherman Dec 14, 2015

Contributor

Does simply accessing the stack property of an Error have a perf impact? You suggested this in the meeting, but you could also provide jquery.longStackTraces.js as a plugin or something to get functionality similar to Bluebird as long as the appropriate hooks exist in core

This comment has been minimized.

@dmethvin

dmethvin Dec 14, 2015

Member

Adding a hook so that a plugin could provide better stack traces seems like a good way to go, I'm looking into it.

@dmethvin

dmethvin Dec 14, 2015

Member

Adding a hook so that a plugin could provide better stack traces seems like a good way to go, I'm looking into it.

+
+// These usually indicate a programmer mistake during development,
+// warn about them ASAP rather than swallowing them by default.
+var rerrorNames = /^(Eval|Internal|Range|Reference|Syntax|Type|URI)Error$/;

This comment has been minimized.

@dmethvin

dmethvin Dec 1, 2015

Member

The error.stack property isn't standardized but it looks like Chrome and Safari do add one for throw new Error(). Using that would make this a much noisier hook so I stayed with identifying the ES-defined exception names.

@dmethvin

dmethvin Dec 1, 2015

Member

The error.stack property isn't standardized but it looks like Chrome and Safari do add one for throw new Error(). Using that would make this a much noisier hook so I stayed with identifying the ES-defined exception names.

+ defer = jQuery.Deferred(),
+ oldWarn = window.console.warn;
+
+ window.console.warn = function( msg ) {

This comment has been minimized.

@markelog

markelog Dec 2, 2015

Member

Did you try sinon for this?

@markelog

markelog Dec 2, 2015

Member

Did you try sinon for this?

This comment has been minimized.

@dmethvin

dmethvin Dec 5, 2015

Member

I tried sinon spy but they didn't offer that much value for such a simple case and I kept getting a strange error. Basically I gave up because I felt I was wasting time.

@dmethvin

dmethvin Dec 5, 2015

Member

I tried sinon spy but they didn't offer that much value for such a simple case and I kept getting a strange error. Basically I gave up because I felt I was wasting time.

This comment has been minimized.

@markelog

markelog Dec 7, 2015

Member

Hm, weird, sinon should be useful in cases like this, especially that it provides integration with assertion libraries like standart assert module and chai. See - http://chaijs.com/plugins/sinon-chai

@leobalter is there integration way with qunit?

@markelog

markelog Dec 7, 2015

Member

Hm, weird, sinon should be useful in cases like this, especially that it provides integration with assertion libraries like standart assert module and chai. See - http://chaijs.com/plugins/sinon-chai

@leobalter is there integration way with qunit?

This comment has been minimized.

@leobalter

leobalter Dec 7, 2015

Member

sinon spy but they didn't offer that much value for such a simple case

Agreed. A few years ago I tried it and saw a bloat library polluting the global scope and QUnit's api. At that moment I've made DexterJS with a very simple spy/stub API alternative but it's not maintained since I didn't have many contributions and stopped using mocks.

@leobalter

leobalter Dec 7, 2015

Member

sinon spy but they didn't offer that much value for such a simple case

Agreed. A few years ago I tried it and saw a bloat library polluting the global scope and QUnit's api. At that moment I've made DexterJS with a very simple spy/stub API alternative but it's not maintained since I didn't have many contributions and stopped using mocks.

This comment has been minimized.

@markelog

markelog Dec 7, 2015

Member

Agreed.

Yeah, but if we would use it across all the code base i.e. everywhere, it seems as matter of consistency and usability for other not that simple cases like this one.

A few years ago I tried it and saw a bloat library polluting the global scope and QUnit's api.

Is it possible to provide an extension to qunit as separate package? Since our extensions already complicate our test code, i fear core testsuite might became more vague if we would extend it even more.

@markelog

markelog Dec 7, 2015

Member

Agreed.

Yeah, but if we would use it across all the code base i.e. everywhere, it seems as matter of consistency and usability for other not that simple cases like this one.

A few years ago I tried it and saw a bloat library polluting the global scope and QUnit's api.

Is it possible to provide an extension to qunit as separate package? Since our extensions already complicate our test code, i fear core testsuite might became more vague if we would extend it even more.

This comment has been minimized.

@leobalter

leobalter Dec 7, 2015

Member

The problem with Dexter was the ajax mock. Maybe re-releasing it with only the spy and stub API will serve most of what many libraries want. It does not need to rely on any test framework.

I'll also check if I should even split the spy and stub features in different modules.

Anyway, you can solve this current PR without any mock/spy function as I mentioned in https://github.com/jquery/jquery/pull/2737/files#r46825810

@leobalter

leobalter Dec 7, 2015

Member

The problem with Dexter was the ajax mock. Maybe re-releasing it with only the spy and stub API will serve most of what many libraries want. It does not need to rely on any test framework.

I'll also check if I should even split the spy and stub features in different modules.

Anyway, you can solve this current PR without any mock/spy function as I mentioned in https://github.com/jquery/jquery/pull/2737/files#r46825810

This comment has been minimized.

@markelog

markelog Dec 7, 2015

Member

Well, i would like qunit api to be able to integrate with sinon, like it can be done with chai, assert or jasmine libs.

@markelog

markelog Dec 7, 2015

Member

Well, i would like qunit api to be able to integrate with sinon, like it can be done with chai, assert or jasmine libs.

This comment has been minimized.

@leobalter

leobalter Dec 7, 2015

Member

AFAIK, sinon already connects to QUnit through sinon-qunit (See: http://sinonjs.org/qunit/).

You may also call sinon api directly, which seems better than having a module messing QUnit's own API.

@leobalter

leobalter Dec 7, 2015

Member

AFAIK, sinon already connects to QUnit through sinon-qunit (See: http://sinonjs.org/qunit/).

You may also call sinon api directly, which seems better than having a module messing QUnit's own API.

This comment has been minimized.

@markelog

markelog Dec 7, 2015

Member

Of course it is :-), should have search for it, thank you Leo!

@markelog

markelog Dec 7, 2015

Member

Of course it is :-), should have search for it, thank you Leo!

@@ -525,6 +525,34 @@ QUnit.test( "jQuery.Deferred.then - spec compatibility", function( assert ) {
} catch ( _ ) {}
} );
+QUnit[ window.console ? "test" : "skip" ]( "jQuery.Deferred.exceptionHook", function( assert ) {

This comment has been minimized.

@markelog

markelog Dec 2, 2015

Member

I think we can extend QUnit API for this kind of pattern

@markelog

markelog Dec 2, 2015

Member

I think we can extend QUnit API for this kind of pattern

This comment has been minimized.

@dmethvin

dmethvin Dec 5, 2015

Member

Something like QUnit.testIf(condition, function) would be useful I agree.

@dmethvin

dmethvin Dec 5, 2015

Member

Something like QUnit.testIf(condition, function) would be useful I agree.

This comment has been minimized.

@gibson042

gibson042 Dec 5, 2015

Member

QUnit.testIf has already been contemplated multiple times, and is easy to add manually. Let's try it out in jQuery core. 👍

@gibson042

gibson042 Dec 5, 2015

Member

QUnit.testIf has already been contemplated multiple times, and is easy to add manually. Let's try it out in jQuery core. 👍

This comment has been minimized.

This comment has been minimized.

@leobalter

leobalter Dec 7, 2015

Member

It's not the first time we have this API suggestion on QUnit, I'll discuss it on QUnit issues.

@leobalter

leobalter Dec 7, 2015

Member

It's not the first time we have this API suggestion on QUnit, I'll discuss it on QUnit issues.

This comment has been minimized.

@dmethvin

dmethvin Dec 20, 2015

Member

If you want to come up for a spec for QUnit.testIf() we can polyfill it until we upgrade, or we could upgrade QUnit and make the change then. Let's do that change outside this ticket though.

@dmethvin

dmethvin Dec 20, 2015

Member

If you want to come up for a spec for QUnit.testIf() we can polyfill it until we upgrade, or we could upgrade QUnit and make the change then. Let's do that change outside this ticket though.

+ jQuery.when(
+ defer.then( function() {
+ // Should get an error
+ jQuery.barf();

This comment has been minimized.

@leobalter

leobalter Dec 7, 2015

Member

Instead of calling jQuery.barf, we could make a direct call to the assertion here, the assert.expect is enough.

@leobalter

leobalter Dec 7, 2015

Member

Instead of calling jQuery.barf, we could make a direct call to the assertion here, the assert.expect is enough.

src/deferred/exceptionHook.js
+
+ // Support: IE9
+ // Console exists when dev tools are open, which can happen at any time
+ var warn = window.console && window.console.warn;

This comment has been minimized.

@dcherman

dcherman Dec 14, 2015

Contributor

This needs to have the context bound to console doesn't it? Otherwise it throws Uncaught TypeError: Illegal invocation in Chrome (probably others too)

@dcherman

dcherman Dec 14, 2015

Contributor

This needs to have the context bound to console doesn't it? Otherwise it throws Uncaught TypeError: Illegal invocation in Chrome (probably others too)

This comment has been minimized.

@mgol

mgol Dec 14, 2015

Member

That's true... @dmethvin did you check it in Chrome?

@mgol

mgol Dec 14, 2015

Member

That's true... @dmethvin did you check it in Chrome?

This comment has been minimized.

@dmethvin

dmethvin Dec 14, 2015

Member

Yep, this needs to be fixed, thanks!

@dmethvin

dmethvin Dec 14, 2015

Member

Yep, this needs to be fixed, thanks!

This comment has been minimized.

@dmethvin

dmethvin Dec 14, 2015

Member

So since I wasn't calling the old console API (just shimming it) the error didn't occur. 😖

@dmethvin

dmethvin Dec 14, 2015

Member

So since I wasn't calling the old console API (just shimming it) the error didn't occur. 😖

dmethvin added some commits Nov 23, 2015

[SQUASH]: Updates per code review suggestions
* The stack is worthless because the promise is resolved by setTimeout()
* New module created for exceptionHook()
* The .stack property is not useful for our purposes
@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 20, 2015

Member

Updated to add the stack hook. I'm not fully confident about what's here, would love a review, esp from @gibson042.

Member

dmethvin commented Dec 20, 2015

Updated to add the stack hook. I'm not fully confident about what's here, would love a review, esp from @gibson042.

test/unit/deferred.js
+ // but a custom getStackHook+exceptionHook pair could save a raw form and
+ // format it to a string only when an exception actually occurs.
+ // For the unit test we just ensure the plumbing works.
+console.trace();

This comment has been minimized.

@dmethvin

dmethvin Dec 20, 2015

Member

This needs to be removed but I wanted to be sure we had the valid stack at this point of execution.

@dmethvin

dmethvin Dec 20, 2015

Member

This needs to be removed but I wanted to be sure we had the valid stack at this point of execution.

+ // Call an optional hook to record the stack, in case of exception
+ // since it's otherwise lost when execution goes async
+ if ( jQuery.Deferred.getStackHook ) {
+ process.stackTrace = jQuery.Deferred.getStackHook();

This comment has been minimized.

@dmethvin

dmethvin Dec 20, 2015

Member

I struggled with finding a good place to put the stack trace, it needs to be per-promise and not leak memory.

@dmethvin

dmethvin Dec 20, 2015

Member

I struggled with finding a good place to put the stack trace, it needs to be per-promise and not leak memory.

This comment has been minimized.

@timmywil

timmywil Dec 21, 2015

Member

Perhaps it doesn't need to be stored, just logged?

@timmywil

timmywil Dec 21, 2015

Member

Perhaps it doesn't need to be stored, just logged?

This comment has been minimized.

@timmywil

timmywil Dec 21, 2015

Member

I'm thinking of something akin to a verbose mode, which could be turned on and run for all deferreds during debugging. Just an idea.

@timmywil

timmywil Dec 21, 2015

Member

I'm thinking of something akin to a verbose mode, which could be turned on and run for all deferreds during debugging. Just an idea.

This comment has been minimized.

@dmethvin

dmethvin Dec 21, 2015

Member

The problem is that we have to get the stack before the setTimeout and at that point we don't know whether there will be a problem or not. If you run the tests with the console.trace() there right now you can see how noisy that would be. We only want to dump the stack if there's an exception.

@dmethvin

dmethvin Dec 21, 2015

Member

The problem is that we have to get the stack before the setTimeout and at that point we don't know whether there will be a problem or not. If you run the tests with the console.trace() there right now you can see how noisy that would be. We only want to dump the stack if there's an exception.

src/deferred/exceptionHook.js
+ var warn = window.console && window.console.warn;
+
+ if ( warn && error && rerrorNames.test( error.name ) ) {
+ warn.call( window.console, "jQuery.Deferred exception: " + error.message, stack );

This comment has been minimized.

@dcherman

dcherman Dec 23, 2015

Contributor

Not that this matters in the slightest, but seeing as how you're accessing the global immediately above this as feature detection, is there any reason to .call rather than just window.console.warn(...)? Might even save some bytes ;)

@dcherman

dcherman Dec 23, 2015

Contributor

Not that this matters in the slightest, but seeing as how you're accessing the global immediately above this as feature detection, is there any reason to .call rather than just window.console.warn(...)? Might even save some bytes ;)

This comment has been minimized.

@dmethvin

dmethvin Dec 23, 2015

Member

Yeah, technically I could just let gzip do its thang and avoid the variable, which would also remove the need for the .call. I'm okay with that if people prefer it.

@dmethvin

dmethvin Dec 23, 2015

Member

Yeah, technically I could just let gzip do its thang and avoid the variable, which would also remove the need for the .call. I'm okay with that if people prefer it.

This comment has been minimized.

@mgol

mgol Dec 23, 2015

Member

Sounds like a good idea.

Michał Gołębiowski

@mgol

mgol Dec 23, 2015

Member

Sounds like a good idea.

Michał Gołębiowski

@dmethvin

This comment has been minimized.

Show comment
Hide comment
@dmethvin

dmethvin Dec 28, 2015

Member

I've put together a plugin that uses the hook for collecting the stack and then shows it when an exception occurs. In the README I mention that having to collect the stack in advance makes Deferreds slower by roughly 2x which is why it isn't on by default. However, since it's such a small amount of code I wonder whether we shouldn't just have a flag or other simple one-liner to enable this rather than making people load another module/plugin to do it.

Member

dmethvin commented Dec 28, 2015

I've put together a plugin that uses the hook for collecting the stack and then shows it when an exception occurs. In the README I mention that having to collect the stack in advance makes Deferreds slower by roughly 2x which is why it isn't on by default. However, since it's such a small amount of code I wonder whether we shouldn't just have a flag or other simple one-liner to enable this rather than making people load another module/plugin to do it.

@timmywil

This comment has been minimized.

Show comment
Hide comment
@timmywil

timmywil Dec 28, 2015

Member

I like that it's a separate plugin, with its own README explaining why it's necessary. A flag can easily be kept on by accident.

Member

timmywil commented Dec 28, 2015

I like that it's a separate plugin, with its own README explaining why it's necessary. A flag can easily be kept on by accident.

@dmethvin dmethvin referenced this pull request in dmethvin/jquery-deferred-reporter Dec 29, 2015

Closed

Do you actually need to throw? #2

@dmethvin dmethvin closed this in 36a7cf9 Jan 13, 2016

@dmethvin dmethvin deleted the dmethvin:2736-unhandled-exceptions branch May 24, 2016

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