From 5b12cfb39457f24f3660e3fb747d73f034cd7881 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 5 Apr 2023 10:50:29 +0200 Subject: [PATCH 1/3] fix(angular): Only open report dialog if error was sent --- packages/angular/src/errorhandler.ts | 14 ++++++- packages/angular/test/errorhandler.test.ts | 46 +++++++++++++++++----- 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 941029c2ff9f..7cd321f38a44 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -83,11 +83,23 @@ function isErrorOrErrorLikeObject(value: unknown): value is Error { class SentryErrorHandler implements AngularErrorHandler { protected readonly _options: ErrorHandlerOptions; + private readonly _openFallbackReportDialog; + public constructor(@Inject('errorHandlerOptions') options?: ErrorHandlerOptions) { this._options = { logErrors: true, ...options, }; + + const client = Sentry.getCurrentHub().getClient(); + this._openFallbackReportDialog = this._options.showDialog && !(client && client.on); + if (this._options.showDialog && client && client.on) { + client.on('afterSendEvent', event => { + if (!event.type) { + Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id }); + } + }); + } } /** @@ -119,7 +131,7 @@ class SentryErrorHandler implements AngularErrorHandler { } // Optionally show user dialog to provide details on what happened. - if (this._options.showDialog) { + if (this._options.showDialog && this._openFallbackReportDialog) { Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); } } diff --git a/packages/angular/test/errorhandler.test.ts b/packages/angular/test/errorhandler.test.ts index e4398fd8aa70..fadae7d9b2de 100644 --- a/packages/angular/test/errorhandler.test.ts +++ b/packages/angular/test/errorhandler.test.ts @@ -1,6 +1,7 @@ import { HttpErrorResponse } from '@angular/common/http'; import * as SentryBrowser from '@sentry/browser'; import { Scope } from '@sentry/browser'; +import type { Event } from '@sentry/types'; import * as SentryUtils from '@sentry/utils'; import { createErrorHandler, SentryErrorHandler } from '../src/errorhandler'; @@ -507,15 +508,6 @@ describe('SentryErrorHandler', () => { expect(captureExceptionSpy).toHaveBeenCalledWith('something happened', expect.any(Function)); }); - it('handleError method shows report dialog', () => { - const showReportDialogSpy = jest.spyOn(SentryBrowser, 'showReportDialog'); - - const errorHandler = createErrorHandler({ showDialog: true }); - errorHandler.handleError(new Error('test')); - - expect(showReportDialogSpy).toBeCalledTimes(1); - }); - it('extracts error with a custom extractor', () => { const customExtractor = (error: unknown) => { if (typeof error === 'string') { @@ -530,5 +522,41 @@ describe('SentryErrorHandler', () => { expect(captureExceptionSpy).toHaveBeenCalledTimes(1); expect(captureExceptionSpy).toHaveBeenCalledWith(new Error('custom error'), expect.any(Function)); }); + + describe('opens the report dialog if `showDialog` is true', () => { + it('by using SDK lifecycle hooks if available', () => { + const client = { + cb: (_: Event) => {}, + on: jest.fn((_, cb) => { + client.cb = cb; + }), + }; + + // @ts-ignore this is a minmal hub, we're missing a few props but that's ok + jest.spyOn(SentryBrowser, 'getCurrentHub').mockImplementationOnce(() => { + return { getClient: () => client }; + }); + + const showReportDialogSpy = jest.spyOn(SentryBrowser, 'showReportDialog'); + + const errorHandler = createErrorHandler({ showDialog: true }); + expect(client.on).toHaveBeenCalledWith('afterSendEvent', expect.any(Function)); + errorHandler.handleError(new Error('test')); + + // this simulates the afterSend hook being called + client.cb({}); + + expect(showReportDialogSpy).toBeCalledTimes(1); + }); + + it('by just calling `showReportDialog` if hooks are not available', () => { + const showReportDialogSpy = jest.spyOn(SentryBrowser, 'showReportDialog'); + + const errorHandler = createErrorHandler({ showDialog: true }); + errorHandler.handleError(new Error('test')); + + expect(showReportDialogSpy).toBeCalledTimes(1); + }); + }); }); }); From 91747a522631c830d5be9888273a105e0a50ded9 Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 5 Apr 2023 12:50:53 +0200 Subject: [PATCH 2/3] move hook registration --- packages/angular-ivy/package.json | 2 +- packages/angular/package.json | 2 +- packages/angular/src/errorhandler.ts | 30 ++++++++++++---------- packages/angular/test/errorhandler.test.ts | 10 +++++--- 4 files changed, 26 insertions(+), 18 deletions(-) diff --git a/packages/angular-ivy/package.json b/packages/angular-ivy/package.json index 2c8513a4fcde..ceea2f953c0f 100644 --- a/packages/angular-ivy/package.json +++ b/packages/angular-ivy/package.json @@ -57,7 +57,7 @@ "lint": "run-s lint:prettier lint:eslint", "lint:eslint": "eslint . --format stylish", "lint:prettier": "prettier --check \"{src,test,scripts}/**/**.ts\"", - "yalc:publish": "ts-node ../../scripts/prepack.ts && yalc publish build --push" + "yalc:publish": "yalc publish build --push" }, "volta": { "extends": "../../package.json" diff --git a/packages/angular/package.json b/packages/angular/package.json index 550130c8337b..8a948e693383 100644 --- a/packages/angular/package.json +++ b/packages/angular/package.json @@ -60,7 +60,7 @@ "test": "yarn test:unit", "test:unit": "jest", "test:unit:watch": "jest --watch", - "yalc:publish": "ts-node ../../scripts/prepack.ts && yalc publish build --push" + "yalc:publish": "yalc publish build --push" }, "volta": { "extends": "../../package.json" diff --git a/packages/angular/src/errorhandler.ts b/packages/angular/src/errorhandler.ts index 7cd321f38a44..89f3b8b556ac 100644 --- a/packages/angular/src/errorhandler.ts +++ b/packages/angular/src/errorhandler.ts @@ -83,23 +83,14 @@ function isErrorOrErrorLikeObject(value: unknown): value is Error { class SentryErrorHandler implements AngularErrorHandler { protected readonly _options: ErrorHandlerOptions; - private readonly _openFallbackReportDialog; + /* indicates if we already registered our the afterSendEvent handler */ + private _registeredAfterSendEventHandler = false; public constructor(@Inject('errorHandlerOptions') options?: ErrorHandlerOptions) { this._options = { logErrors: true, ...options, }; - - const client = Sentry.getCurrentHub().getClient(); - this._openFallbackReportDialog = this._options.showDialog && !(client && client.on); - if (this._options.showDialog && client && client.on) { - client.on('afterSendEvent', event => { - if (!event.type) { - Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id }); - } - }); - } } /** @@ -131,8 +122,21 @@ class SentryErrorHandler implements AngularErrorHandler { } // Optionally show user dialog to provide details on what happened. - if (this._options.showDialog && this._openFallbackReportDialog) { - Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); + if (this._options.showDialog) { + const client = Sentry.getCurrentHub().getClient(); + + if (client && client.on && !this._registeredAfterSendEventHandler) { + client.on('afterSendEvent', event => { + if (!event.type) { + Sentry.showReportDialog({ ...this._options.dialogOptions, eventId: event.event_id }); + } + }); + + // We only want to register this hook once in the lifetime of the error handler + this._registeredAfterSendEventHandler = true; + } else if (!client || !client.on) { + Sentry.showReportDialog({ ...this._options.dialogOptions, eventId }); + } } } diff --git a/packages/angular/test/errorhandler.test.ts b/packages/angular/test/errorhandler.test.ts index fadae7d9b2de..3a61785cfa69 100644 --- a/packages/angular/test/errorhandler.test.ts +++ b/packages/angular/test/errorhandler.test.ts @@ -524,9 +524,11 @@ describe('SentryErrorHandler', () => { }); describe('opens the report dialog if `showDialog` is true', () => { - it('by using SDK lifecycle hooks if available', () => { + it.only('by using SDK lifecycle hooks if available', () => { const client = { - cb: (_: Event) => {}, + cb: (_: Event) => { + console.log('wat'); + }, on: jest.fn((_, cb) => { client.cb = cb; }), @@ -534,14 +536,16 @@ describe('SentryErrorHandler', () => { // @ts-ignore this is a minmal hub, we're missing a few props but that's ok jest.spyOn(SentryBrowser, 'getCurrentHub').mockImplementationOnce(() => { + console.log('getcurrenthub'); + return { getClient: () => client }; }); const showReportDialogSpy = jest.spyOn(SentryBrowser, 'showReportDialog'); const errorHandler = createErrorHandler({ showDialog: true }); - expect(client.on).toHaveBeenCalledWith('afterSendEvent', expect.any(Function)); errorHandler.handleError(new Error('test')); + expect(client.on).toHaveBeenCalledWith('afterSendEvent', expect.any(Function)); // this simulates the afterSend hook being called client.cb({}); From fd40d12e461f05df394d3fd2d8ff6afca4b15aab Mon Sep 17 00:00:00 2001 From: Lukas Stracke Date: Wed, 5 Apr 2023 13:04:47 +0200 Subject: [PATCH 3/3] fix lint error --- packages/angular/test/errorhandler.test.ts | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/packages/angular/test/errorhandler.test.ts b/packages/angular/test/errorhandler.test.ts index 3a61785cfa69..c43ad41629c1 100644 --- a/packages/angular/test/errorhandler.test.ts +++ b/packages/angular/test/errorhandler.test.ts @@ -524,11 +524,9 @@ describe('SentryErrorHandler', () => { }); describe('opens the report dialog if `showDialog` is true', () => { - it.only('by using SDK lifecycle hooks if available', () => { + it('by using SDK lifecycle hooks if available', () => { const client = { - cb: (_: Event) => { - console.log('wat'); - }, + cb: (_: Event) => {}, on: jest.fn((_, cb) => { client.cb = cb; }), @@ -536,8 +534,6 @@ describe('SentryErrorHandler', () => { // @ts-ignore this is a minmal hub, we're missing a few props but that's ok jest.spyOn(SentryBrowser, 'getCurrentHub').mockImplementationOnce(() => { - console.log('getcurrenthub'); - return { getClient: () => client }; });