From c82b4e8d27f011daa3d44417d7c86bc45effc384 Mon Sep 17 00:00:00 2001 From: Francesco Novy Date: Thu, 10 Jul 2025 16:10:15 +0200 Subject: [PATCH] ref(core): Rewrite `logger` to avoid side effects --- packages/core/src/carrier.ts | 2 + packages/core/src/utils/logger.ts | 152 ++++++++++++++------ packages/core/test/clear-global-scope.ts | 0 packages/core/test/lib/utils/logger.test.ts | 57 ++++++++ 4 files changed, 165 insertions(+), 46 deletions(-) delete mode 100644 packages/core/test/clear-global-scope.ts create mode 100644 packages/core/test/lib/utils/logger.test.ts diff --git a/packages/core/src/carrier.ts b/packages/core/src/carrier.ts index d11ef2940d0c..31827ad7b87e 100644 --- a/packages/core/src/carrier.ts +++ b/packages/core/src/carrier.ts @@ -24,7 +24,9 @@ export interface SentryCarrier { globalScope?: Scope; defaultIsolationScope?: Scope; defaultCurrentScope?: Scope; + /** @deprecated Logger is no longer set. Instead, we keep enabled state in loggerSettings. */ logger?: Logger; + loggerSettings?: { enabled: boolean }; /** Overwrites TextEncoder used in `@sentry/core`, need for `react-native@0.73` and older */ encodePolyfill?: (input: string) => Uint8Array; diff --git a/packages/core/src/utils/logger.ts b/packages/core/src/utils/logger.ts index 669917dfbadf..d17f67f6f6e6 100644 --- a/packages/core/src/utils/logger.ts +++ b/packages/core/src/utils/logger.ts @@ -3,8 +3,19 @@ import { DEBUG_BUILD } from '../debug-build'; import type { ConsoleLevel } from '../types-hoist/instrument'; import { GLOBAL_OBJ } from './worldwide'; -/** Prefix for logging strings */ -const PREFIX = 'Sentry Logger '; +/** A Sentry Logger instance. */ +export interface Logger { + disable(): void; + enable(): void; + isEnabled(): boolean; + log(...args: Parameters): void; + info(...args: Parameters): void; + warn(...args: Parameters): void; + error(...args: Parameters): void; + debug(...args: Parameters): void; + assert(...args: Parameters): void; + trace(...args: Parameters): void; +} export const CONSOLE_LEVELS: readonly ConsoleLevel[] = [ 'debug', @@ -16,20 +27,19 @@ export const CONSOLE_LEVELS: readonly ConsoleLevel[] = [ 'trace', ] as const; -type LoggerMethod = (...args: unknown[]) => void; -type LoggerConsoleMethods = Record; +/** Prefix for logging strings */ +const PREFIX = 'Sentry Logger '; /** This may be mutated by the console instrumentation. */ -export const originalConsoleMethods: { - [key in ConsoleLevel]?: (...args: unknown[]) => void; -} = {}; - -/** A Sentry Logger instance. */ -export interface Logger extends LoggerConsoleMethods { - disable(): void; - enable(): void; - isEnabled(): boolean; -} +export const originalConsoleMethods: Partial<{ + log(...args: Parameters): void; + info(...args: Parameters): void; + warn(...args: Parameters): void; + error(...args: Parameters): void; + debug(...args: Parameters): void; + assert(...args: Parameters): void; + trace(...args: Parameters): void; +}> = {}; /** * Temporarily disable sentry console instrumentations. @@ -43,15 +53,15 @@ export function consoleSandbox(callback: () => T): T { } const console = GLOBAL_OBJ.console as Console; - const wrappedFuncs: Partial = {}; + const wrappedFuncs: Partial void>> = {}; const wrappedLevels = Object.keys(originalConsoleMethods) as ConsoleLevel[]; // Restore all wrapped console methods wrappedLevels.forEach(level => { - const originalConsoleMethod = originalConsoleMethods[level] as LoggerMethod; - wrappedFuncs[level] = console[level] as LoggerMethod | undefined; - console[level] = originalConsoleMethod; + const originalConsoleMethod = originalConsoleMethods[level]; + wrappedFuncs[level] = console[level] as (...args: unknown[]) => void; + console[level] = originalConsoleMethod as (...args: unknown[]) => void; }); try { @@ -59,44 +69,94 @@ export function consoleSandbox(callback: () => T): T { } finally { // Revert restoration to wrapped state wrappedLevels.forEach(level => { - console[level] = wrappedFuncs[level] as LoggerMethod; + console[level] = wrappedFuncs[level] as (...args: unknown[]) => void; }); } } -function makeLogger(): Logger { - let enabled = false; - const logger: Partial = { - enable: () => { - enabled = true; - }, - disable: () => { - enabled = false; - }, - isEnabled: () => enabled, - }; - - if (DEBUG_BUILD) { - CONSOLE_LEVELS.forEach(name => { - logger[name] = (...args: Parameters<(typeof GLOBAL_OBJ.console)[typeof name]>) => { - if (enabled) { - consoleSandbox(() => { - GLOBAL_OBJ.console[name](`${PREFIX}[${name}]:`, ...args); - }); - } - }; - }); - } else { - CONSOLE_LEVELS.forEach(name => { - logger[name] = () => undefined; +function enable(): void { + _getLoggerSettings().enabled = true; +} + +function disable(): void { + _getLoggerSettings().enabled = false; +} + +function isEnabled(): boolean { + return _getLoggerSettings().enabled; +} + +function log(...args: Parameters): void { + _maybeLog('log', ...args); +} + +function info(...args: Parameters): void { + _maybeLog('info', ...args); +} + +function warn(...args: Parameters): void { + _maybeLog('warn', ...args); +} + +function error(...args: Parameters): void { + _maybeLog('error', ...args); +} + +function debug(...args: Parameters): void { + _maybeLog('debug', ...args); +} + +function assert(...args: Parameters): void { + _maybeLog('assert', ...args); +} + +function trace(...args: Parameters): void { + _maybeLog('trace', ...args); +} + +function _maybeLog(level: ConsoleLevel, ...args: Parameters<(typeof console)[typeof level]>): void { + if (!DEBUG_BUILD) { + return; + } + + if (isEnabled()) { + consoleSandbox(() => { + GLOBAL_OBJ.console[level](`${PREFIX}[${level}]:`, ...args); }); } +} + +function _getLoggerSettings(): { enabled: boolean } { + if (!DEBUG_BUILD) { + return { enabled: false }; + } - return logger as Logger; + return getGlobalSingleton('loggerSettings', () => ({ enabled: false })); } /** * This is a logger singleton which either logs things or no-ops if logging is not enabled. * The logger is a singleton on the carrier, to ensure that a consistent logger is used throughout the SDK. */ -export const logger = getGlobalSingleton('logger', makeLogger); +export const logger = { + /** Enable logging. */ + enable, + /** Disable logging. */ + disable, + /** Check if logging is enabled. */ + isEnabled, + /** Log a message. */ + log, + /** Log level info */ + info, + /** Log a warning. */ + warn, + /** Log an error. */ + error, + /** Log a debug message. */ + debug, + /** Log an assertion. */ + assert, + /** Log a trace. */ + trace, +} satisfies Logger; diff --git a/packages/core/test/clear-global-scope.ts b/packages/core/test/clear-global-scope.ts deleted file mode 100644 index e69de29bb2d1..000000000000 diff --git a/packages/core/test/lib/utils/logger.test.ts b/packages/core/test/lib/utils/logger.test.ts new file mode 100644 index 000000000000..21d26b273dea --- /dev/null +++ b/packages/core/test/lib/utils/logger.test.ts @@ -0,0 +1,57 @@ +import { beforeEach, describe, expect, it, vi } from 'vitest'; +import { logger } from '../../../src'; +import { getMainCarrier, getSentryCarrier } from '../../../src/carrier'; + +describe('logger', () => { + beforeEach(() => { + vi.clearAllMocks(); + getSentryCarrier(getMainCarrier()).loggerSettings = undefined; + }); + + it('works with defaults', () => { + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + logger.log('test'); + expect(consoleLogSpy).not.toHaveBeenCalled(); + expect(logger.isEnabled()).toBe(false); + }); + + it('allows to enable and disable logging', () => { + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + + logger.log('test'); + expect(logger.isEnabled()).toBe(false); + expect(consoleLogSpy).not.toHaveBeenCalled(); + + logger.enable(); + logger.log('test'); + expect(logger.isEnabled()).toBe(true); + expect(consoleLogSpy).toHaveBeenCalledTimes(1); + + logger.log('test2'); + expect(consoleLogSpy).toHaveBeenCalledTimes(2); + + logger.disable(); + + logger.log('test3'); + expect(logger.isEnabled()).toBe(false); + expect(consoleLogSpy).toHaveBeenCalledTimes(2); + }); + + it('picks up enabled logger settings from carrier', () => { + getSentryCarrier(getMainCarrier()).loggerSettings = { enabled: true }; + + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + logger.log('test'); + expect(consoleLogSpy).toHaveBeenCalledTimes(1); + expect(logger.isEnabled()).toBe(true); + }); + + it('picks up disabled logger settings from carrier', () => { + getSentryCarrier(getMainCarrier()).loggerSettings = { enabled: false }; + + const consoleLogSpy = vi.spyOn(console, 'log').mockImplementation(() => {}); + logger.log('test'); + expect(consoleLogSpy).toHaveBeenCalledTimes(0); + expect(logger.isEnabled()).toBe(false); + }); +});