fix(utils): Prevent logger circular dependencies (#4069)

This aims to prevent the `logger` -> `misc` -> `logger` dependency cycle from ever happening again. - Move `getGlobalObject` to its own module. - Move `consoleSandbox` into the logger module itself, since that's the only place it's used. - Given how easy it would be to recreate this problem - just add to `logger.ts`'s dependencies (not knowing that that's what they are) any function which logs to the console using the logger - add a warning comment at the top of both dependencies. As a bonus, move `getLocationHref` into the browser module, a move which was missed in an earlier rearrangement.
getsentry · Oct 15, 2021 · 84a6dc0 · 84a6dc0
1 parent f44d370
commit 84a6dc0
Show file tree

Hide file tree

Showing 11 changed files with 128 additions and 108 deletions.
diff --git a/packages/utils/src/browser.ts b/packages/utils/src/browser.ts
@@ -1,3 +1,4 @@
+import { getGlobalObject } from './global';
 import { isString } from './is';
 
 /**
@@ -108,3 +109,15 @@ function _htmlElementAsString(el: unknown, keyAttrs?: string[]): string {
   }
   return out.join('');
 }
+
+/**
+ * A safe form of location.href
+ */
+export function getLocationHref(): string {
+  const global = getGlobalObject<Window>();
+  try {
+    return global.document.location.href;
+  } catch (oO) {
+    return '';
+  }
+}
diff --git a/packages/utils/src/global.ts b/packages/utils/src/global.ts
@@ -0,0 +1,44 @@
+/**
+ * NOTE: In order to avoid circular dependencies, if you add a function to this module and it needs to print something,
+ * you must either a) use `console.log` rather than the logger, or b) put your function elsewhere.
+ */
+
+/* eslint-disable @typescript-eslint/no-explicit-any */
+
+import { Integration } from '@sentry/types';
+
+import { isNodeEnv } from './node';
+
+/** Internal */
+interface SentryGlobal {
+  Sentry?: {
+    Integrations?: Integration[];
+  };
+  SENTRY_ENVIRONMENT?: string;
+  SENTRY_DSN?: string;
+  SENTRY_RELEASE?: {
+    id?: string;
+  };
+  __SENTRY__: {
+    globalEventProcessors: any;
+    hub: any;
+    logger: any;
+  };
+}
+
+const fallbackGlobalObject = {};
+
+/**
+ * Safely get global scope object
+ *
+ * @returns Global scope object
+ */
+export function getGlobalObject<T>(): T & SentryGlobal {
+  return (isNodeEnv()
+    ? global
+    : typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
+    ? window // eslint-disable-line no-restricted-globals
+    : typeof self !== 'undefined'
+    ? self
+    : fallbackGlobalObject) as T & SentryGlobal;
+}
diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts
@@ -2,6 +2,7 @@ export * from './async';
 export * from './browser';
 export * from './dsn';
 export * from './error';
+export * from './global';
 export * from './instrument';
 export * from './is';
 export * from './logger';

diff --git a/packages/utils/src/instrument.ts b/packages/utils/src/instrument.ts
@@ -2,9 +2,9 @@
 /* eslint-disable @typescript-eslint/ban-types */
 import { WrappedFunction } from '@sentry/types';
 
+import { getGlobalObject } from './global';
 import { isInstanceOf, isString } from './is';
 import { logger } from './logger';
-import { getGlobalObject } from './misc';
 import { fill } from './object';
 import { getFunctionName } from './stacktrace';
 import { supportsHistory, supportsNativeFetch } from './supports';

diff --git a/packages/utils/src/logger.ts b/packages/utils/src/logger.ts
@@ -1,12 +1,58 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import { consoleSandbox, getGlobalObject } from './misc';
+import { WrappedFunction } from '@sentry/types';
+
+import { getGlobalObject } from './global';
 
 // TODO: Implement different loggers for different environments
 const global = getGlobalObject<Window | NodeJS.Global>();
 
 /** Prefix for logging strings */
 const PREFIX = 'Sentry Logger ';
 
+/** JSDoc */
+interface ExtensibleConsole extends Console {
+  [key: string]: any;
+}
+
+/**
+ * Temporarily unwrap `console.log` and friends in order to perform the given callback using the original methods.
+ * Restores wrapping after the callback completes.
+ *
+ * @param callback The function to run against the original `console` messages
+ * @returns The results of the callback
+ */
+export function consoleSandbox(callback: () => any): any {
+  const global = getGlobalObject<Window>();
+  const levels = ['debug', 'info', 'warn', 'error', 'log', 'assert'];
+
+  if (!('console' in global)) {
+    return callback();
+  }
+
+  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+  const originalConsole = (global as any).console as ExtensibleConsole;
+  const wrappedLevels: { [key: string]: any } = {};
+
+  // Restore all wrapped console methods
+  levels.forEach(level => {
+    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
+    if (level in (global as any).console && (originalConsole[level] as WrappedFunction).__sentry_original__) {
+      wrappedLevels[level] = originalConsole[level] as WrappedFunction;
+      originalConsole[level] = (originalConsole[level] as WrappedFunction).__sentry_original__;
+    }
+  });
+
+  // Perform callback manipulations
+  const result = callback();
+
+  // Revert restoration to wrapped state
+  Object.keys(wrappedLevels).forEach(level => {
+    originalConsole[level] = wrappedLevels[level];
+  });
+
+  return result;
+}
+
 /** JSDoc */
 class Logger {
   /** JSDoc */

diff --git a/packages/utils/src/misc.ts b/packages/utils/src/misc.ts
@@ -1,43 +1,9 @@
 /* eslint-disable @typescript-eslint/no-explicit-any */
-import { Event, Integration, StackFrame, WrappedFunction } from '@sentry/types';
+import { Event, StackFrame } from '@sentry/types';
 
-import { isNodeEnv } from './node';
+import { getGlobalObject } from './global';
 import { snipLine } from './string';
 
-/** Internal */
-interface SentryGlobal {
-  Sentry?: {
-    Integrations?: Integration[];
-  };
-  SENTRY_ENVIRONMENT?: string;
-  SENTRY_DSN?: string;
-  SENTRY_RELEASE?: {
-    id?: string;
-  };
-  __SENTRY__: {
-    globalEventProcessors: any;
-    hub: any;
-    logger: any;
-  };
-}
-
-const fallbackGlobalObject = {};
-
-/**
- * Safely get global scope object
- *
- * @returns Global scope object
- */
-export function getGlobalObject<T>(): T & SentryGlobal {
-  return (isNodeEnv()
-    ? global
-    : typeof window !== 'undefined' // eslint-disable-line no-restricted-globals
-    ? window // eslint-disable-line no-restricted-globals
-    : typeof self !== 'undefined'
-    ? self
-    : fallbackGlobalObject) as T & SentryGlobal;
-}
-
 /**
  * Extended Window interface that allows for Crypto API usage in IE browsers
  */
@@ -143,44 +109,6 @@ export function getEventDescription(event: Event): string {
   return event.event_id || '<unknown>';
 }
 
-/** JSDoc */
-interface ExtensibleConsole extends Console {
-  [key: string]: any;
-}
-
-/** JSDoc */
-export function consoleSandbox(callback: () => any): any {
-  const global = getGlobalObject<Window>();
-  const levels = ['debug', 'info', 'warn', 'error', 'log', 'assert'];
-
-  if (!('console' in global)) {
-    return callback();
-  }
-
-  // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-  const originalConsole = (global as any).console as ExtensibleConsole;
-  const wrappedLevels: { [key: string]: any } = {};
-
-  // Restore all wrapped console methods
-  levels.forEach(level => {
-    // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-    if (level in (global as any).console && (originalConsole[level] as WrappedFunction).__sentry_original__) {
-      wrappedLevels[level] = originalConsole[level] as WrappedFunction;
-      originalConsole[level] = (originalConsole[level] as WrappedFunction).__sentry_original__;
-    }
-  });
-
-  // Perform callback manipulations
-  const result = callback();
-
-  // Revert restoration to wrapped state
-  Object.keys(wrappedLevels).forEach(level => {
-    originalConsole[level] = wrappedLevels[level];
-  });
-
-  return result;
-}
-
 /**
  * Adds exception values, type and value to an synthetic Exception.
  * @param event The event to modify.
@@ -223,18 +151,6 @@ export function addExceptionMechanism(
   }
 }
 
-/**
- * A safe form of location.href
- */
-export function getLocationHref(): string {
-  const global = getGlobalObject<Window>();
-  try {
-    return global.document.location.href;
-  } catch (oO) {
-    return '';
-  }
-}
-
 // https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
 const SEMVER_REGEXP = /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$/;
 

diff --git a/packages/utils/src/node.ts b/packages/utils/src/node.ts
@@ -1,3 +1,8 @@
+/**
+ * NOTE: In order to avoid circular dependencies, if you add a function to this module and it needs to print something,
+ * you must either a) use `console.log` rather than the logger, or b) put your function elsewhere.
+ */
+
 /**
  * Checks whether we're in the Node.js or Browser environment
  *

diff --git a/packages/utils/src/supports.ts b/packages/utils/src/supports.ts
@@ -1,5 +1,5 @@
+import { getGlobalObject } from './global';
 import { logger } from './logger';
-import { getGlobalObject } from './misc';
 
 /**
  * Tells whether current environment supports ErrorEvent objects

diff --git a/packages/utils/src/time.ts b/packages/utils/src/time.ts
@@ -1,4 +1,4 @@
-import { getGlobalObject } from './misc';
+import { getGlobalObject } from './global';
 import { dynamicRequire, isNodeEnv } from './node';
 
 /**

diff --git a/packages/utils/test/global.test.ts b/packages/utils/test/global.test.ts
@@ -0,0 +1,12 @@
+import { getGlobalObject } from '../src/global';
+
+describe('getGlobalObject()', () => {
+  test('should return the same object', () => {
+    const backup = global.process;
+    delete global.process;
+    const first = getGlobalObject();
+    const second = getGlobalObject();
+    expect(first).toEqual(second);
+    global.process = backup;
+  });
+});
diff --git a/packages/utils/test/misc.test.ts b/packages/utils/test/misc.test.ts
@@ -1,12 +1,6 @@
 import { StackFrame } from '@sentry/types';
 
-import {
-  addContextToFrame,
-  getEventDescription,
-  getGlobalObject,
-  parseRetryAfterHeader,
-  stripUrlQueryAndFragment,
-} from '../src/misc';
+import { addContextToFrame, getEventDescription, parseRetryAfterHeader, stripUrlQueryAndFragment } from '../src/misc';
 
 describe('getEventDescription()', () => {
   test('message event', () => {
@@ -117,17 +111,6 @@ describe('getEventDescription()', () => {
   });
 });
 
-describe('getGlobalObject()', () => {
-  test('should return the same object', () => {
-    const backup = global.process;
-    delete global.process;
-    const first = getGlobalObject();
-    const second = getGlobalObject();
-    expect(first).toEqual(second);
-    global.process = backup;
-  });
-});
-
 describe('parseRetryAfterHeader', () => {
   test('no header', () => {
     expect(parseRetryAfterHeader(Date.now())).toEqual(60 * 1000);