kriskowal / q

A promise library for JavaScript
MIT License
14.93k stars 1.2k forks source link

Improve unhandled rejection reason strings #817

Open timmfin opened 7 years ago

timmfin commented 7 years ago

This PR improves the strings that end up in Q.getUnhandledReasons() two different ways:

  1. Ensure that unhandled rejection strings created from actual Errors in all modern browsers include the error type and message (in addition to the stack).
  2. Add a hook, Q.customizeRejectionString which 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 uses error.stack and the output of error.stack in Firefox and Safari is different than I expected. For example:

try { throw new Error('Test error') } catch (e) { console.log(e.stack) }

// in Chrome (and similar in Edge/IE)...
// => Error: Test error
// =>      at <anonymous>:1:13

// but in Firefox and Safari the error type and message isn't included, and you get ...
// => @debugger eval code:1:13

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.stack is missing the error type & message and manually insert it to the beginning of the string that ends up in Q.getUnhandledReasons():

Before in Firefox

Q.reject(new Error('faux error'));
Q.getUnhandledReasons()[0];
// => @debugger eval code:1:10

After in Firefox

Q.reject(new Error('faux error'));
Q.getUnhandledReasons()[0];
// => Error: faux error
// => @debugger eval code:1:10

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.customizeRejectionString hook which can allow developers to customize unhandled rejection strings themselves. For example:

CustomClass = function () {}
Q.reject(new CustomClass)

Q.getUnhandledReasons()[0];
// => (no stack) [object Object]

// But if we add this...
Q.customizeRejectionString = function(reason) {
  if (reason.constructor && reason.constructor.name) {
    return 'CUSTOM: ' + reason.constructor.name;
  } 
};

Q.reject(new CustomClass)

Q.getUnhandledReasons()[1]
//=> (no stack) CUSTOM: CustomClass

Or some JSON.stringify-ing...

Q.customizeRejectionString = function(reason) {
  if (!(reason instanceof Error) && typeof reason === 'object') {
    return JSON.stringify(reason)
  } 
};

Q.reject({ foo: 'bar' });
Q.getUnhandledReasons()[0]
// => (no stack) {"foo":"bar"}

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.customizeRejectionString could 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 https://github.com/kriskowal/q/pull/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.

kriskowal commented 7 years ago

Released in v1.5.1

timmfin commented 7 years ago

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) ¯\_(ツ)_/¯

kriskowal commented 7 years ago

@timmfin Indeed. There was an unrelated change to the same feature.