support logging "duck typed" Error objects

[terencehonles]

Description of the change

In TypeScript Error objects might be implemented instead of extended (i.e. HttpErrorResponse). This causes the instanceof check to fail, and this allows more wiggle room. I checked the codebase and I didn't see any other uses of instanceof on the item.err so I imagine this should be not an issue.

Description here

Type of change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to not work as expected)

Development

• Lint rules pass locally
• The code changed/added as part of this pull request has been covered with tests
• All tests related to the changed code pass in development

Code review

• This pull request has a descriptive title and information useful to a reviewer. There may be a screenshot or screencast attached
• "Ready for review" label attached to the PR and reviewers mentioned in a comment
• Changes have been reviewed by at least one other engineer
• Issue from task tracker has a link to this pull request

[waltjones]

This expression is long enough it probably merits being extracted into a function, especially since it appears twice.

[terencehonles]

I noticed there was an isError function and I modified that instead.

[terencehonles]

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.

[terencehonles]

One thing to call out the typeName function will produce odd results with the following:

class Test {
    [Symbol.toStringTag] = 'Test'
}

// "OK", but not in the set of expected return values
({}).toString.call(new Test()).match(/\s([a-zA-Z]+)/)[1].toLowerCase() // 'test'

class Test2 {
    [Symbol.toStringTag] = 'Test words'
}

// Regex doesn't capture everything
({}).toString.call(new Test2()).match(/\s([a-zA-Z]+)/)[1].toLowerCase() // 'test'

This is mostly not going to be an issue, but it may break when the default is expected to be object

[terencehonles]

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

[waltjones]

I'm concerned that the duck typing is so broad it will affect other users of Rollbar.js. While I understand stack is not a required property of Error, duck typing based on just message and name is so generic it risks including objects that were intended as custom data on a message occurrence, not as an error object. In effect, this would cause those occurrences to be converted to exceptions.

I also have concerns about the Angular example. My interpretation is that HttpErrorResponse is a type of HTTP response, not a type of Error. If it includes an Error, it will be assigned to the error property of the response: https://github.com/angular/angular/blob/9a5ac47331faf0f6030bff75ad8ef8dab2f8d2d9/packages/common/http/src/response.ts#L311-L315

I would be more in favor of a solution where the caller finds or creates a valid error object, or where the type that gets inferred is more certain.

For example, this precise situation, passing a HttpErrorResponse object to Rollbar.log() , would automatically get converted to an exception occurrence even though the intent may easily have been to log it as custom data. Many users may already be doing this, and the duck typing would break that use case.

[terencehonles]

Well the problem is that Rollbar.js is too "magical" and there is not a good way to force this object to be interpreted as an error. I agree with the concerns about potentially breaking existing behavior and why I suggested there at least be a name and a message.

This issue was a result of the following documentation: https://docs.rollbar.com/docs/angular

I am asking for this to be reported as an error this.rollbar.error(err.originalError || err); but is not. One thing to note is that originalError will never exist (at least on modern versions of Angular which this handler fills in for) and if you are going to use it it will be ngOriginalError (in practice I've not seen it, and the default handler will log both errors so the documentation should probably not prefer ngOriginalError).

While it is true that the example I give is also an HTTP response it declares it is also an Error. Because JavaScript lacks multiple inheritance an implementer is required to choose one of the two. Because the HttpResponseBase provides more to the object that is used instead of Error as a base. One thing to point out in the documentation you linked is the last part:

The error property will contain either a wrapped Error object or the error response returned from the server.

While it may contain an Error object, it may also contain the JSON/body returned by the server .

Stumbling across an Angular example while finding that ^^^ I also see their example suggests logging the HttpErrorResponse as an error: https://github.com/angular/angular/blob/69385f7df4e678bbd8a04825b4ea2020f66190bc/aio/content/examples/http/src/app/http-error-handler.service.ts#L33-L34

I agree that might not be strong reasoning, but I think the stronger reasoning is that I'm calling rollbar.error with a single argument. I'm not sure how or why people use custom data, but I would hope the normal convention is not rollbar.error({name, message}). Unfortunately it looks like you might be suggesting that might be the case.

[waltjones]

The custom data can have any structure, message is a common key name in Rollbar related data, and name is common in general. So having a custom data object that fits this duck type is reasonably likely. It's especially likely that some Angular users would be passing the HttpErrorResponse we're discussing as custom data on a non-error message. There are maybe as many people that log HTTP responses as non-errors than as errors. (Not that either way is better, just that it needs to remain supported.)

One way forward would be a config option specifying types that should be treated as errors. Something like:

config.handleAsError = ['HttpErrorResponse'];

Then the utility function could compare against this list if present, instead of the duck type.

This has a few advantages:

• The default behavior is unchanged.
• No false positives.
• Precision. An app could both have non-standard error types and have custom data with message and name properties. Everything would still be handled as expected.

[terencehonles]

How do you imagine this working with strings? I would think it would need to be a callable and I'm not sure what it would be acting upon. Would it need to be applied to all arguments passed to rollbar.log? Would it make sense to types to add to the instanceof check?

// base classes
config.additionalErrorTypes = [HttpErrorResponse]

// callable
config.additionalErrorTypes = (e) => e instanceof HttpErrorResponse

[waltjones]

I had two reasons for suggesting using strings in the config.

1. As you've discovered, type matching is currently performed using strings. (typeName function) 2. Passing strings in the config means the types don't have to be defined in the file where the config is initialized. In some cases, that could even be an inline script tag.

It's hard to imagine taking a major change to how type matching is performed. That said, my comment wasn't meant to be prescriptive.

[terencehonles]

Hi @waltjones sorry I couldn't follow up sooner, but while I understand your suggestion and why you suggested it. I'm not sure how it would work. The typeName function will return object for anything that's not an Error subclass:

function typeName(x) {
  return ({}).toString.call(x).match(/\s([a-zA-Z]+)/)[1].toLowerCase();
}

typeName(new Error); // 'error'

class SubError extends Error {}
typeName(new SubError); // 'error'

class CustomError {}
typeName(new CustomError); // 'object'

class Thing {}
typeName(new Thing); // 'object'

As a result I think either allowing a user to supply a predicate function or passing classes to the config will be the only thing that works. For the array of classes approach the downside is the classes would have to be resolved at the time the config is set. For the predicate function this would at least allow the user some flexibility and it could possibly be extended to return a promise. With the predicate function the classes could be globally stored on window or some other shared structure.

For me this detail does not matter because I will be configuring rollbar during Angular's module initialization and I can import the classes I need and passing them as an array or a predicate function is pretty much the same.

A promise might be taking things too far since if the promise doesn't resolve by the time an error occurs (or breaks when resolving) it might complicate how things should be handled.

Anyways, some input on what might get merged would be appreciated.