From 389f4eee498e43edf1baf994c649f489881ac10f Mon Sep 17 00:00:00 2001 From: Abhijeet Prasad Date: Thu, 28 Apr 2022 10:23:36 -0400 Subject: [PATCH] ref(browser): Remove showReportDialog on browser client (#4973) Remove `showReportDialog` as a client method so it can be tree-shaken out if not used. Also allow for hub to be explicitly passed in to `showReportDialog`. --- packages/browser/src/client.ts | 26 -------------- packages/browser/src/exports.ts | 1 - packages/browser/src/helpers.ts | 42 +--------------------- packages/browser/src/sdk.ts | 46 +++++++++++++++++++----- packages/browser/test/unit/index.test.ts | 34 ++++++++++++------ packages/core/src/api.ts | 11 +++--- 6 files changed, 68 insertions(+), 92 deletions(-) diff --git a/packages/browser/src/client.ts b/packages/browser/src/client.ts index d57b35c02d31..004f1359ffab 100644 --- a/packages/browser/src/client.ts +++ b/packages/browser/src/client.ts @@ -1,10 +1,7 @@ import { BaseClient, Scope, SDK_VERSION } from '@sentry/core'; import { ClientOptions, Event, EventHint, Options, Severity, SeverityLevel } from '@sentry/types'; -import { getGlobalObject, logger } from '@sentry/utils'; import { eventFromException, eventFromMessage } from './eventbuilder'; -import { IS_DEBUG_BUILD } from './flags'; -import { injectReportDialog, ReportDialogOptions } from './helpers'; import { Breadcrumbs } from './integrations'; export interface BaseBrowserOptions { @@ -62,29 +59,6 @@ export class BrowserClient extends BaseClient { super(options); } - /** - * Show a report dialog to the user to send feedback to a specific event. - * - * @param options Set individual options for the dialog - */ - public showReportDialog(options: ReportDialogOptions = {}): void { - // doesn't work without a document (React Native) - const document = getGlobalObject().document; - if (!document) { - return; - } - - if (!this._isEnabled()) { - IS_DEBUG_BUILD && logger.error('Trying to call showReportDialog with Sentry Client disabled'); - return; - } - - injectReportDialog({ - ...options, - dsn: options.dsn || this.getDsn(), - }); - } - /** * @inheritDoc */ diff --git a/packages/browser/src/exports.ts b/packages/browser/src/exports.ts index 4a1dc5837747..93a45d353ea9 100644 --- a/packages/browser/src/exports.ts +++ b/packages/browser/src/exports.ts @@ -52,6 +52,5 @@ export { opera11StackParser, winjsStackParser, } from './stack-parsers'; -export { injectReportDialog } from './helpers'; export { defaultIntegrations, forceLoad, init, lastEventId, onLoad, showReportDialog, flush, close, wrap } from './sdk'; export { SDK_NAME } from './version'; diff --git a/packages/browser/src/helpers.ts b/packages/browser/src/helpers.ts index d0b3f8f5c9aa..9bf1d5631288 100644 --- a/packages/browser/src/helpers.ts +++ b/packages/browser/src/helpers.ts @@ -1,18 +1,13 @@ -import { captureException, getReportDialogEndpoint, withScope } from '@sentry/core'; +import { captureException, withScope } from '@sentry/core'; import { DsnLike, Event as SentryEvent, Mechanism, Scope, WrappedFunction } from '@sentry/types'; import { addExceptionMechanism, addExceptionTypeValue, addNonEnumerableProperty, - getGlobalObject, getOriginalFunction, - logger, markFunctionWrapped, } from '@sentry/utils'; -import { IS_DEBUG_BUILD } from './flags'; - -const global = getGlobalObject(); let ignoreOnError: number = 0; /** @@ -182,38 +177,3 @@ export interface ReportDialogOptions { /** Callback after reportDialog showed up */ onLoad?(): void; } - -/** - * Injects the Report Dialog script - * @hidden - */ -export function injectReportDialog(options: ReportDialogOptions = {}): void { - if (!global.document) { - return; - } - - if (!options.eventId) { - IS_DEBUG_BUILD && logger.error('Missing eventId option in showReportDialog call'); - return; - } - - if (!options.dsn) { - IS_DEBUG_BUILD && logger.error('Missing dsn option in showReportDialog call'); - return; - } - - const script = global.document.createElement('script'); - script.async = true; - script.src = getReportDialogEndpoint(options.dsn, options); - - if (options.onLoad) { - // eslint-disable-next-line @typescript-eslint/unbound-method - script.onload = options.onLoad; - } - - const injectionPoint = global.document.head || global.document.body; - - if (injectionPoint) { - injectionPoint.appendChild(script); - } -} diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts index 184893f15cf0..3740a1318f0a 100644 --- a/packages/browser/src/sdk.ts +++ b/packages/browser/src/sdk.ts @@ -1,5 +1,11 @@ -import { getCurrentHub, getIntegrationsToSetup, initAndBind, Integrations as CoreIntegrations } from '@sentry/core'; -import { Hub } from '@sentry/types'; +import { + getCurrentHub, + getIntegrationsToSetup, + getReportDialogEndpoint, + Hub, + initAndBind, + Integrations as CoreIntegrations, +} from '@sentry/core'; import { addInstrumentationHandler, getGlobalObject, @@ -121,9 +127,21 @@ export function init(options: BrowserOptions = {}): void { * * @param options Everything is optional, we try to fetch all info need from the global scope. */ -export function showReportDialog(options: ReportDialogOptions = {}): void { - const hub = getCurrentHub(); - const scope = hub.getScope(); +export function showReportDialog(options: ReportDialogOptions = {}, hub: Hub = getCurrentHub()): void { + // doesn't work without a document (React Native) + const global = getGlobalObject(); + if (!global.document) { + IS_DEBUG_BUILD && logger.error('Global document not defined in showReportDialog call'); + return; + } + + const { client, scope } = hub.getStackTop(); + const dsn = options.dsn || (client && client.getDsn()); + if (!dsn) { + IS_DEBUG_BUILD && logger.error('DSN not configured for showReportDialog call'); + return; + } + if (scope) { options.user = { ...scope.getUser(), @@ -134,9 +152,21 @@ export function showReportDialog(options: ReportDialogOptions = {}): void { if (!options.eventId) { options.eventId = hub.lastEventId(); } - const client = hub.getClient(); - if (client) { - client.showReportDialog(options); + + const script = global.document.createElement('script'); + script.async = true; + script.src = getReportDialogEndpoint(dsn, options); + + if (options.onLoad) { + // eslint-disable-next-line @typescript-eslint/unbound-method + script.onload = options.onLoad; + } + + const injectionPoint = global.document.head || global.document.body; + if (injectionPoint) { + injectionPoint.appendChild(script); + } else { + IS_DEBUG_BUILD && logger.error('Not injecting report dialog. No injection point found in HTML'); } } diff --git a/packages/browser/test/unit/index.test.ts b/packages/browser/test/unit/index.test.ts index c8d1df23bac9..37fa972512a1 100644 --- a/packages/browser/test/unit/index.test.ts +++ b/packages/browser/test/unit/index.test.ts @@ -1,4 +1,4 @@ -import { SDK_VERSION } from '@sentry/core'; +import { getReportDialogEndpoint, SDK_VERSION } from '@sentry/core'; import { addBreadcrumb, @@ -24,6 +24,14 @@ const dsn = 'https://53039209a22b4ec1bcc296a3c9fdecd6@sentry.io/4291'; // eslint-disable-next-line no-var declare var global: any; +jest.mock('@sentry/core', () => { + const original = jest.requireActual('@sentry/core'); + return { + ...original, + getReportDialogEndpoint: jest.fn(), + }; +}); + describe('SentryBrowser', () => { const beforeSend = jest.fn(); @@ -74,16 +82,14 @@ describe('SentryBrowser', () => { }); describe('showReportDialog', () => { + beforeEach(() => { + (getReportDialogEndpoint as jest.Mock).mockReset(); + }); + describe('user', () => { const EX_USER = { email: 'test@example.com' }; const options = getDefaultBrowserClientOptions({ dsn }); const client = new BrowserClient(options); - const reportDialogSpy = jest.spyOn(client, 'showReportDialog'); - - beforeEach(() => { - reportDialogSpy.mockReset(); - }); - it('uses the user on the scope', () => { configureScope(scope => { scope.setUser(EX_USER); @@ -92,8 +98,11 @@ describe('SentryBrowser', () => { showReportDialog(); - expect(reportDialogSpy).toBeCalled(); - expect(reportDialogSpy.mock.calls[0][0]!.user!.email).toBe(EX_USER.email); + expect(getReportDialogEndpoint).toHaveBeenCalledTimes(1); + expect(getReportDialogEndpoint).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ user: { email: EX_USER.email } }), + ); }); it('prioritizes options user over scope user', () => { @@ -105,8 +114,11 @@ describe('SentryBrowser', () => { const DIALOG_OPTION_USER = { email: 'option@example.com' }; showReportDialog({ user: DIALOG_OPTION_USER }); - expect(reportDialogSpy).toBeCalled(); - expect(reportDialogSpy.mock.calls[0][0]!.user!.email).toBe(DIALOG_OPTION_USER.email); + expect(getReportDialogEndpoint).toHaveBeenCalledTimes(1); + expect(getReportDialogEndpoint).toHaveBeenCalledWith( + expect.any(Object), + expect.objectContaining({ user: { email: DIALOG_OPTION_USER.email } }), + ); }); }); }); diff --git a/packages/core/src/api.ts b/packages/core/src/api.ts index dced7a8b181e..31375593fd4a 100644 --- a/packages/core/src/api.ts +++ b/packages/core/src/api.ts @@ -53,14 +53,15 @@ export function getReportDialogEndpoint( } if (key === 'user') { - if (!dialogOptions.user) { + const user = dialogOptions.user; + if (!user) { continue; } - if (dialogOptions.user.name) { - encodedOptions += `&name=${encodeURIComponent(dialogOptions.user.name)}`; + if (user.name) { + encodedOptions += `&name=${encodeURIComponent(user.name)}`; } - if (dialogOptions.user.email) { - encodedOptions += `&email=${encodeURIComponent(dialogOptions.user.email)}`; + if (user.email) { + encodedOptions += `&email=${encodeURIComponent(user.email)}`; } } else { encodedOptions += `&${encodeURIComponent(key)}=${encodeURIComponent(dialogOptions[key] as string)}`;