Conversation
6d66554 to
0ca5710
Compare
37eecbd to
70760fd
Compare
0ca5710 to
4ad2b5f
Compare
a0132ff to
9a60f23
Compare
4ad2b5f to
9c05fec
Compare
9a60f23 to
eacefe1
Compare
9c05fec to
53f65cb
Compare
53f65cb to
99bbc51
Compare
eacefe1 to
7d5e8e4
Compare
8b30a3c to
202e102
Compare
There was a problem hiding this comment.
Pull request overview
This PR introduces an environment-agnostic logger abstraction to support the architectural refactoring described in issue #151. The implementation separates core logging functionality from browser-specific implementation details, setting the foundation for a cleaner package structure that will enable code reuse across different JavaScript environments (Node.js, Nuxt, etc.).
Changes:
- Added core logger abstraction (
Loggerinterface,LogType,setLogger(),log()) to @hawk.so/core - Migrated browser-specific logger from
utils/log.tstologger/logger.tsas a factory function - Updated all imports across the codebase to use the new logger from @hawk.so/core
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/core/src/logger/logger.ts | Defines environment-agnostic Logger interface, LogType, and global logger singleton with setLogger/log functions |
| packages/core/src/index.ts | Exports Logger, LogType, setLogger, and log from core package |
| packages/javascript/src/logger/logger.ts | Browser-specific logger implementation as factory function createBrowserLogger |
| packages/javascript/src/catcher.ts | Initializes logger in constructor; minor formatting improvements to object destructuring |
| packages/javascript/src/utils/log.ts | Removed old browser-specific logger implementation |
| packages/javascript/src/utils/validation.ts | Updated to import log from @hawk.so/core |
| packages/javascript/src/utils/event.ts | Updated to import log from @hawk.so/core |
| packages/javascript/src/modules/socket.ts | Updated to import log from @hawk.so/core |
| packages/javascript/src/modules/fetchTimer.ts | Updated to import log from @hawk.so/core |
| packages/javascript/src/addons/breadcrumbs.ts | Updated to import log from @hawk.so/core |
| packages/javascript/tests/logger/logger.test.ts | Comprehensive tests for browser logger implementation |
| packages/javascript/tests/breadcrumbs.test.ts | Updated to spy on core log function instead of console.warn |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| const decodedIntegrationToken: DecodedIntegrationToken = JSON.parse(atob(this.token)); | ||
| const {integrationId} = decodedIntegrationToken; | ||
| const { integrationId } = decodedIntegrationToken; |
There was a problem hiding this comment.
This formatting change (adding spaces inside object destructuring) appears to be unrelated to the logger abstraction changes and is a minor style adjustment. While it improves consistency with modern JavaScript style conventions, it would be better to keep such formatting changes separate from functional changes in a PR, or apply them consistently throughout the file if they're part of a deliberate style update.
| const { integrationId } = decodedIntegrationToken; | |
| const {integrationId} = decodedIntegrationToken; |
| return user; | ||
| } | ||
| const newUser: AffectedUser = {id: id()}; | ||
| const newUser: AffectedUser = { id: id() }; |
There was a problem hiding this comment.
This formatting change (adding spaces inside object destructuring) appears to be unrelated to the logger abstraction changes and is a minor style adjustment. While it improves consistency with modern JavaScript style conventions, it would be better to keep such formatting changes separate from functional changes in a PR, or apply them consistently throughout the file if they're part of a deliberate style update.
| const newUser: AffectedUser = { id: id() }; | |
| const newUser: AffectedUser = {id: id()}; |
| */ | ||
| private getAddons(error: Error | string): HawkJavaScriptEvent['addons'] { | ||
| const {innerWidth, innerHeight} = window; | ||
| const { innerWidth, innerHeight } = window; |
There was a problem hiding this comment.
This formatting change (adding spaces inside object destructuring) appears to be unrelated to the logger abstraction changes and is a minor style adjustment. While it improves consistency with modern JavaScript style conventions, it would be better to keep such formatting changes separate from functional changes in a PR, or apply them consistently throughout the file if they're part of a deliberate style update.
| export function setLogger(logger: Logger): void { | ||
| loggerInstance = logger; | ||
| } | ||
|
|
||
| /** | ||
| * Logs a message using the registered logger implementation. | ||
| * | ||
| * If no logger has been registered via {@link setLogger}, this is a no-op. | ||
| * | ||
| * @param msg - Message to log. | ||
| * @param type - Log level (default: 'log'). | ||
| * @param args - Additional arguments to log. | ||
| */ | ||
| export function log(msg: string, type?: LogType, args?: unknown): void { | ||
| if (loggerInstance) { | ||
| loggerInstance(msg, type, args); | ||
| } | ||
| } |
There was a problem hiding this comment.
The core logger abstraction (setLogger and log functions in @hawk.so/core) lacks test coverage. While the browser-specific implementation (createBrowserLogger) has comprehensive tests, the core abstraction functions are not tested. Consider adding tests to verify that: 1) log is a no-op when no logger is set, 2) setLogger correctly registers a logger, 3) log correctly delegates to the registered logger, and 4) the logger can be replaced by calling setLogger again.
| * @param {HawkInitialSettings|string} settings - If settings is a string, it means an Integration Token | ||
| */ | ||
| constructor(settings: HawkInitialSettings | string) { | ||
| setLogger(createBrowserLogger(VERSION)); |
There was a problem hiding this comment.
The logger is set on every Catcher instantiation, which means creating multiple Catcher instances will repeatedly call setLogger with the same logger implementation. While this doesn't cause functional issues (since VERSION is constant), it's wasteful. Consider checking if a logger is already set before creating and setting a new one, or set it once at module initialization time. For example: if (!isLoggerSet()) { setLogger(createBrowserLogger(VERSION)); } where isLoggerSet() is a new function exported from @hawk.so/core.
| * | ||
| * @example | ||
| * ```TypeScript | ||
| * import { createBrowserLogger } from '@hawk.so/browser'; |
There was a problem hiding this comment.
The import path in the documentation example is incorrect. The function is exported from '@hawk.so/javascript', not '@hawk.so/browser'. This could confuse users trying to use the logger. The example should use the correct package name that matches where this code actually lives.
| * import { createBrowserLogger } from '@hawk.so/browser'; | |
| * import { createBrowserLogger } from '@hawk.so/javascript'; |
| const editorLabelStyle = `line-height: 1em; | ||
| color: #fff; | ||
| display: inline-block; | ||
| line-height: 1em; |
There was a problem hiding this comment.
The CSS style string contains duplicate 'line-height: 1em;' properties on lines 33 and 36. While this doesn't cause an error, the second declaration will override the first, making the first one redundant. Consider removing one of the duplicate declarations to clean up the code.
| line-height: 1em; |
Related #151
Added env-agnostic
Loggerabstraction.Also added global singleton
loggerInstancewhich could be set viasetLoggerand used vialogmethods.Also added browser-specific logger factory-method
createBrowserLoggerwhich may be used asLogger.