test: add node 14, 16 test #24
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 18 minutes and 40 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 (2)
WalkthroughThis pull request introduces multiple new GitHub Actions workflow files for continuous integration (CI) involving different Node.js versions (14, 16, and 20). It also updates the Changes
Possibly related PRs
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.9, npm/eslint-config-egg@13.1.0, npm/npminstall@7.12.0, npm/runscript@1.6.0, npm/tshy-after@1.2.0, npm/tshy@1.18.0, npm/typescript@5.7.2 |
|
🚨 Potential security issues detected. Learn more about Socket for GitHub ↗︎ To accept the risk, merge this PR and you will not be notified again.
Next stepsWhat is a typosquat?Package name is similar to other popular packages and may not be the package you want. Use care when consuming similarly named packages and ensure that you did not intend to consume a different package. Malicious packages often publish using similar names as existing popular packages. Take a deeper look at the dependencyTake a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at support [AT] socket [DOT] dev. Remove the packageIf you happen to install a dependency that Socket reports as Known Malware you should immediately remove it and select a different dependency. For other alert types, you may may wish to investigate alternative packages or consider if there are other ways to mitigate the specific risk posed by the dependency. Mark a package as acceptable riskTo ignore an alert, reply with a comment starting with
|
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: 6
🧹 Outside diff range and nitpick comments (2)
src/utils.ts (1)
12-18: Add JSDoc documentation for thegetDirnamefunctionConsider adding documentation to explain the function's purpose in handling both CommonJS and ESM environments.
+/** + * Get the directory name in both CommonJS and ESM environments. + * Handles the difference between __dirname (CommonJS) and import.meta.url (ESM). + * @returns {string} The directory path + */ export function getDirname() {🧰 Tools
🪛 eslint
[error] 16-16: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-freepackage.json (1)
Line range hint
3-5: Engine requirement conflicts with Node.js 14, 16 testingThe engine requirement of
>=18.19.0conflicts with the PR objective of testing on Node.js 14 and 16.Either:
- Remove Node.js 14, 16 testing if they're not supported, or
- Adjust the engine requirement to support the tested versions:
"engine": { - "node": ">=18.19.0" + "node": ">=14.0.0" }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
.github/workflows/nodejs-14.yml(1 hunks).github/workflows/nodejs-16.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks)README.md(1 hunks)package.json(2 hunks)src/utils.ts(1 hunks)test/plugin.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- README.md
- .github/workflows/nodejs-16.yml
🧰 Additional context used
🪛 eslint
src/utils.ts
[error] 16-16: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
src/utils.ts
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🔇 Additional comments (5)
src/utils.ts (2)
1-3: LGTM! Good use of the node: prefix
The import statements follow Node.js best practices by using the node: prefix for built-in modules.
12-18: Verify Node.js version compatibility
Let's ensure the implementation is compatible with the target Node.js versions (14, 16).
✅ Verification successful
Let me gather more information about the implementation details and imports.
Based on the results, I can now generate the final response:
Implementation is compatible with Node.js 14+ ✓
The implementation is verified to be compatible because:
- Uses
node:urlimport which is supported since Node.js 14.18.0 import.meta.urlis supported in Node.js 14+ in ESM context- The code properly handles both ESM (
import.meta.url) and CommonJS (__dirname) environments - The implementation is already being used successfully in test files
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check package.json for Node.js version requirements and verify implementation compatibility
# Check package.json for engine requirements
echo "Checking Node.js version requirements:"
if [ -f package.json ]; then
jq '.engines' package.json
fi
# Search for any version-specific handling in the codebase
echo -e "\nChecking for version-specific handling:"
rg -l "process\.version"
# Check for any existing usage of getDirname
echo -e "\nChecking existing usage of getDirname:"
rg "getDirname\("
Length of output: 423
Script:
#!/bin/bash
# Check imports and implementation details
# Check for fileURLToPath usage
echo "Checking fileURLToPath imports and usage:"
rg "fileURLToPath"
# Check for import.meta.url usage
echo -e "\nChecking import.meta.url usage:"
rg "import\.meta\.url"
# Check if this is an ESM module
echo -e "\nChecking for ESM indicators:"
rg "type.*=.*module" package.json
fd -e mjs
rg "export.*{.*}" -g "*.{ts,js,mjs}"
Length of output: 889
🧰 Tools
🪛 eslint
[error] 16-16: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
package.json (2)
34-34: LGTM: Dependency updates look good
The updates to @types/node, eslint-config-egg, runscript, and tshy are appropriate and follow semver.
Also applies to: 38-38, 41-42
71-72: LGTM: Export configuration is correct
The types and module exports are properly configured for both ESM and CommonJS usage.
test/plugin.test.ts (1)
Line range hint 1-4: Verify Node.js 14 compatibility with node: protocol imports
The use of node: protocol imports might not be fully compatible with all Node.js 14 versions. Consider:
- For Node.js 14: Only supported in 14.18.0+
- For Node.js 16: Fully supported
Let's check the package.json for the minimum Node.js version requirement:
| if (typeof __dirname !== 'undefined') { | ||
| return __dirname; | ||
| } | ||
| // @ts-ignore |
There was a problem hiding this comment.
Replace @ts-ignore with @ts-expect-error
The @ts-ignore directive should be replaced with @ts-expect-error as it provides better type-checking guarantees by failing if the following line becomes error-free.
- // @ts-ignore
+ // @ts-expect-error📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // @ts-ignore | |
| // @ts-expect-error |
🧰 Tools
🪛 eslint
[error] 16-16: Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free.
(@typescript-eslint/ban-ts-comment)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 22)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
🪛 GitHub Check: Node.js / Test (macos-latest, 18)
[failure] 16-16:
Use "@ts-expect-error" instead of "@ts-ignore", as "@ts-ignore" will do nothing if the following line is error-free
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #24 +/- ##
==========================================
- Coverage 88.00% 86.69% -1.31%
==========================================
Files 6 6
Lines 425 436 +11
Branches 53 53
==========================================
+ Hits 374 378 +4
- Misses 51 58 +7 ☔ View full report in Codecov by Sentry. |
Summary by CodeRabbit
New Features
getDirnameto determine the directory name based on the environment.Bug Fixes
runscriptmodule in tests to improve module access.Documentation
Chores
4.0.2and modified various dependencies inpackage.json.