Core: Re-throw errors that happened in callbacks wrapped in jQuery ready #3210

mgol · 2016-06-29T12:22:15Z

Summary

Re-throw errors that happened in callbacks wrapped in jQuery ready

Also, expose jQuery.readyException that allows to overwrite the default
ready error handler.

Checklist

Mark an [x] for completed items, if you're not sure leave them unchecked and we can assist.

All authors have signed the CLA at https://contribute.jquery.com/CLA/
New tests have been added to show the fix or feature works
Grunt build and unit tests pass locally with these changes
If needed, a docs issue/PR was created at https://github.com/jquery/api.jquery.com (Document jQuery.readyException api.jquery.com#942)

Thanks! Bots and humans will be around shortly to check it out.

Fixes gh-3174

mgol · 2016-06-29T12:39:49Z

Please review. I'd like to have it released quickly, preferably no later than next week. cc @jquery/core

dmethvin · 2016-07-01T16:57:51Z

test/unit/core.js

+	var message,
+		origWindowSetTimeout = window.setTimeout;
+
+	window.setTimeout = function( fn ) {


I was wondering how you'd do this 👍

You could use sinon here, right?

Yeah, I was thinking about sinon but it's not activated at all for this module, I'd need to enter the sinon sandbox to be able to use spies, wouldn't I?

Not necessary to use sandbox i think, sandbox usually needed to clean up some scope of spies/stubs, you probably could just use -

sinon.stub(window, 'setTimeout', myOwnThing);

Thanks, I'll try that.

You can use sinon if you want, but for a case like this it's really nice to be able to look at the code and understand what it does without reading though sinon docs.

I'd say sinon is used almost universally for mocking/spying/stubing/etc it has very simple obvious api with clear documentation and we already use it.

So i would suggest to use it anywhere we can

@markelog I had to create a sandbox and use this.sandbox.stub instead of sinon.stub or stubs wouldn't have been cleaned up (because how would Sinon know when to clean up?). It works fine now.

dmethvin · 2016-07-01T16:58:18Z

LGTM

markelog · 2016-07-01T21:29:38Z

src/core/ready.js

 define( [
 	"../core",
 	"../var/document",
+	"../core/readyException",


This should be 3.1 only though i.e. minor? Do we have corresponding docs pr/issue?

I know you labelled it as 3.1.0, just checking

Oh, i see jquery/api.jquery.com#942, we need a pr for it though? Before we can release?

I'll submit a docs PR once this is accepted, no worries.

At the meeting we agreed that since this issue kind of breaks all Angular apps (as they wrap their init code in $() and all bootstrapping errors are silenced) we'll release a quick 3.1.0 with maybe only this fix. I renamed previous milestones: 3.0.1 to 3.1.1 and 3.1.0 to 3.2.0.

Sounds good

markelog · 2016-07-01T21:37:51Z

Do you wanna release before providing a fix for #3194?

mgol · 2016-07-01T22:04:54Z

src/core/readyException.js

+jQuery.readyException = function( error ) {
+	window.setTimeout( function() {
+		throw error;
+	} );


I forgot @gibson042 wanted to have this reject as well so that a more general potential unhandled rejections mechanism would catch it. The code would be:

jQuery.readyException = function( error ) { window.setTimeout( function() { throw error; } ); throw error; };

I have doubts about the need for that, though - the error thrown in a timeout would fire window.onerror anyway, do we need both mechanism to work here? Usually an error is either synchronous and handled by window.onerror or asynchronous and handled by the other mechanism.

cc @dmethvin as you already LGTM'd this PR. :)

It seems we would need to rename readyException? If it would be used in more general sense? Also we need to re-throw error twice?

It's not meant to be used in a more general sense, it was only about integrating with existing debugging tools.

Not sure if i understand the use-case, maybe it would be better to discuss this in separate ticket?

I forgot @gibson042 wanted to have this reject as well so that a more general potential unhandled rejections mechanism would catch it.

If you wanted to catch your own ready handler's rejections you can already do $.when($.ready).then(fn).catch(errfn) in 3.0. What other cases do we want to handle? I feel like 99.9% of the cases are errors and as long as we manage to dump a stack we're doing all we need to do.

The important detail to me is that our default handling doesn't suppress unhandled rejections, since doing so was the essence of the original complaint (and I still want to keep the door open for an unhandled rejections plugin). In fact, if we were willing to advocate $.when($.ready).then(fn).catch(errfn) strongly enough, then gh-3174 could be closed with no code changes whatsoever.

@gibson042 I think that could work only if we deprecated/removed $(fn) but this is such a popular pattern that I don't see it going away any time soon. And this fact alone is enough for me to want it not silencing exceptions.

nicklou · 2016-07-05T19:11:20Z

Is it possible that we use something similar to$.on( "error", handler ) method, but with a different event?

For instance:

$( fn ).on( "exception", handler );

That way, you don't break the existing API and the modification is not on deferreds necessarily, although I am not sure if it is possible to put a correlation between the fn called and the .on() handler.

dmethvin · 2016-07-05T19:46:40Z

For existing code, no additional API is useful unless its default provides helpful information, which this PR does. Someone who doesn't like the default can change it.

For new code where an exception is expected, you can either attach a handler to the promise via .catch() as described above or you can just add a try/catch to the synchronous code.

I don't think anything else is required is it? What case would another API handle that those options do not?

Also, expose jQuery.readyException that allows to overwrite the default ready error handler. Fixes jquerygh-3174 Closes jquerygh-3210

mgol · 2016-07-06T09:59:21Z

The PR is ready and passes npm test locally. Travis fails because npm currently has problems in some locations.

I've also submitted API docs for this method: jquery/api.jquery.com#946. Please review.

I'd like to land this today if no one objects.

gibson042 · 2016-07-06T18:00:34Z

test/unit/core.js

+	jQuery( function() {
+		throw new Error( "Error in jQuery ready" );
+	} );
+	assert.strictEqual( message, "Error in jQuery ready", "The error wasn't thrown in a timeout" );


Shouldn't "wasn't" here be "was"?

gibson042 · 2016-07-06T18:03:24Z

The default jQuery.readyException still needs a throw error.

Also, expose jQuery.readyException that allows to overwrite the default ready error handler. Fixes jquerygh-3174 Closes jquerygh-3210

mgol · 2016-07-06T21:27:47Z

The default jQuery.readyException still needs a throw error.

What about the @dmethvin's remark at #3210 (comment)?

I'm not sure I see the need of a double-error-dispatch approach here. In general there are two kinds of errors - the ones that propagate to window.onerror and the ones who don't and might be collected by a mechanism that records unhandled promise rejections (https://github.com/jquery/jquery/blob/3.0.0/src/deferred/exceptionHook.js). If here we'd throw both synchronously and asynchronously, we'd collect the error in both of them.

To me the fact that jQuery( fn ) uses Deferreds is somewhat an implementation details since you can't use this API in the same way as you can attach error handlers to promises (the jQuery.ready promise is meant for that).

timmywil · 2016-07-06T21:36:39Z

I agree. The main problem was that there was no info on the error. One throw seems like enough.

gibson042 · 2016-07-06T22:32:58Z

To me the fact that jQuery( fn ) uses Deferreds is somewhat an implementation details since you can't use this API in the same way as you can attach error handlers to promises (the jQuery.ready promise is meant for that).

Okay, that's persuasive. Suggestion withdrawn.

markelog · 2016-07-07T08:06:06Z

src/core/ready.js

-	readyList.then( fn );
+	readyList
+		.then( fn )
+		.catch( function( error ) {


.catch(jQuery.readyException)

wouldn't work?

@markelog #3210 (comment)

I added a comment there.

Also, expose jQuery.readyException that allows to overwrite the default ready error handler. Fixes jquerygh-3174 Closes jquerygh-3210

markelog · 2016-07-07T08:38:26Z

LGTM

mgol · 2016-07-07T08:41:03Z

All comments addressed. I'll land it later today unless new review remarks come up before.

jquerybot added the CLA: Valid label Jun 29, 2016

mgol added this to the 3.1.0 milestone Jun 29, 2016

mgol mentioned this pull request Jun 29, 2016

Document jQuery.readyException jquery/api.jquery.com#942

Closed

mgol mentioned this pull request Jun 29, 2016

Exceptions silently swallowed in $(fn) #3174

Closed

mgol removed this from the 3.1.0 milestone Jun 29, 2016

mgol mentioned this pull request Jun 29, 2016

Deferred: Fix exceptions silent swallowing #3202

Closed

4 tasks

dmethvin reviewed Jul 1, 2016
View reviewed changes

markelog reviewed Jul 1, 2016
View reviewed changes

mgol reviewed Jul 1, 2016
View reviewed changes

mgol force-pushed the readyException branch from 682c5e4 to 0fe6cad Compare July 6, 2016 09:06

gibson042 reviewed Jul 6, 2016
View reviewed changes

mgol force-pushed the readyException branch from 0fe6cad to bfbc395 Compare July 6, 2016 18:41

markelog reviewed Jul 7, 2016
View reviewed changes

Core: Re-throw errors that happened in callbacks wrapped in jQuery ready

ad6a94c

Also, expose jQuery.readyException that allows to overwrite the default ready error handler. Fixes jquerygh-3174 Closes jquerygh-3210

mgol force-pushed the readyException branch from bfbc395 to ad6a94c Compare July 7, 2016 08:23

mgol merged commit ad6a94c into jquery:master Jul 7, 2016

mgol deleted the readyException branch July 7, 2016 16:07

lock bot locked as resolved and limited conversation to collaborators Jan 18, 2019

Core: Re-throw errors that happened in callbacks wrapped in jQuery ready #3210

Core: Re-throw errors that happened in callbacks wrapped in jQuery ready #3210

Uh oh!

Conversation

mgol commented Jun 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Checklist

Uh oh!

mgol commented Jun 29, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dmethvin commented Jul 1, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

markelog commented Jul 1, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicklou commented Jul 5, 2016

Uh oh!

dmethvin commented Jul 5, 2016

Uh oh!

mgol commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gibson042 commented Jul 6, 2016

Uh oh!

mgol commented Jul 6, 2016

Uh oh!

timmywil commented Jul 6, 2016

Uh oh!

gibson042 commented Jul 6, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mgol commented Jun 29, 2016 •

edited

Loading

mgol Jul 1, 2016 •

edited

Loading

markelog Jul 1, 2016 •

edited

Loading

markelog commented Jul 1, 2016 •

edited

Loading