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.

Sign up for GitHub

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

ref(angular) Add error-like objects handling #6446

Merged
merged 3 commits into from
Jan 9, 2023
Merged

ref(angular) Add error-like objects handling #6446

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
6c7d87f
ref(angular) Add error-like objects handling
theofidry Dec 16, 2022
513af89
Inline
theofidry Dec 20, 2022
a25e6c5
Simplify the condition
theofidry Dec 21, 2022
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
41 changes: 35 additions & 6 deletions packages/angular/src/errorhandler.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ function tryToUnwrapZonejsError(error: unknown): unknown | Error {

function extractHttpModuleError(error: HttpErrorResponse): string | Error {
// The `error` property of http exception can be either an `Error` object, which we can use directly...
if (error.error instanceof Error) {
if (isErrorOrErrorLikeObject(error.error)) {
return error.error;
}

Expand All @@ -50,6 +50,35 @@ function extractHttpModuleError(error: HttpErrorResponse): string | Error {
return error.message;
}

function isObject(value: unknown): value is Record<string, unknown> {
return value !== null && typeof value === 'object';
}

function hasOwnProperty<ObjectType extends Record<string, unknown>, PropertyType extends PropertyKey>(
object: ObjectType,
propertyName: PropertyType,
): object is ObjectType & Record<PropertyType, unknown> {
return Object.prototype.hasOwnProperty.call(object, propertyName);
}

function hasErrorName(value: Record<string, unknown>): value is Pick<Error, 'name'> {
return hasOwnProperty(value, 'name') && typeof value.name === 'string';
Copy link
Member

Choose a reason for hiding this comment

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

Not saying it's wrong but why do we need the hasOwnProperty check here? Wouldn't it be enough to check for typeof value.name === 'string'?

Or actually, for the string check, we could simply reuse our isString check from @sentry/utils.

}

function hasErrorMessage(value: Record<string, unknown>): value is Pick<Error, 'message'> {
return hasOwnProperty(value, 'message') && typeof value.message === 'string';
}

function hasErrorStack(value: Record<string, unknown>): value is Pick<Error, 'stack'> {
return !hasOwnProperty(value, 'stack') || typeof value.stack === 'string';
}

function isErrorOrErrorLikeObject(value: unknown): value is Error {
return (
value instanceof Error || (isObject(value) && hasErrorName(value) && hasErrorMessage(value) && hasErrorStack(value))
Copy link
Contributor Author

Choose a reason for hiding this comment

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

of course this could all be inlined, but I find a lot more readable that way

Copy link
Member

Choose a reason for hiding this comment

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

It's very readable but we also have to consider the bundle size impact here. Hence my question if hasOwnProperty is strictly necessary.

Maybe, we could also inline the isObject check?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not very familiar with how the code impacts the bundle size, is there a way I could check the result so that I can check different things to see if there is still a way to keep it readable without increasing the size too much?

);
}

/**
* Implementation of Angular's ErrorHandler provider that can be used as a drop-in replacement for the stock one.
*/
Expand Down Expand Up @@ -117,16 +146,16 @@ class SentryErrorHandler implements AngularErrorHandler {
protected _defaultExtractor(errorCandidate: unknown): unknown {
const error = tryToUnwrapZonejsError(errorCandidate);

// We can handle messages and Error objects directly.
if (typeof error === 'string' || error instanceof Error) {
return error;
}

// If it's http module error, extract as much information from it as we can.
if (error instanceof HttpErrorResponse) {
return extractHttpModuleError(error);
}

// We can handle messages and Error objects directly.
if (typeof error === 'string' || isErrorOrErrorLikeObject(error)) {
return error;
}

// Nothing was extracted, fallback to default error message.
return null;
}
Expand Down
33 changes: 9 additions & 24 deletions packages/angular/test/errorhandler.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ class CustomError extends Error {
}

class ErrorLikeShapedClass implements Partial<Error> {
constructor(public message: string) {}
constructor(public name: string, public message: string) {}
}

function createErrorEvent(message: string, innerError: any): ErrorEvent {
Expand Down Expand Up @@ -118,8 +118,7 @@ describe('SentryErrorHandler', () => {
createErrorHandler().handleError(errorLikeWithoutStack);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
// TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332
expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function));
expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithoutStack, expect.any(Function));
});

it('extracts an error-like object with a stack', () => {
Expand All @@ -132,8 +131,7 @@ describe('SentryErrorHandler', () => {
createErrorHandler().handleError(errorLikeWithStack);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
// TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332
expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function));
expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithStack, expect.any(Function));
});

it('extracts an object that could look like an error but is not (does not have a message)', () => {
Expand All @@ -150,7 +148,6 @@ describe('SentryErrorHandler', () => {

it('extracts an object that could look like an error but is not (does not have an explicit name)', () => {
const notErr: Partial<Error> = {
// missing name; but actually is always there as part of the Object prototype
message: 'something failed.',
};

Expand Down Expand Up @@ -194,12 +191,12 @@ describe('SentryErrorHandler', () => {
});

it('extracts an instance of class not extending Error but that has an error-like shape', () => {
const err = new ErrorLikeShapedClass('something happened');
const err = new ErrorLikeShapedClass('sentry-error', 'something happened');

createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith('Handled unknown error', expect.any(Function));
expect(captureExceptionSpy).toHaveBeenCalledWith(err, expect.any(Function));
});

it('extracts an instance of a class that does not extend Error and does not have an error-like shape', () => {
Expand Down Expand Up @@ -304,11 +301,7 @@ describe('SentryErrorHandler', () => {
createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
// TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332
expect(captureExceptionSpy).toHaveBeenCalledWith(
'Http failure response for (unknown url): undefined undefined',
expect.any(Function),
);
expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithoutStack, expect.any(Function));
});

it('extracts an `HttpErrorResponse` with error-like object with a stack', () => {
Expand All @@ -322,11 +315,7 @@ describe('SentryErrorHandler', () => {
createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
// TODO: to be changed; see https://github.com/getsentry/sentry-javascript/issues/6332
expect(captureExceptionSpy).toHaveBeenCalledWith(
'Http failure response for (unknown url): undefined undefined',
expect.any(Function),
);
expect(captureExceptionSpy).toHaveBeenCalledWith(errorLikeWithStack, expect.any(Function));
});

it('extracts an `HttpErrorResponse` with an object that could look like an error but is not (does not have a message)', () => {
Expand All @@ -347,7 +336,6 @@ describe('SentryErrorHandler', () => {

it('extracts an `HttpErrorResponse` with an object that could look like an error but is not (does not have an explicit name)', () => {
const notErr: Partial<Error> = {
// missing name; but actually is always there as part of the Object prototype
message: 'something failed.',
};
const err = new HttpErrorResponse({ error: notErr });
Expand Down Expand Up @@ -453,16 +441,13 @@ describe('SentryErrorHandler', () => {
});

it('extracts an `HttpErrorResponse` with an instance of class not extending Error but that has an error-like shape', () => {
const innerErr = new ErrorLikeShapedClass('something happened');
const innerErr = new ErrorLikeShapedClass('sentry-error', 'something happened');
const err = new HttpErrorResponse({ error: innerErr });

createErrorHandler().handleError(err);

expect(captureExceptionSpy).toHaveBeenCalledTimes(1);
expect(captureExceptionSpy).toHaveBeenCalledWith(
'Http failure response for (unknown url): undefined undefined',
expect.any(Function),
);
expect(captureExceptionSpy).toHaveBeenCalledWith(innerErr, expect.any(Function));
});

it('extracts an `HttpErrorResponse` with an instance of a class that does not extend Error and does not have an error-like shape', () => {
Expand Down