fix: make sure only one logger instance in global #2
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThe pull request introduces updates to the project's continuous integration and build processes. It includes modifications to the GitHub Actions workflows, adding support for Node.js version 22, creating a new publish workflow, and updating the project's configuration. The changes also involve dependency updates in the Changes
Sequence DiagramsequenceDiagram
participant Repo as GitHub Repository
participant CI as GitHub Actions
participant Node as Node.js
participant Build as Build Process
participant Publish as Package Publish
Repo->>CI: Trigger workflow
CI->>Node: Setup Node.js (v20)
CI->>Build: Install dependencies
Build->>Build: Run build script
Build->>Publish: Publish package
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/node@20.17.11, npm/tshy@1.18.0 |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.github/workflows/pkg.pr.new.yml (1)
1-2: Consider restricting workflow triggersRunning on both push and pull_request events might cause duplicate workflow runs when PRs are pushed to.
Consider this change:
-on: [push, pull_request] +on: + push: + branches: [ master ] + pull_request: + types: [ opened, synchronize, reopened ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks)README.md(2 hunks)package.json(3 hunks)
🔇 Additional comments (6)
package.json (3)
27-29: LGTM! Enhanced type checking in CI
Good addition of type checking using @arethetypeswrong/cli in the preci script. This will help catch type-related issues before publishing.
71-72: LGTM! Proper dual ESM/CommonJS support
The addition of the module field properly supports both ESM and CommonJS environments, following Node.js best practices.
7-15: Verify tshy major version upgrade impact
The upgrade from tshy v1.1.1 to v3.0.2 is a major version bump that might include breaking changes.
Run this script to check for breaking changes:
✅ Verification successful
Let me try different commands to verify the impact of the tshy upgrade.
Let me check the tsconfig.json and look for any other tshy-related configurations.
tshy upgrade appears safe with minimal configuration
The tshy setup in this project is quite minimal:
- Simple exports configuration in package.json
- Basic usage in prepublishOnly script
- No complex tshy-specific configurations in tsconfig.json
The upgrade should be safe as:
- The project uses a straightforward exports configuration which is a core feature
- No advanced or deprecated features are being used
- The tshy-after package is also present which is the recommended companion package
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check tshy's changelog or releases for breaking changes
gh api repos/unjs/tshy/releases | jq -r '.[].body' | grep -i "breaking"
# Verify if other projects have similar tshy configuration
rg '"tshy":\s*{.*"exports"' -A 5
Length of output: 312
Script:
#!/bin/bash
# Search for tshy configuration in package.json
rg '"tshy":' -A 10 package.json
# Search for tshy-related files
fd tshy
# Check if there are any tshy configuration files
fd tsconfig
Length of output: 591
Script:
#!/bin/bash
# Check tsconfig.json content
cat tsconfig.json
# Look for any tshy usage in the codebase
rg -l 'tshy'
# Check if there are any build scripts that use tshy
rg 'tshy' package.json
Length of output: 368
.github/workflows/nodejs.yml (1)
16-16: LGTM! Node.js 22 support added
The addition of Node.js 22 to the test matrix aligns with the package.json updates.
Run this script to verify Node.js version compatibility:
✅ Verification successful
Node.js version compatibility verified
The package.json specifies "node": ">=16.0.0" as the engine requirement, which is compatible with all Node.js versions in the CI matrix (16, 18, 20, 22). There are no version-specific code patterns that might cause compatibility issues with Node.js 22.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if package.json engine constraints match CI versions
rg '"node":\s*">=\d+'
# Check for any Node.js version specific code
rg 'process\.version|node:\d+|>=\d+\.\d+\.\d+'
Length of output: 144
README.md (2)
74-74: LGTM! Improved contributors visualization
Great improvement to use contrib.rocks for a dynamic visualization of contributors. This approach:
- Automatically updates as new contributors join
- Provides a better visual representation
- Reduces maintenance overhead
70-70: Verify LICENSE file existence
The license link has been updated from LICENSE.txt to LICENSE.
Let's verify the correct license file exists:
✅ Verification successful
License file exists and matches the updated link
The verification confirms that:
- The
LICENSEfile exists in the repository - It's an MIT license file as expected
- The link in the README correctly points to the existing file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify license file existence and format
# Check if LICENSE file exists
if [ -f LICENSE ]; then
echo "LICENSE file exists"
head -n 1 LICENSE
else
echo "LICENSE file not found"
# Check if it's using the old name
[ -f LICENSE.txt ] && echo "Found LICENSE.txt instead"
fi
Length of output: 101
fengmk2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Logger.ts (1)
10-13: LGTM! Consider enhancing the comment.The conditional initialization of the global logger instances map is a good improvement that prevents accidental overwrites of existing instances.
Consider making the comment more descriptive:
-// make sure we have a global logger instances map +// Initialize the global logger instances map only if it doesn't exist +// This ensures we don't overwrite existing logger instances during multiple imports
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Logger.ts(1 hunks)
🔇 Additional comments (1)
src/Logger.ts (1)
10-13: Verify global variable usage across the codebase.
Let's ensure the global variable is consistently accessed and there are no direct assignments elsewhere that could bypass this initialization check.
✅ Verification successful
Global variable __ONE_LOGGER_INSTANCES__ is safely managed within Logger.ts
The verification shows that the global variable __ONE_LOGGER_INSTANCES__ is:
- Only initialized and accessed within
src/Logger.ts - Has consistent access patterns through
globalThis - Has proper type declaration
- All modifications are properly controlled through the Logger class methods
The implementation follows a safe singleton pattern where:
- The variable is declared with proper typing
- Initial check and initialization is done once
- Access is controlled through getter/setter methods
- Cleanup is handled through a delete operation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any direct assignments to __ONE_LOGGER_INSTANCES__
# and verify its consistent usage across the codebase
# Search for any direct assignments to __ONE_LOGGER_INSTANCES__
echo "Checking for direct assignments to __ONE_LOGGER_INSTANCES__..."
rg -l "__ONE_LOGGER_INSTANCES__\s*=" --type ts
# Search for all usages of __ONE_LOGGER_INSTANCES__ to verify consistent access patterns
echo "Checking all usages of __ONE_LOGGER_INSTANCES__..."
rg -A 2 "__ONE_LOGGER_INSTANCES__" --type ts
# Look for potential ESM/CJS interop issues with globalThis
echo "Checking for potential module system interop issues..."
rg "global\." --type ts
Length of output: 1247
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
[skip ci] ## [1.0.1](v1.0.0...v1.0.1) (2025-01-01) ### Bug Fixes * make sure only one logger instance in global ([#2](#2)) ([f8c8d18](f8c8d18))
1 participant
Summary by CodeRabbit
Release Notes
CI/CD Improvements
Dependencies
Project Configuration
Documentation