fix: patch File to support test on Node.js 18 #31
Conversation
also use npm trusted publisher node-modules/github-actions#14
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 46 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 updates introduce configuration and test enhancements across the project. Changes include updating workflow references and secrets, extending Changes
Sequence Diagram(s)sequenceDiagram
participant TestRunner as Vitest
participant Setup as test/setup.ts
participant NodeEnv as Node.js Environment
TestRunner->>Setup: Load setup.ts before tests
Setup->>NodeEnv: Check if global.File exists
alt File not defined
Setup->>NodeEnv: Assign buffer.File to global.File
end
TestRunner->>Test: Execute tests with initialized environment
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @fengmk2, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request primarily aims to ensure compatibility and enable testing of the project on Node.js 18. It achieves this by introducing a polyfill for the File object, updating various development dependencies, and refining the build and test configurations to support modern Node.js environments and module systems. Additionally, it includes a minor fix for canonicalized string comparison in the signing utility.
Highlights
- Node.js 18 Compatibility: I've added a polyfill for the File object in test/setup.ts to ensure tests run correctly on Node.js versions prior to 20, specifically targeting Node.js 18.
- Dependency Upgrades: I've updated several key dependencies, including urllib, oxlint, tshy, and vitest, to their latest versions, bringing in various improvements and bug fixes.
- Improved Build and Test Setup: I've enhanced the project's build and test infrastructure by adding a module entry in package.json for better ESM support, introducing a dedicated CJS test script, and updating linting configurations to ignore new test files.
- Canonicalized String Comparison Refinement: I've refactored the src/util/sign.ts file to use a dedicated helper function for canonicalized string comparison, which should improve the robustness of the signing process.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
Pull Request Overview
This PR addresses Node.js 18 compatibility issues by patching the global File object and upgrades the build/release infrastructure to use npm trusted publisher.
- Adds vitest configuration with a test setup file that polyfills
Filefor Node.js < 20 - Refactors comparison function in sign utility to improve code organization
- Updates dependencies and switches to npm trusted publisher workflow
Reviewed Changes
Copilot reviewed 7 out of 8 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| vite.config.ts | Adds vitest configuration with setup file |
| test/setup.ts | Polyfills global File object for Node.js 18 compatibility |
| test/hello.cjs | Adds CommonJS test file for validating package compatibility |
| src/util/sign.ts | Extracts comparison function to module level |
| package.json | Updates dependencies and adds CJS test script |
| .oxlintrc.json | Updates linter configuration to ignore new test files |
| .github/workflows/release.yml | Switches to npm trusted publisher workflow |
There was a problem hiding this comment.
Code Review
This pull request introduces a patch to support testing on Node.js 18 by polyfilling the File API. It also adds a CommonJS smoke test and updates several dependencies. The changes look good overall, but I've found a critical issue in the signature generation logic. The custom sort function for canonicalizing resource parameters is incorrect and could lead to signature validation failures. I've left a comment with a suggested fix.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #31 +/- ##
==========================================
+ Coverage 91.40% 91.41% +0.01%
==========================================
Files 15 15
Lines 1443 1445 +2
Branches 291 291
==========================================
+ Hits 1319 1321 +2
Misses 124 124 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
.oxlintrc.json (1)
5-6: Lint env and ignore patterns for tests
- Removing mocha env is fine; if you use Vitest globals (describe/it/expect) in test files, consider enabling Vitest env for those files to avoid no-undef flags.
- Ignoring test/*.cjs and test/setup.ts is OK; alternatively, prefer per-file overrides so tests still benefit from linting.
Suggested addition (outside the shown range) to keep test linting while enabling Vitest globals:
{ "overrides": [ { "files": ["test/**/*.test.ts", "test/**/*.spec.ts"], "env": { "vitest": true } } ] }Also applies to: 11-17
package.json (2)
12-16: Make test:cjs build-aware to avoid local run failuresAs written, running
npm run test:cjslocally will fail if dist hasn’t been built. Two options:
- Option A (simple): make test:cjs build before running, and simplify CI:
- "test:cjs": "node test/hello.cjs", + "test:cjs": "npm run prepublishOnly && node test/hello.cjs", - "ci": "npm run cov && npm run prepublishOnly && attw --pack && npm run test:cjs", + "ci": "npm run cov && attw --pack && npm run test:cjs",
- Option B (preferred): add a dedicated build script and reuse it:
+ "build": "tshy && tshy-after", - "test:cjs": "node test/hello.cjs", + "test:cjs": "npm run build && node test/hello.cjs", - "ci": "npm run cov && npm run prepublishOnly && attw --pack && npm run test:cjs", + "ci": "npm run cov && npm run build && attw --pack && npm run test:cjs",
96-98: Confirm intent for "module" alongside "exports"The module field is often used by bundlers; with a fully-specified exports map it’s optional. If your consumers/tools rely on "module", keeping it is fine; otherwise you can drop it to avoid redundancy.
src/util/sign.ts (1)
48-50: Ensure canonicalization uses the corrected comparatorOnce the comparator is fixed, this sort will be deterministic and spec-compliant. If you prefer, you can also drop the custom comparator and rely on default string sort:
-for (const key of Object.keys(parameters).sort( - compareCanonicalizedString -)) { +for (const key of Object.keys(parameters).sort()) { processFunc(key); }If interoperability is critical across locales/ICU builds, prefer a comparator like
a.localeCompare(b, 'en')to avoid locale-dependent ordering.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks).oxlintrc.json(1 hunks)package.json(3 hunks)src/util/sign.ts(2 hunks)test/hello.cjs(1 hunks)test/setup.ts(1 hunks)vite.config.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (24, ubuntu-latest)
- GitHub Check: build (20, ubuntu-latest)
- GitHub Check: build (22, ubuntu-latest)
- GitHub Check: build (18, ubuntu-latest)
🔇 Additional comments (2)
.gitignore (1)
32-32: LGTM: ignore pnpm lockfileAdding pnpm-lock.yaml is correct and consistent with ignoring other lock files.
vite.config.ts (1)
3-7: Vitest setup registration looks goodSetup file is correctly registered; this will ensure the File polyfill is loaded before tests.
| accessKeyId: 'LTAI5tG666666666666666', | ||
| accessKeySecret: '66666666666666666666666666666666', | ||
| bucket: 'foo', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoded secrets and logging sensitive config
Even as placeholders, committing credentials and logging the constructed client can leak secrets if envs are used later. Use env vars with safe fallbacks and avoid logging the full object.
- accessKeyId: 'LTAI5tG666666666666666',
- accessKeySecret: '66666666666666666666666666666666',
+ accessKeyId: process.env.OSS_ACCESS_KEY_ID || 'test-ak',
+ accessKeySecret: process.env.OSS_ACCESS_KEY_SECRET || 'test-secret',
@@
-console.log(ossObject);
+require('node:assert').ok(ossObject);Also applies to: 11-11
🤖 Prompt for AI Agents
In test/hello.cjs around lines 6 to 8 and line 11, avoid hardcoding sensitive
credentials like accessKeyId and accessKeySecret directly in the code. Instead,
read these values from environment variables with safe fallback defaults if
needed. Also, remove or modify any logging of the full client object to prevent
accidental exposure of secrets.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/OSSObject.test.ts (2)
986-988: Preferprocess.versions.nodefor version checkAvoid relying on the leading 'v' in
process.version. This is a tiny robustness win.- it.skipIf(process.version.startsWith('v18.'))( + it.skipIf(process.versions.node.startsWith('18.'))(
986-1012: Optional: keep coverage on Node 18 by adapting size/timeout and cleaning up temp fileInstead of skipping entirely on Node 18, you can reduce the payload size there, increase the test timeout, and delete the temp file after the run. This keeps the path exercised on all supported Node versions and prevents CI disk bloat.
- // timeout on Node.js 18 - it.skipIf(process.version.startsWith('v18.'))( - 'should add very big file: 4mb with streaming way', - async () => { - name = `${prefix}oss-client/oss/bigfile-4mb.bin`; - const bigFile = path.join(tmpdir, 'bigfile-4mb.bin'); - await writeFile(bigFile, Buffer.alloc(4 * 1024 * 1024).fill('a\n')); - const object = await ossObject.putStream( - name, - createReadStream(bigFile) - ); - assert.equal(typeof object.res.headers['x-oss-request-id'], 'string'); - assert.equal(typeof object.res.rt, 'number'); - assert.equal(object.res.size, 0); - assert.equal(object.name, name); - - // check content - const r = await ossObject.get(name); - assert.equal(r.res.status, 200); - assert.equal(r.res.headers['content-type'], 'application/octet-stream'); - assert.equal(r.res.size, 4 * 1024 * 1024); - const buf = await readFile(bigFile); - assert.ok(r.content); - assert.equal(r.content.length, buf.length); - assert.deepEqual(r.content, buf); - } - ); + it( + 'should add big file with streaming (4MB, 2MB on Node 18)', + async () => { + const isNode18 = process.versions.node.startsWith('18.'); + const SIZE = isNode18 ? 2 * 1024 * 1024 : 4 * 1024 * 1024; + name = `${prefix}oss-client/oss/bigfile-${isNode18 ? '2mb' : '4mb'}.bin`; + const bigFile = path.join(tmpdir, `bigfile-${SIZE}.bin`); + await writeFile(bigFile, Buffer.alloc(SIZE).fill('a\n')); + try { + const object = await ossObject.putStream( + name, + createReadStream(bigFile) + ); + assert.equal(typeof object.res.headers['x-oss-request-id'], 'string'); + assert.equal(typeof object.res.rt, 'number'); + assert.equal(object.res.size, 0); + assert.equal(object.name, name); + + // check content + const r = await ossObject.get(name); + assert.equal(r.res.status, 200); + assert.equal(r.res.headers['content-type'], 'application/octet-stream'); + assert.equal(r.res.size, SIZE); + const buf = await readFile(bigFile); + assert.ok(r.content); + assert.equal(r.content.length, buf.length); + assert.deepEqual(r.content, buf); + } finally { + await rm(bigFile, { force: true }).catch(() => {}); + } + }, + 120_000 + );Also add this import near the top of the file:
import { rm } from 'node:fs/promises';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/OSSObject.test.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: build (22, ubuntu-latest)
- GitHub Check: build (24, ubuntu-latest)
- GitHub Check: build (20, ubuntu-latest)
- GitHub Check: build (18, ubuntu-latest)
🔇 Additional comments (1)
test/OSSObject.test.ts (1)
986-1012: Pragmatic skip for Node 18 flakiness is acceptableSkipping this large streaming test on Node 18 to avoid timeouts is reasonable for CI stability.
[skip ci] ## <small>2.5.1 (2025-08-09)</small> * fix: patch File to support test on Node.js 18 (#31) ([93edf11](93edf11)), closes [#31](#31) * chore: remove eslint deps (#28) ([46b4acf](46b4acf)), closes [#28](#28) * chore: update oxlint config to use @eggjs/oxlint-config (#30) ([798a8d1](798a8d1)), closes [#30](#30)
|
🎉 This PR is included in version 2.5.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
also use npm trusted publisher node-modules/github-actions#14
Summary by CodeRabbit
New Features
Chores
Tests