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

Improve unhandled rejection reason strings #817

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

timmfin
Copy link

@timmfin timmfin commented Jun 16, 2017

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 #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
Copy link
Owner

Released in v1.5.1

@timmfin
Copy link
Author

timmfin commented Oct 20, 2017

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
Copy link
Owner

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

Copy link
Owner

@kriskowal kriskowal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’m open to this feature. Sorry for the delay.


// Public hook to allow for custom tweaking of unhandled rejection string (note, stack
// will be appended afterwards by code below)
if (Q.customizeRejectionString) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we should name the hook rewriteReason

Q.reject(error);

expect(Q.getUnhandledReasons()).toEqual([error.stack]);
if (firstLineOfStack && firstLineOfStack.indexOf('a reason') >= 0) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Check lint.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants