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`.
getsentry · Apr 28, 2022 · 389f4ee · 389f4ee
1 parent de2822b
commit 389f4ee
Show file tree

Hide file tree

Showing 6 changed files with 68 additions and 92 deletions.
diff --git 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<BrowserClientOptions> {
     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<Window>().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
@@ -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
@@ -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<Window>();
 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
@@ -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<Window>();
+  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<BrowserClient>();
-  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
@@ -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
@@ -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)}`;