Conversation
and change to npm trusted publisher node-modules/github-actions#14
WalkthroughThe updates include switching the release workflow to use a new reusable workflow and removing an NPM secret, simplifying the lint configuration by extending from an external base, updating the minimum Node.js version, adjusting test and script imports, and making minor code style changes in TypeScript files without altering logic. Changes
Sequence Diagram(s)No sequence diagram generated, as all changes are configuration, style, or naming updates without new or modified control flow. Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Possibly related PRs
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. ✨ 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 focuses on updating and streamlining the project's development environment and configuration. I've removed an experimental Node.js flag, updated the required Node.js version to align with recent LTS releases, and significantly simplified the linting setup by adopting a shared, external configuration. These changes aim to improve maintainability, enhance security practices for publishing, and ensure compatibility with the latest Node.js features.
Highlights
- Node.js Environment Update: The
--experimental-strip-typesflag has been removed from the test script, and the minimum required Node.js version has been updated tov22.18.0in bothpackage.jsonandReadme.md. - Streamlined Linting Configuration: The
.oxlintrc.jsonfile has been significantly simplified by removing extensive inline rules and now extends a shared@eggjs/oxlint-config, centralizing linting rules and promoting consistency. - Dependency and Publishing Configuration Updates: The
package.jsonnow includes@eggjs/oxlint-configas a new development dependency and updates theoxlintversion. Additionally, thepublishConfighas been adjusted by removing thetag: "alpha"setting. - Minor Code Refactoring and Test Helper Renaming: Minor code adjustments were made in
src/context.tsandsrc/response.tsto change destructuring assignments to direct property access. Test files (test/request/accept.test.tsandtest/request/ip.test.ts) were also updated to use consistent naming for test helpers.
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. ↩
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #25 +/- ##
=======================================
Coverage 98.05% 98.05%
=======================================
Files 5 5
Lines 2059 2059
Branches 220 220
=======================================
Hits 2019 2019
Misses 40 40 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Code Review
This pull request introduces several beneficial updates to the project's configuration and codebase. The primary change is the removal of the --experimental-strip-types flag, which is appropriate given the updated Node.js engine requirement to version 22.18.0 or higher. The linting setup is significantly improved by extending a shared configuration from @eggjs/oxlint-config, which centralizes rules and enhances maintainability. Other changes include updating dependencies, removing the alpha tag from the publish configuration, and applying minor stylistic improvements in the source and test files for better readability and consistency with the new linting rules. All changes are positive and improve the project's quality.
There was a problem hiding this comment.
Pull Request Overview
This PR removes the --experimental-strip-types flag from Node.js test scripts and updates various configurations to use a trusted NPM publisher workflow. The changes include updating Node.js version requirements, simplifying linting configuration, and refactoring some code for consistency.
Key changes:
- Removed
--experimental-strip-typesflag from test script and updated Node.js version requirement - Simplified oxlint configuration by extending an external base config
- Updated GitHub Actions workflow to use npm-release instead of node-release
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| package.json | Updated Node.js engine requirement, removed experimental flag, updated dependencies |
| .oxlintrc.json | Simplified config by extending external base instead of local rule definitions |
| .github/workflows/release.yml | Changed workflow to use npm-release with trusted publisher |
| Readme.md | Updated minimum Node.js version in documentation |
| src/response.ts | Refactored destructuring to direct property access |
| src/context.ts | Refactored destructuring to direct property access |
| test/request/ip.test.ts | Renamed import alias for clarity |
| test/request/accept.test.ts | Renamed import aliases for clarity and consistency |
|
🎉 This PR is included in version 3.0.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
![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 (9)
package.json (1)
66-66: Lint config: new base and oxlint upgradeAdding
@eggjs/oxlint-configand upgradingoxlintto ^1.11.0 looks good. Ensure the extended config is compatible with the new oxlint version.If OXLint supports package-based extends, prefer a package reference over a node_modules file path for portability (see .oxlintrc comment).
- "extends": ["./node_modules/@eggjs/oxlint-config/.oxlintrc.json"] + "extends": ["@eggjs/oxlint-config"]Also applies to: 88-88
.oxlintrc.json (3)
7-7: Delegating rules to shared configEmpty rules object is fine when fully deferring to the shared config. Consider documenting local overrides here for future maintainers.
14-14: Prefer package-based extends for portabilityReferring to a file within node_modules is brittle across different environments. If OXLint supports it, switch to package-based extends.
- "extends": ["./node_modules/@eggjs/oxlint-config/.oxlintrc.json"] + "extends": ["@eggjs/oxlint-config"]If OXLint doesn’t support package-based resolution, keep the current path.
13-13: Optional: expand ignore patternsIf linting compiled output is unnecessary, consider ignoring build artifacts.
"ignorePatterns": [ "index.d.ts", "test/fixtures/**", "__snapshots__", - "example/cjs/helloworld.js" + "example/cjs/helloworld.js", + "dist/**", + "coverage/**" ],Readme.md (1)
22-22: Install section updated to 22.18.0 — ensure tagline matchesGood. The top tagline (Line 3) still mentions 22.17.1. Please update it to avoid confusion.
Proposed text:
- From: “drop Node.js < 22.17.1 support.”
- To: “drop Node.js < 22.18.0 support.”
test/request/ip.test.ts (1)
20-21: Nit: Consider renaming local variable for clarityUsing
const request = createRequest(...)can be confusing alongsidereqand the importedcreateRequest. ConsiderkoaRequestfor readability.- const request = createRequest(req, undefined, app); + const koaRequest = createRequest(req, undefined, app); - assert.strictEqual(request.ip, '127.0.0.1'); + assert.strictEqual(koaRequest.ip, '127.0.0.1');- const request = createRequest(req); - assert.strictEqual(request.ip, '127.0.0.2'); + const koaRequest = createRequest(req); + assert.strictEqual(koaRequest.ip, '127.0.0.2');- const request = createRequest(req); - assert.strictEqual(request.ip, '127.0.0.2'); + const koaRequest = createRequest(req); + assert.strictEqual(koaRequest.ip, '127.0.0.2');- const request = createRequest(req); - assert.strictEqual(request.ip, '127.0.0.2'); - request.ip = '127.0.0.1'; - assert.strictEqual(request.ip, '127.0.0.1'); + const koaRequest = createRequest(req); + assert.strictEqual(koaRequest.ip, '127.0.0.2'); + koaRequest.ip = '127.0.0.1'; + assert.strictEqual(koaRequest.ip, '127.0.0.1');Also applies to: 30-32, 52-54, 63-65
.github/workflows/release.yml (1)
10-10: Pin reusable workflow to a tag or commit for supply-chain safetyReferencing
@masteris mutable. Prefer a semver tag or a specific commit SHA.- uses: eggjs/github-actions/.github/workflows/npm-release.yml@master + uses: eggjs/github-actions/.github/workflows/npm-release.yml@<pinned-tag-or-commit>test/request/accept.test.ts (2)
13-13: Optional: Make the type assertion less brittle than instanceofDepending on CJS/ESM interop,
instanceof acceptscan be fragile. An interface/shape check is more resilient.- assert.ok(ctx.accept instanceof accepts); + assert.ok(typeof ctx.accept?.types === 'function');
23-26: Nit: Rename local variable for clarityAs in other tests, consider avoiding
requestas a local var to reduce ambiguity.- const request = createRequest(); - request.req.headers.accept = + const koaRequest = createRequest(); + koaRequest.req.headers.accept = 'application/*;q=0.2, image/jpeg;q=0.8, text/html, text/plain'; - ctx.accept = accepts(request.req); + ctx.accept = accepts(koaRequest.req);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.github/workflows/release.yml(1 hunks).oxlintrc.json(1 hunks)Readme.md(1 hunks)package.json(3 hunks)src/context.ts(1 hunks)src/response.ts(1 hunks)test/request/accept.test.ts(2 hunks)test/request/ip.test.ts(6 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
test/request/ip.test.ts (1)
test/test-helpers/context.ts (1)
request(25-27)
src/response.ts (1)
src/context.ts (2)
body(486-488)body(491-493)
test/request/accept.test.ts (3)
test/test-helpers/context.ts (2)
context(6-23)request(25-27)src/context.ts (1)
accepts(298-303)src/request.ts (1)
accepts(476-481)
⏰ 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). (1)
- GitHub Check: Node.js / Test (windows-latest, 24)
🔇 Additional comments (8)
src/context.ts (1)
206-206: Style consistency: direct property access (no-op change).Switching to
const res = this.res;is behaviorally identical to destructuring here and improves consistency withsrc/response.ts. LGTM.src/response.ts (1)
201-201: Style consistency: direct property access (no-op change).Using
const body = this.body;is equivalent to the prior destructuring and aligns access patterns across the codebase. No functional change. LGTM.package.json (1)
8-8: publishConfig tag removal – manual verification requiredRemoving the custom dist-tag means npm will publish under “latest” by default. I didn’t detect any npm-publish steps or
permissions: id-token: writeentries in your workflows. Please manually verify that:
- You intend to publish under the “latest” dist-tag.
- Your release pipeline (e.g.
.github/workflows/release.yml) is configured as a Trusted Publisher, i.e. setsand invokespermissions: id-token: writenpm publish(or an equivalent action) with the correct authentication.test/request/ip.test.ts (2)
6-6: Rename to createRequest is consistent and correctAlias import and usages look good. Matches the helper’s exported name and avoids confusion with Node’s req/request objects.
Also applies to: 20-20, 30-30, 43-43, 52-52, 63-63
6-6: No leftoverRequestusages in testsI searched across all
test/**/*.tsfiles for bothRequest(calls and anyimport { request as Request }aliases and found no matches. All tests have been updated to usecreateRequest..github/workflows/release.yml (2)
9-12: Good move to the npm trusted publisher workflowSwitching to
npm-release.ymland droppingNPM_TOKENaligns with OIDC-based publishing.
9-12: Verify OIDC-based auth, trusted publisher, and flag removalWe’ve confirmed:
- No lingering
NPM_TOKENsecrets in any.github/workflows/*.yml.- No occurrences of
--experimental-strip-typesacross the repo.Next steps:
- Manually inspect or fetch the reusable workflow (
eggjs/github-actions/.github/workflows/npm-release.yml@master) to ensure it requestsid-token: writeand handles npm provenance (so you don’t needNPM_TOKEN).- Confirm the published package is registered with npm as a Trusted Publisher for this repo/org.
test/request/accept.test.ts (1)
4-4: Imports and usages updated consistently
- Default import rename to
acceptsis consistent.- Helper alias
request as createRequestaligns with other tests.- Reassignment via
ctx.accept = accepts(request.req)is correct for overriding.Also applies to: 6-6, 13-13, 23-23, 26-26
| "version": "3.0.0", | ||
| "engines": { | ||
| "node": ">= 22.17.1" | ||
| "node": ">= 22.18.0" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Engine bump to Node >=22.18.0
Aligns with README Installation section. Ensure CI uses the same Node version.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Check versions referenced in Readme.md:"
rg -n -S "22\.17\.1|22\.18\.0" Readme.md || true
echo
echo "Check GitHub Actions node versions:"
fd -a .github/workflows | xargs -I{} sh -c 'echo "--- {} ---"; rg -n "setup-node|node-version" -A 2 "{}" || true'Length of output: 933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Verifying GitHub Actions are pinned to Node ≥22.18.0:"
rg -n "actions/setup-node|node-version" .github/workflows || trueLength of output: 331
Update CI to pin Node ≥22.18.0
The workflow currently installs “node-version: 22”, which may resolve to any 22.x patch and could fall below 22.18.0. To align with your package.json engines field and README, pin the CI to at least v22.18.0.
• .github/workflows/pkg.pr.new.yml
– Line 15: change node-version: 22 → node-version: 22.18.0
Suggested diff:
--- .github/workflows/pkg.pr.new.yml
@@ -13,2 +13,2
- - uses: actions/setup-node@v4
- node-version: 22
+ - uses: actions/setup-node@v4
+ node-version: 22.18.0📝 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.
| "node": ">= 22.18.0" | |
| - uses: actions/setup-node@v4 | |
| node-version: 22.18.0 |
🤖 Prompt for AI Agents
In .github/workflows/pkg.pr.new.yml at line 15, update the node-version from
"22" to "22.18.0" to ensure the CI uses Node.js version 22.18.0 or higher,
matching the package.json engines field and README requirements.
| "scripts": { | ||
| "pretest": "npm run lint -- --fix", | ||
| "test": "node --test --test-force-exit --test-update-snapshots --experimental-strip-types", | ||
| "test": "node --test --test-force-exit --test-update-snapshots", |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Removed --experimental-strip-types from test runner
Good cleanup. Please confirm all tests are JS/compiled outputs or otherwise runnable without TS stripping. If tests are .ts, consider compiling in pretest or using a runner that supports TS.
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "List potential TS test files:"
fd -e ts -e tsx -e mts -a test || true
echo
echo "Show test script:"
jq -r '.scripts.test' package.jsonLength of output: 3605
Ensure TypeScript tests still run after removing --experimental-strip-types
We’ve got a large number of .ts test files under test/. Node’s built-in test runner no longer strips TS syntax, so your current script
"test": "node --test --test-force-exit --test-update-snapshots"will fail on those files. You’ll need to either:
- Compile your
.tstests to JS before running:- Add a
preteststep (tsc --buildor similar), pointtestat the compiled output directory.
- Add a
- Or switch to a runner/loader that supports TS on-the-fly:
- e.g.
"test": "node --loader ts-node/esm --test --test-force-exit --test-update-snapshots" - Or use
@swc-node/register,esm-loader, etc.
- e.g.
Please update your package.json scripts (or CI) accordingly so that all .ts tests execute successfully.
🤖 Prompt for AI Agents
In package.json at line 13, the current test script uses Node's built-in test
runner which no longer supports running TypeScript files directly, causing test
failures. To fix this, update the test script to either compile the TypeScript
tests before running by adding a pretest step like "tsc --build" and pointing
the test script to the compiled JavaScript output, or modify the test script to
use a loader that supports TypeScript on-the-fly, such as adding "--loader
ts-node/esm" to the node command. Ensure the updated script runs all .ts test
files successfully.
and change to npm trusted publisher node-modules/github-actions#14
Summary by CodeRabbit