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 unhandled exception notices for Deferred #2736

Closed
dmethvin opened this issue Nov 20, 2015 · 9 comments
Closed

Add unhandled exception notices for Deferred #2736

dmethvin opened this issue Nov 20, 2015 · 9 comments
Assignees
Labels
Milestone

Comments

@dmethvin
Copy link
Member

@dmethvin dmethvin commented Nov 20, 2015

Right now, if there is an exception in a .then() it is silently swallowed. I don't think we should ship 3.0.0 with that behavior.

http://jsbin.com/bodevaqive/edit?js,console

Maybe a console.warn message? We may end up yelling about rejections that might have been dealt with by a handler attached later in time.

One possible tradeoff would be to only generate the console warnings for errors that seem to be programming mistakes such as TypeError or ReferenceError. That prevents the really bad mistakes such as the example above with a typo in a function name.

For more complex cases we could recommend that people use native Promise if it's consistently available across their supported platforms, since it will be easier to debug.

@dmethvin dmethvin added the Deferred label Nov 20, 2015
@dmethvin dmethvin added this to the 3.0.0 milestone Nov 20, 2015
@mgol
Copy link
Member

@mgol mgol commented Nov 21, 2015

Unhandled rejections may be handled later so going through console seems wrong to me. We could expose something like jQuery.Deferred.unhandledRejections and/or a special event people could hook to to see rejections.

If this went through a property and not events, we'd need to remove stuff from there once it's handled so we'd have to be careful about perf.

Although this:

One possible tradeoff would be to only generate the console warnings for errors that seem to be programming mistakes such as TypeError or ReferenceError.

seems like a good tradeoff to me. But maybe we could do more.

@mgol
Copy link
Member

@mgol mgol commented Nov 21, 2015

Also, it's worth noting what Node.js did to make working with rejections easier: https://nodejs.org/api/process.html#process_event_unhandledrejection

tl;dr: a unhandlerRejection event. We could do both that and a console.warn for TypeError/ReferenceError.

@timmywil
Copy link
Member

@timmywil timmywil commented Nov 22, 2015

I'm not a fan of console messages from a production library. What about removing the default unhandled rejection handler – which would just rethrow – if another handler is attached?

@gibson042
Copy link
Member

@gibson042 gibson042 commented Nov 22, 2015

I don't have time to address this today, but we can talk about it at the next meeting. I'm opposed to the core library having an opinion, and still maintain that it's a plugin problem for now. I'm happy to write it... have we documented current best practices?

@timmywil
Copy link
Member

@timmywil timmywil commented Nov 23, 2015

@gibson042 There's a note about it in the blog post, but I don't think we've added anything to api.jquery.com about it. Is the deferred.then page the best place for that?

@timmywil
Copy link
Member

@timmywil timmywil commented Nov 23, 2015

Also, I agree. Supporting Promises/A+ is a mixed bag for this reason. I'm inclined to leave this up to the user, but warn strongly against allowing phantom errors.

@dmethvin
Copy link
Member Author

@dmethvin dmethvin commented Nov 23, 2015

Having a default that swallows errors is really bad though, especially compared to the current behavior that gives immediate feedback about programming errors.

@timmywil
Copy link
Member

@timmywil timmywil commented Nov 23, 2015

It is bad, but there's an argument to be made that it comes with the territory. Either way, we'll probably be more equipped to make this decision if we have code to review. My backlog is clear. I could mock something up.

@markelog
Copy link
Member

@markelog markelog commented Nov 23, 2015

dmethvin added a commit to dmethvin/jquery that referenced this issue Nov 23, 2015
dmethvin added a commit to dmethvin/jquery that referenced this issue Nov 23, 2015
dmethvin added a commit to dmethvin/jquery that referenced this issue Dec 20, 2015
@dmethvin dmethvin closed this in 36a7cf9 Jan 13, 2016
@mgol mgol removed the Needs review label Feb 23, 2016
@mgol mgol removed the Has Pull Request label Mar 6, 2016
dmethvin added a commit to dmethvin/jquery that referenced this issue May 11, 2016
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.
dmethvin added a commit that referenced this issue May 12, 2016
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.
@lock lock bot locked as resolved and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
5 participants
You can’t perform that action at this time.