less mutation on options; review comments

getsentry · Feb 29, 2024 · 0a6730a · 0a6730a
1 parent dbb42d4
commit 0a6730a
Show file tree

Hide file tree

Showing 4 changed files with 78 additions and 77 deletions.
diff --git a/...ckages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts b/...ckages/browser-integration-tests/suites/manual-client/skip-init-browser-extension/test.ts
@@ -1,31 +1,34 @@
 import { expect } from '@playwright/test';
 import { sentryTest } from '../../../utils/fixtures';
 
-sentryTest('should not initialize when inside a Chrome browser extension', async ({ getLocalTestUrl, page }) => {
-  await page.route('https://dsn.ingest.sentry.io/**/*', route => {
-    return route.fulfill({
-      status: 200,
-      contentType: 'application/json',
-      body: JSON.stringify({ id: 'test-id' }),
+sentryTest(
+  'should not initialize when inside a Firefox/Safari browser extension',
+  async ({ getLocalTestUrl, page }) => {
+    await page.route('https://dsn.ingest.sentry.io/**/*', route => {
+      return route.fulfill({
+        status: 200,
+        contentType: 'application/json',
+        body: JSON.stringify({ id: 'test-id' }),
+      });
     });
-  });
 
-  const errorLogs: string[] = [];
+    const errorLogs: string[] = [];
 
-  page.on('console', message => {
-    if (message.type() === 'error') errorLogs.push(message.text());
-  });
+    page.on('console', message => {
+      if (message.type() === 'error') errorLogs.push(message.text());
+    });
 
-  const url = await getLocalTestUrl({ testDir: __dirname });
-  await page.goto(url);
+    const url = await getLocalTestUrl({ testDir: __dirname });
+    await page.goto(url);
 
-  const isInitialized = await page.evaluate(() => {
-    return !!(window as any).Sentry.isInitialized();
-  });
+    const isInitialized = await page.evaluate(() => {
+      return !!(window as any).Sentry.isInitialized();
+    });
 
-  expect(isInitialized).toEqual(false);
-  expect(errorLogs.length).toEqual(1);
-  expect(errorLogs[0]).toEqual(
-    '[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
-  );
-});
+    expect(isInitialized).toEqual(false);
+    expect(errorLogs.length).toEqual(1);
+    expect(errorLogs[0]).toEqual(
+      '[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
+    );
+  },
+);
diff --git a/...ackages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts b/...ackages/browser-integration-tests/suites/manual-client/skip-init-chrome-extension/test.ts
@@ -26,6 +26,6 @@ sentryTest('should not initialize when inside a Chrome browser extension', async
   expect(isInitialized).toEqual(false);
   expect(errorLogs.length).toEqual(1);
   expect(errorLogs[0]).toEqual(
-    '[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
+    '[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
   );
 });
diff --git a/packages/browser/src/sdk.ts b/packages/browser/src/sdk.ts
@@ -44,6 +44,40 @@ export function getDefaultIntegrations(_options: Options): Integration[] {
   ];
 }
 
+function applyDefaultOptions(options: BrowserOptions = {}): BrowserOptions {
+  const defaultOptions: BrowserOptions = {
+    defaultIntegrations: getDefaultIntegrations(options),
+    release:
+      typeof __SENTRY_RELEASE__ === 'string' // This allows build tooling to find-and-replace __SENTRY_RELEASE__ to inject a release value
+        ? __SENTRY_RELEASE__
+        : WINDOW.SENTRY_RELEASE && WINDOW.SENTRY_RELEASE.id // This supports the variable that sentry-webpack-plugin injects
+          ? WINDOW.SENTRY_RELEASE.id
+          : undefined,
+    autoSessionTracking: true,
+    sendClientReports: true,
+  };
+
+  return { ...defaultOptions, ...options };
+}
+
+function shouldShowBrowserExtensionError(): boolean {
+  const windowWithMaybeChrome = WINDOW as typeof WINDOW & { chrome?: { runtime?: { id?: string } } };
+  const isInsideChromeExtension =
+    windowWithMaybeChrome &&
+    windowWithMaybeChrome.chrome &&
+    windowWithMaybeChrome.chrome.runtime &&
+    windowWithMaybeChrome.chrome.runtime.id;
+
+  const windowWithMaybeBrowser = WINDOW as typeof WINDOW & { browser?: { runtime?: { id?: string } } };
+  const isInsideBrowserExtension =
+    windowWithMaybeBrowser &&
+    windowWithMaybeBrowser.browser &&
+    windowWithMaybeBrowser.browser.runtime &&
+    windowWithMaybeBrowser.browser.runtime.id;
+
+  return !!isInsideBrowserExtension || !!isInsideChromeExtension;
+}
+
 /**
  * A magic string that build tooling can leverage in order to inject a release value into the SDK.
  */
@@ -95,48 +129,14 @@ declare const __SENTRY_RELEASE__: string | undefined;
  *
  * @see {@link BrowserOptions} for documentation on configuration options.
  */
-export function init(options: BrowserOptions = {}): void {
-  if (options.defaultIntegrations === undefined) {
-    options.defaultIntegrations = getDefaultIntegrations(options);
-  }
-  if (options.release === undefined) {
-    // This allows build tooling to find-and-replace __SENTRY_RELEASE__ to inject a release value
-    if (typeof __SENTRY_RELEASE__ === 'string') {
-      options.release = __SENTRY_RELEASE__;
-    }
-
-    // This supports the variable that sentry-webpack-plugin injects
-    if (WINDOW.SENTRY_RELEASE && WINDOW.SENTRY_RELEASE.id) {
-      options.release = WINDOW.SENTRY_RELEASE.id;
-    }
-  }
-  if (options.autoSessionTracking === undefined) {
-    options.autoSessionTracking = true;
-  }
-  if (options.sendClientReports === undefined) {
-    options.sendClientReports = true;
-  }
-
-  // inside Chrome extension (chrome.* API)
-  const windowWithMaybeChrome = WINDOW as typeof WINDOW & { chrome?: { runtime?: { id?: string } } };
-
-  // inside Firefox/Safari extension (browser.* API)
-  const windowWithMaybeBrowser = WINDOW as typeof WINDOW & { browser?: { runtime?: { id?: string } } };
+export function init(browserOptions: BrowserOptions = {}): void {
+  const options = applyDefaultOptions(browserOptions);
 
-  if (
-    (windowWithMaybeBrowser &&
-      windowWithMaybeBrowser.browser &&
-      windowWithMaybeBrowser.browser.runtime &&
-      windowWithMaybeBrowser.browser.runtime.id) ||
-    (windowWithMaybeChrome &&
-      windowWithMaybeChrome.chrome &&
-      windowWithMaybeChrome.chrome.runtime &&
-      windowWithMaybeChrome.chrome.runtime.id)
-  ) {
+  if (shouldShowBrowserExtensionError()) {
     consoleSandbox(() => {
       // eslint-disable-next-line no-console
       console.error(
-        '[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
+        '[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
       );
     });
     return;

diff --git a/packages/browser/test/unit/sdk.test.ts b/packages/browser/test/unit/sdk.test.ts
@@ -137,14 +137,9 @@ describe('init', () => {
 
     const options = getDefaultBrowserOptions({ dsn: PUBLIC_DSN, defaultIntegrations: DEFAULT_INTEGRATIONS });
 
-    it('should not log a browser extension error if executed inside regular browser environment', () => {
-      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
-
-      init(options);
-
-      expect(consoleErrorSpy).toBeCalledTimes(0);
-
-      consoleErrorSpy.mockRestore();
+    afterEach(() => {
+      Object.defineProperty(WINDOW, 'chrome', { value: undefined, writable: true });
+      Object.defineProperty(WINDOW, 'browser', { value: undefined, writable: true });
     });
 
     it('should log a browser extension error if executed inside a Chrome extension', () => {
@@ -159,15 +154,10 @@ describe('init', () => {
 
       expect(consoleErrorSpy).toBeCalledTimes(1);
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        '[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
+        '[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
       );
 
       consoleErrorSpy.mockRestore();
-
-      Object.defineProperty(WINDOW, 'chrome', {
-        value: null,
-        writable: true,
-      });
     });
 
     it('should log a browser extension error if executed inside a Firefox/Safari extension', () => {
@@ -179,12 +169,20 @@ describe('init', () => {
 
       expect(consoleErrorSpy).toBeCalledTimes(1);
       expect(consoleErrorSpy).toHaveBeenCalledWith(
-        '[Sentry] You cannot run Sentry this way in an extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
+        '[Sentry] You cannot run Sentry this way in a browser extension, check: https://docs.sentry.io/platforms/javascript/troubleshooting/#setting-up-sentry-in-shared-environments-eg-browser-extensions',
       );
 
       consoleErrorSpy.mockRestore();
+    });
+
+    it('should not log a browser extension error if executed inside regular browser environment', () => {
+      const consoleErrorSpy = jest.spyOn(console, 'error').mockImplementation(() => {});
 
-      Object.defineProperty(WINDOW, 'browser', { value: null, writable: true });
+      init(options);
+
+      expect(consoleErrorSpy).toBeCalledTimes(0);
+
+      consoleErrorSpy.mockRestore();
     });
   });
 });