-
Notifications
You must be signed in to change notification settings - Fork 43
refactor: Avoid NodeJS-specific module (use explicit logger scopes)
#3309
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
No such thing in the browser.
modulemodule (use explicit logger scopes)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request removes the NodeJS-specific module dependency from logger initialization across the entire codebase by replacing new Logger(module) with explicit string identifiers like new Logger('ClassName'). This change improves portability to non-NodeJS environments and makes log tracing more explicit and descriptive.
Key changes:
- Replaced all
new Logger(module)calls withnew Logger('<descriptive-string>')across all packages - Exported
LoggerModuletype from utils package for consistent typing - Updated
LoggerFactory.createLoggerparameter frommodule: NodeJS.ModuletologgerModule: LoggerModule
Reviewed changes
Copilot reviewed 166 out of 168 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/utils/src/Logger.ts | Exported LoggerModule type to make it publicly available |
| packages/utils/src/exports.ts | Added LoggerModule to public exports |
| packages/utils/test/Logger.test.ts | Updated test logger to use string identifier 'Logger' |
| packages/utils/src/executeSafePromise.ts | Changed logger name from module to 'executeSafePromise' |
| packages/utils/src/TheGraphClient.ts | Changed logger name to 'TheGraphClient' |
| packages/sdk/src/utils/LoggerFactory.ts | Updated parameter name from module to loggerModule and imported LoggerModule type |
| packages/sdk/src/utils/waitForAssignmentsToPropagate.ts | Changed logger creation to use 'waitForAssignmentsToPropagate' |
| packages/sdk/src/utils/utils.ts | Changed logger name to 'utils' |
| packages/sdk/src/utils/persistence/ServerPersistence.ts | Updated LoggerFactory usage to use 'ServerPersistence' |
| packages/sdk/src/utils/LoggingJsonRpcProvider.ts | Changed logger name to 'LoggingJsonRpcProvider' |
| packages/sdk/src/subscribe/*.ts | Updated all subscription-related files to use descriptive class/file names |
| packages/sdk/src/encryption/*.ts | Updated encryption-related files to use class names |
| packages/sdk/src/contracts/*.ts | Updated all contract files to use descriptive names |
| packages/sdk/src/StreamrClient.ts | Changed logger to 'StreamrClient' |
| packages/sdk/src/ProxyNodeFinder.ts | Changed logger to 'ProxyNodeFinder' |
| packages/sdk/src/NetworkNodeFacade.ts | Changed logger to 'NetworkNodeFacade' |
| packages/sdk/test/**/*.ts | Updated all test files to use descriptive test file names |
| packages/trackerless-network/src/**/*.ts | Updated all network layer files with descriptive names |
| packages/trackerless-network/test/**/*.ts | Updated test files with descriptive test identifiers |
| packages/proto-rpc/src/*.ts | Updated RPC-related files with class names |
| packages/node/src/**/*.ts | Updated broker node files with descriptive names |
| packages/node/test/**/*.ts | Updated test files with descriptive test identifiers |
| packages/test-utils/src/index.ts | Changed logger to generic 'index' identifier |
| packages/geoip-location/src/*.ts | Updated geolocation files with descriptive names |
| packages/geoip-location/test/*.ts | Updated test with descriptive identifier |
| packages/dht/src/**/*.ts | Updated all DHT files with class/file names |
| packages/dht/test/**/*.ts | Updated test files with descriptive test identifiers |
| packages/cdn-location/src/*.ts | Updated CDN location files with descriptive names |
| packages/cdn-location/test/*.ts | Updated test with descriptive identifier |
| packages/autocertifier-server/src/*.ts | Updated autocertifier server files with class names |
| packages/autocertifier-server/bin/*.ts | Updated script with descriptive identifier |
| packages/autocertifier-client/src/*.ts | Updated client files with class names |
| packages/cli-tools/bin/*.ts | Updated CLI tool with descriptive identifier |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| import { StreamIDBuilder } from '../../src/StreamIDBuilder' | ||
|
|
||
| const logger = new Logger(module) | ||
| const logger = new Logger('utils') |
Copilot
AI
Dec 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The logger identifier 'utils' is too generic and could be ambiguous in test logs. Since this is a test utility file in the SDK package, consider using 'sdk-test-utils' or 'test-utils' to make it easier to distinguish from other similarly named files when reading logs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot open a new pull request to apply changes based on this feedback
|
@mondoreale I've opened a new pull request, #3314, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@mondoreale I've opened a new pull request, #3315, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
@mondoreale I've opened a new pull request, #3316, to work on those changes. Once the pull request is ready, I'll request review from you. |
…3314) ## Summary Addresses review feedback on #3309 to improve logger traceability. The generic identifier `'utils'` was ambiguous when debugging logs from multiple files across packages. ## Changes - Changed logger identifier from `'utils'` to `'sdk-utils'` in `packages/sdk/src/utils/utils.ts` This follows the pattern used by other SDK files like `StreamrClient`, `NetworkNodeFacade`, and `Operator` which use package-scoped identifiers for clarity. ## Checklist before requesting a review - [ ] Is this a breaking change? If it is, be clear in summary. - [x] Read through code myself one more time. - [x] Make sure any and all `TODO` comments left behind are meant to be left in. - [x] Has reasonable passing test coverage? - [ ] Updated changelog if applicable. - [ ] Updated documentation if applicable. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/streamr-dev/network/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > Aligns SDK utils logging with package-scoped identifiers for clearer traceability. > > - Update `packages/sdk/src/utils/utils.ts`: change `new Logger('utils')` to `new Logger('sdk-utils')` > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 2b3925b. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mondoreale <[email protected]>
|
|
## Summary Addresses feedback on PR #3309 to use a more descriptive logger identifier for improved log tracing. ## Changes - Changed logger identifier from `'index'` to `'test-utils'` in `packages/test-utils/src/index.ts` The generic `'index'` identifier made it difficult to distinguish logs when multiple packages have index.ts files. The new identifier aligns with the package name `@streamr/test-utils`. ## Checklist before requesting a review - [ ] Is this a breaking change? If it is, be clear in summary. - [x] Read through code myself one more time. - [x] Make sure any and all `TODO` comments left behind are meant to be left in. - [x] Has reasonable passing test coverage? - [ ] Updated changelog if applicable. - [ ] Updated documentation if applicable. <!-- START COPILOT CODING AGENT TIPS --> --- ✨ Let Copilot coding agent [set things up for you](https://github.com/streamr-dev/network/issues/new?title=✨+Set+up+Copilot+instructions&body=Configure%20instructions%20for%20this%20repository%20as%20documented%20in%20%5BBest%20practices%20for%20Copilot%20coding%20agent%20in%20your%20repository%5D%28https://gh.io/copilot-coding-agent-tips%29%2E%0A%0A%3COnboard%20this%20repo%3E&assignees=copilot) — coding agent works faster and does higher quality work when set up for your repo. <!-- CURSOR_SUMMARY --> --- > [!NOTE] > <sup>[Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) is generating a summary for commit f5f936e. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mondoreale <[email protected]>
…#3315) ## Summary Addresses feedback on PR #3309 to use a more specific logger identifier in test utilities, avoiding ambiguity with the source file's logger. ## Changes - Changed logger identifier from `'utils'` to `'sdk-test-utils'` in `packages/sdk/test/test-utils/utils.ts` This prevents confusion in logs between the test utilities logger and the identically-named logger in `packages/sdk/src/utils/utils.ts`. ## Checklist before requesting a review - [ ] Is this a breaking change? If it is, be clear in summary. - [x] Read through code myself one more time. - [x] Make sure any and all `TODO` comments left behind are meant to be left in. - [x] Has reasonable passing test coverage? - [ ] Updated changelog if applicable. - [ ] Updated documentation if applicable. <!-- START COPILOT CODING AGENT TIPS --> --- 💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more [Copilot coding agent tips](https://gh.io/copilot-coding-agent-tips) in the docs. --------- Co-authored-by: copilot-swe-agent[bot] <[email protected]> Co-authored-by: mondoreale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 166 out of 168 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 166 out of 168 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This pull request standardizes logger initialization throughout the codebase by replacing the use of
new Logger(module)with a more descriptive string identifier for each module or class. This improves the clarity of log messages and makes it easier to trace logs back to their source components.Changes
Logger initialization updates (all changes):
Replaced
new Logger(module)withnew Logger('<ClassOrFileName>')in all relevant files acrossautocertifier-client,autocertifier-server,cdn-location, anddhtpackages to ensure consistent and descriptive logging.Replaced all occurrences of
loggerFactory.createLogger(module)withloggerFactory.createLogger('<ClassOrFileName>')in all relevant files across the monorepo.Future steps
Note
Because logger instances are created at the top level of each module, simply importing the package into a host project will eagerly instantiate all associated loggers, even if the corresponding exports are never used.
Many modules currently instantiate a logger at import time (e.g.
new Logger('Foo')). This introduces a side effect during module evaluation, which prevents effective tree-shaking of both the module itself and theLoggerimplementation.A future improvement would be to decouple logger creation from module imports by moving initialization behind explicit factory functions or lazy accessors. This would reduce import-time side effects and allow unused modules (and the logger code) to be safely tree-shaken by the bundler.
Loggerwas the key reason why we kept using (and polyfilling)module. Consumers will be able to drop that env-specific "dependency" from their setups soon.