This repository was archived by the owner on Jan 22, 2025. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Improve unhandled rejection reason strings #817
Open
timmfin
wants to merge
6
commits into
kriskowal:master
Choose a base branch
from
timmfin:improve-rejection-reason-strings
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Improve unhandled rejection reason strings #817
timmfin
wants to merge
6
commits into
kriskowal:master
from
timmfin:improve-rejection-reason-strings
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
… always contain the error's type and message since some browsers (like Firefox) don't include the error type/message in `reason.stack`.
…e how the rejection is string-ified.
…tabs from elsewhere in the file).
Owner
|
Released in v1.5.1 |
Author
|
Looks like ^ comment on v1.5.1 is accidental? (The PR isn’t merged and I don’t see any other commits in 1.5.1 related to this) ¯\_(ツ)_/¯ |
This PR improves the strings that end up in
Q.getUnhandledReasons()two different ways:Errors in all modern browsers include the error type and message (in addition to the stack).Q.customizeRejectionStringwhich can be used for developers to customize how rejection values are string-ified.For 1, the problem I ran into was that rejection reasons inside
Q.getUnhandledReasons()didn't actually contain the error type and message and they only had the error stack in some browsers. This is because Q useserror.stackand the output oferror.stackin Firefox and Safari is different than I expected. For example:Not only was that surprising to me, but it makes tracking down the unhandled rejections that real users run into in Firefox & Safari much harder to debug since the error message is missing. So in these changes I detect when
error.stackis missing the error type & message and manually insert it to the beginning of the string that ends up inQ.getUnhandledReasons():Before in Firefox
After in Firefox
While 1 is an improvement, it doesn't help when some code rejects an object, primitive, or custom instance (which don't have a stack strace). So 2 adds the
Q.customizeRejectionStringhook which can allow developers to customize unhandled rejection strings themselves. For example:Or some
JSON.stringify-ing...I know that we ideally should only be rejecting JS errors and not other values, but we're far from perfect. We have plenty of code written before we had a better understanding of promises, have legacy CoffeeScript where automatic returns can get in the way, need to do a better job of teaching developers about promise foot-guns, etc. So
Q.customizeRejectionStringcould a big help for us to track down some of these problematic promise chains.I should also mention that I've added a some tests for these changes, ensured npm test was green, and ran q-spec.html in a few browsers (latest Chrome, Firefox, and Safari).
ps: This PR and #816 came out of me trying to provide better tooling to other (HubSpot) developers trying to track down and fix various unhandled rejections. On our production apps we have code that polls
Q.getUnhandledReasons()every 5 seconds, sending anything there to a JS error tracking service. While that helped us track down several problematic promise chains, there still were many unhandled "errors" that were useless. And I'm hoping these changes will help track those down.