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

support logging "duck typed" Error objects #915

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/server/rollbar.js
Original file line number Diff line number Diff line change
Expand Up @@ -280,7 +280,7 @@ Rollbar.prototype.errorHandler = function () {
return next(err, request, response);
}

if (err instanceof Error) {
if (_.isError(err)) {
return this.error(err, request, cb);
}
return this.error('Error: ' + err, request, cb);
Expand Down
10 changes: 6 additions & 4 deletions src/utility.js
Original file line number Diff line number Diff line change
Expand Up @@ -139,8 +139,10 @@ function isIterable(i) {
* @returns true if e is an error
*/
function isError(e) {
// Detect both Error and Firefox Exception type
return isType(e, 'error') || isType(e, 'exception');
// Detect Error, Firefox Exception type and error like object
return (isType(e, 'error')
|| isType(e, 'exception')
|| (typeof e === 'object' && e && e.name && e.message));
Copy link
Author

@terencehonles terencehonles Jan 20, 2021

Choose a reason for hiding this comment

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

I did not save the typeName and move the comparisons inline because of this #915 (comment)

}

function redact() {
Expand Down Expand Up @@ -410,7 +412,7 @@ function createItem(args, logger, notifier, requestKeys, lambdaContext) {
break;
case 'object':
case 'array':
if (arg instanceof Error || (typeof DOMException !== 'undefined' && arg instanceof DOMException)) {
Copy link
Author

Choose a reason for hiding this comment

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

in my browser DOMException is a subclass of Error so the case 'domexception': should catch it already, but isError now catches it because a DOMException also has a name and a message.

if (isError(arg)) {
err ? extraArgs.push(arg) : err = arg;
break;
}
Expand All @@ -428,7 +430,7 @@ function createItem(args, logger, notifier, requestKeys, lambdaContext) {
custom ? extraArgs.push(arg) : custom = arg;
break;
default:
if (arg instanceof Error || (typeof DOMException !== 'undefined' && arg instanceof DOMException)) {
if (isError(arg)) {
err ? extraArgs.push(arg) : err = arg;
break;
}
Expand Down
32 changes: 32 additions & 0 deletions test/utility.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,38 @@ describe('isError', function() {
expect(_.isError(e)).to.be.ok();
done();
});
it('should handle classes implementing the error interface', function(done) {
// This is a mostly browser compliant way of doing this
// just for the sake of doing it, even though we mostly
// need this to work in node environments
function TestCustomError(message) {
Object.defineProperty(this, 'name', {
enumerable: false,
writable: false,
value: 'TestCustomError'
});

Object.defineProperty(this, 'message', {
enumerable: false,
writable: true,
value: message
});

if (Error.hasOwnProperty('captureStackTrace')) {
Error.captureStackTrace(this, TestCustomError);
} else {
Object.defineProperty(this, 'stack', {
enumerable: false,
writable: false,
value: (new Error(message)).stack
});
}
}

var e = new TestCustomError('bork');
expect(_.isError(e)).to.be.ok();
done();
});
});

describe('merge', function() {
Expand Down