Conversation
BREAKING CHANGE: drop Node.js < 22.17.1 support
WalkthroughThis update introduces a new major alpha release with significant changes to project configuration, examples, and testing. Node.js version requirements are raised to 22.17.1+, and ESM/CJS example projects are added. The package structure, exports, and dev tooling are simplified. Tests and imports are modernized, and legacy files are removed. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExampleApp (CJS/ESM/TS)
participant KoaFramework
User->>ExampleApp: HTTP Request
ExampleApp->>KoaFramework: Passes request to Application instance
KoaFramework->>ExampleApp: Executes middleware (responds "Hello World")
ExampleApp->>User: HTTP Response ("Hello World, ...")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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. 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 (
|
commit: |
![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 introduces a significant breaking change by transitioning the @eggjs/koa library to an ESM-only module system and raising the minimum Node.js version requirement. This comprehensive update also includes a modernization of the build pipeline, a migration to Node.js's native test runner, and refreshed usage examples, aiming to align the project with current JavaScript ecosystem standards and improve maintainability.
Highlights
- ESM-only Migration: The library has been refactored to be ESM-only, dropping CommonJS support. This is reflected in
package.jsonupdates to module resolution and simplifiedexportsfields, as well as changes to internal import paths from.jsto.tsacross the codebase. - Node.js Version Bump: The minimum required Node.js version has been significantly increased from
18.19.0to22.17.1, aligning the project with newer LTS releases and enabling the use of modern JavaScript features. - Build System Modernization: The project's build process has been streamlined by removing
tshyandtshy-afterdependencies, adopting standard TypeScript compilation commands (tsc --build), and configuringtsconfig.jsonwith modern options likeverbatimModuleSyntaxandtarget: ES2024. - Testing Framework Migration: The test suite has been migrated from a previous test runner (likely Mocha, given dependency removals) to Node.js's native test runner (
node --test), with numerous test files updated to usenode:testimports and modern async patterns withnode:events.once. - Example Updates: Outdated example files have been removed and replaced with new, structured examples for both CommonJS and ESM, including a dedicated TypeScript example, to better demonstrate usage with the updated library.
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.
Code Review
This pull request is a significant refactoring to make the library ESM-only, dropping support for older Node.js versions and switching to tsc for builds and node:test for testing. The changes are well-executed and consistent with the goal.
I've found a few areas for improvement:
- The
cleanscript inpackage.jsonis not cross-platform. - Some of the refactored tests contain redundant event handlers which can be removed or simplified for clarity.
Overall, this is a great step forward for the library. Please see my detailed comments.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #24 +/- ##
==========================================
- Coverage 98.54% 98.05% -0.49%
==========================================
Files 5 5
Lines 2059 2059
Branches 385 220 -165
==========================================
- Hits 2029 2019 -10
- Misses 30 40 +10 ☔ 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
♻️ Duplicate comments (8)
test/request/secure.test.ts (1)
2-5: Same.tsimport issue as above
import { request } from '../test-helpers/context.ts';hits the same loader requirement. Verify your test runner or--loaderflag is configured globally; otherwise add it to the per-file shebang or package script.test/application/index.test.ts (1)
12-32: Excellent async/await modernization, but remove redundant error handler.The conversion from callback-based
doneto async/await withonce(app, 'error')is a great improvement. However, theapp.on('error', ...)handler on lines 20-22 is now redundant since the test awaits the error event on line 30 and performs the same assertion.test/response/flushHeaders.test.ts (1)
147-178: Remove redundant error handler and use consistent assertions.The async/await conversion is good, but there are two issues:
- The
app.once('error', ...)handler on lines 149-151 is redundant with theawait once(app, 'error')on line 176- Line 150 uses
assert.equalinstead ofassert.strictEqual, which is inconsistent with the strict assertion importApply this diff for consistency:
- app.once('error', err => { - assert.equal(err.message, 'mock error'); - });- assert.equal(err.message, 'mock error'); + assert.strictEqual(err.message, 'mock error');test/application/respond.test.ts (1)
674-692: Perfect implementation of previous suggestion.This change implements exactly what was suggested in the previous review - converting from callback-based error handling to
async/awaitwithonce(). The test is now more direct and easier to read.test/request/accepts.test.ts (1)
1-5: Imports look good – just verify TypeScript loader presenceSame remark about
.tsimports as above; once the loader is in place these changes are fine.test/application/use.test.ts (1)
2-2: Ensure loader for.tssource importsThe tests import
../../src/index.ts; confirm your test runner starts Node with an ESM TypeScript loader or uses pre-built JS.Also applies to: 6-6
test/request/acceptsLanguages.test.ts (1)
1-4: Verify.tshelper import works under node:testRequires the same loader / pre-compilation setup mentioned earlier.
test/response/last-modified.test.ts (1)
1-4: TypeScript helper import needs proper loaderMake sure the test runner invokes Node with a TS loader or uses compiled helpers.
🧹 Nitpick comments (5)
.oxlintrc.json (1)
149-154: Confirm new ignore-pattern still covers all example artefacts
"example/cjs/helloworld.js"replaces the old.cjspath, but the new example directory also contains apackage.jsonplus possible ancillary files. If those should stay outside linting, consider expanding the pattern to"example/cjs/**"instead of a single file.example/cjs/package.json (1)
1-11: Consider adding anenginesfield and pinning the @eggjs/koa version
The repo now mandates Node 22.17.1+. It’s helpful for downstream users if the example package advertises that via"engines": { "node": ">=22.17.1" }Also,
"@eggjs/koa": "alpha"resolves to the newest alpha each install and may break unexpectedly. Using an explicit prerelease tag ("1.0.0-alpha.x") or workspace reference keeps the example reproducible..github/workflows/node.js.yml (1)
16-16: LGTM! Node.js version update aligns with PR objectives.The update to test only Node.js versions 22 and 24 correctly reflects the new minimum version requirement of 22.17.1+. This removes testing for older unsupported versions (18, 20).
Consider being more specific about the minimum version if needed:
- version: '22, 24' + version: '22.17.1, 24'example/cjs/helloworld.js (1)
10-12: Consider making the port configurable.While the hardcoded port 3000 is fine for an example, consider documenting that users should customize the port for their needs.
-app.listen(3000); +const port = process.env.PORT || 3000; +app.listen(port); -console.log('Server is running on port 3000'); +console.log(`Server is running on port ${port}`);test/response/flushHeaders.test.ts (1)
95-144: Good async/await conversion, but consider simplification.The conversion from callback-based to Promise-based testing is correct and maintains the original test logic. The manual Promise construction with resolve/reject handlers properly replaces the previous
donecallback pattern.Consider using Node.js test utilities or creating a helper function if this Promise pattern is repeated across multiple tests to reduce boilerplate.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (96)
.github/workflows/node.js.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).gitignore(1 hunks).oxlintrc.json(1 hunks)AUTHORS(0 hunks)LICENSE(1 hunks)Readme.md(2 hunks)example/cjs/helloworld.js(1 hunks)example/cjs/package.json(1 hunks)example/esm/helloworld.js(1 hunks)example/esm/package.json(1 hunks)example/extend/Context.ts(1 hunks)example/extend/Request.ts(1 hunks)example/extend/middleware.ts(1 hunks)example/helloworld.cjs(0 hunks)example/helloworld.mjs(0 hunks)example/helloworld.ts(1 hunks)package.json(2 hunks)src/application.ts(1 hunks)src/context.ts(1 hunks)src/index.ts(1 hunks)src/request.ts(1 hunks)src/response.ts(1 hunks)test/application.test-d.ts(0 hunks)test/application/context.test.ts(1 hunks)test/application/currentContext.test.ts(1 hunks)test/application/index.test.ts(5 hunks)test/application/inspect.test.ts(1 hunks)test/application/onerror.test.ts(1 hunks)test/application/request.test.ts(1 hunks)test/application/respond.test.ts(7 hunks)test/application/response.test.ts(1 hunks)test/application/toJSON.test.ts(1 hunks)test/application/use.test.ts(3 hunks)test/context/assert.test.ts(1 hunks)test/context/cookies.test.ts(1 hunks)test/context/inspect.test.ts(1 hunks)test/context/onerror.test.ts(1 hunks)test/context/state.test.ts(1 hunks)test/context/throw.test.ts(1 hunks)test/context/toJSON.test.ts(1 hunks)test/index.test.ts(1 hunks)test/index.test.ts.snapshot(1 hunks)test/request/accept.test.ts(1 hunks)test/request/accepts.test.ts(1 hunks)test/request/acceptsCharsets.test.ts(1 hunks)test/request/acceptsEncodings.test.ts(1 hunks)test/request/acceptsLanguages.test.ts(1 hunks)test/request/charset.test.ts(1 hunks)test/request/fresh.test.ts(1 hunks)test/request/get.test.ts(1 hunks)test/request/header.test.ts(1 hunks)test/request/headers.test.ts(1 hunks)test/request/host.test.ts(1 hunks)test/request/hostname.test.ts(1 hunks)test/request/href.test.ts(1 hunks)test/request/idempotent.test.ts(1 hunks)test/request/inspect.test.ts(1 hunks)test/request/ip.test.ts(1 hunks)test/request/ips.test.ts(1 hunks)test/request/is.test.ts(1 hunks)test/request/length.test.ts(1 hunks)test/request/origin.test.ts(1 hunks)test/request/path.test.ts(1 hunks)test/request/protocol.test.ts(1 hunks)test/request/query.test.ts(1 hunks)test/request/querystring.test.ts(1 hunks)test/request/search.test.ts(1 hunks)test/request/secure.test.ts(1 hunks)test/request/stale.test.ts(1 hunks)test/request/subdomains.test.ts(1 hunks)test/request/type.test.ts(1 hunks)test/request/whatwg-url.test.ts(1 hunks)test/response/append.test.ts(1 hunks)test/response/attachment.test.ts(1 hunks)test/response/body.test.ts(1 hunks)test/response/etag.test.ts(1 hunks)test/response/flushHeaders.test.ts(5 hunks)test/response/has.test.ts(1 hunks)test/response/header.test.ts(1 hunks)test/response/headers.test.ts(1 hunks)test/response/inspect.test.ts(1 hunks)test/response/is.test.ts(1 hunks)test/response/last-modified.test.ts(1 hunks)test/response/length.test.ts(1 hunks)test/response/message.test.ts(1 hunks)test/response/redirect.test.ts(1 hunks)test/response/remove.test.ts(1 hunks)test/response/set.test.ts(1 hunks)test/response/socket.test.ts(1 hunks)test/response/status.test.ts(1 hunks)test/response/type.test.ts(1 hunks)test/response/vary.test.ts(1 hunks)test/response/writable.test.ts(1 hunks)test/test-helpers/context.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (4)
- example/helloworld.mjs
- test/application.test-d.ts
- example/helloworld.cjs
- AUTHORS
🧰 Additional context used
🧬 Code Graph Analysis (5)
example/esm/helloworld.js (1)
src/application.ts (1)
Application(33-322)
src/index.ts (1)
src/application.ts (1)
Application(33-322)
example/cjs/helloworld.js (1)
src/application.ts (1)
Application(33-322)
test/application/index.test.ts (3)
src/context.ts (1)
assert(92-108)test/test-helpers/context.ts (1)
request(25-27)src/application.ts (2)
keys(101-103)keys(105-107)
test/application/respond.test.ts (2)
src/context.ts (1)
assert(92-108)test/test-helpers/context.ts (1)
request(25-27)
🔇 Additional comments (132)
LICENSE (1)
4-4: Good update to maintain ongoing copyright coverageExtending the copyright notice to “2023-present” keeps the LICENSE current and avoids the need for yearly edits.
.gitignore (1)
13-13: 👍 Addtsconfig.tsbuildinfoto VCS ignore listIgnoring the TS build-info artefact prevents needless noise in commits and CI caches.
example/extend/middleware.ts (1)
1-1: LGTM: Import path updated correctly to TypeScript sourceThe change from
.jsto.tsextension aligns with the project's migration to TypeScript-first development and ensures proper module resolution and type safety..github/workflows/pkg.pr.new.yml (1)
15-15: Node.js version update aligns with breaking change requirementsThe workflow correctly updates Node.js from version 20 to 22, which is consistent with the PR's breaking change that drops support for Node.js versions below 22.17.1.
test/test-helpers/context.ts (1)
3-3: LGTM: Import path correctly updated to TypeScript sourceThe change from
application.jstoapplication.tsmaintains consistency with the project's migration to TypeScript source imports and ensures proper type resolution in test helpers.src/request.ts (1)
13-15: LGTM: TypeScript import paths updated consistentlyThe import path changes from
.jsto.tsextensions are correct and align with the project's migration to TypeScript-first development. This ensures proper module resolution and maintains type safety across the core source files.Readme.md (2)
3-3: Node.js version requirement correctly updatedThe documentation accurately reflects the breaking change by updating the minimum Node.js version from 18.19.0 to 22.17.1, providing clear guidance to users about the new requirements.
22-22: Installation requirements clearly communicatedThe installation section properly emphasizes the Node.js v22.17.1+ requirement for the new major version, helping users understand the breaking change before installation.
test/request/ip.test.ts (2)
2-2: LGTM: Explicit test function imports.Adding explicit imports for
describeanditfromnode:testimproves clarity and aligns with the migration to native Node.js testing.
5-6: LGTM: Import path migration to TypeScript.The update from
.jsto.tsextensions is correct and consistent with the broader TypeScript source migration across the codebase.test/index.test.ts (1)
1-11: LGTM: Export validation test.This new test file provides valuable validation of the Koa module exports during the TypeScript migration. The use of snapshot testing ensures export structure consistency, and the native Node.js test runner usage aligns with the modernization effort.
test/context/throw.test.ts (1)
2-2: LGTM: Consistent test modernization.The explicit import of test functions from
node:testand the update to TypeScript import paths are consistent with the broader migration strategy. No functional changes to test logic, which is appropriate.Also applies to: 6-6
src/application.ts (1)
14-17: LGTM: Core TypeScript import migration.The update of import paths from
.jsto.tsextensions for core Koa components (Context, Request, Response, and types) is essential for the TypeScript source migration. These changes maintain all functionality while enabling the new module resolution strategy.test/request/protocol.test.ts (1)
2-2: LGTM: Final consistent migration.The explicit import of test functions from
node:testand the TypeScript import path update complete the consistent migration pattern seen across all test files. The modernization maintains all test functionality while adopting native Node.js testing.Also applies to: 4-4
test/request/querystring.test.ts (2)
2-2: LGTM: Native Node.js test runner adoptionThe import of
describeanditfromnode:testaligns with the project's migration to native Node.js testing capabilities, which is appropriate given the minimum Node.js version requirement of 22.17.1.
6-6: LGTM: TypeScript module import consistencyThe change from
.jsto.tsextension for the context helper import maintains consistency with the project's TypeScript migration and ensures proper module resolution.test/context/onerror.test.ts (2)
2-2: LGTM: Native test runner integrationThe explicit import of test functions from
node:testis consistent with the project's modernization to use Node.js native testing capabilities.
8-9: LGTM: TypeScript import path updatesThe updates to use
.tsextensions for both the main index and context helper imports are consistent with the project's TypeScript migration and ensure proper module resolution.test/response/inspect.test.ts (2)
3-3: LGTM: Consistent test framework migrationThe import of test functions from
node:testmaintains consistency with the project-wide adoption of Node.js native testing capabilities.
5-5: LGTM: TypeScript helper importThe change to
.tsextension for the context helper import aligns with the project's TypeScript migration strategy.test/application/context.test.ts (2)
2-2: LGTM: Native Node.js test integrationThe explicit import of
describeanditfromnode:testis appropriate for leveraging Node.js native testing features, especially given the updated minimum Node.js version requirement.
6-6: LGTM: Main module TypeScript importThe update to import from
../../src/index.tscorrectly reflects the TypeScript source structure and ensures proper type resolution for theApplicationandContextclasses.test/response/header.test.ts (2)
2-2: LGTM: Test framework modernizationThe import from
node:testis consistent with the project's adoption of native Node.js testing capabilities.
6-7: LGTM: Consistent TypeScript importsBoth the context helper and main Koa module imports have been properly updated to use
.tsextensions, maintaining consistency with the project's TypeScript migration.test/context/cookies.test.ts (2)
2-2: LGTM! Explicit imports align with Node.js built-in test runner migration.The explicit import of
describeanditfromnode:testis consistent with the project's migration to use Node.js's built-in test runner instead of external testing frameworks.
6-6: LGTM! Import path updated to reference TypeScript source files.The change from
.jsto.tsextension correctly reflects the project's transition to reference TypeScript source files directly, which is consistent with the ESM-only migration.test/response/redirect.test.ts (2)
2-2: LGTM! Consistent with Node.js built-in test runner migration.The explicit import of test functions from
node:testaligns with the project-wide standardization on Node.js's native test runner.
6-7: LGTM! Import paths updated to TypeScript extensions.Both helper and main module imports have been correctly updated to use
.tsextensions, maintaining consistency with the project's TypeScript source file migration.test/response/attachment.test.ts (2)
2-2: LGTM! Test runner import standardization.The explicit import from
node:testis consistent with the project's adoption of Node.js's built-in test runner capabilities.
6-7: LGTM! TypeScript source file imports.The imports have been correctly updated to reference
.tsfiles, aligning with the project's migration to use TypeScript source files directly in the ESM-only setup.test/request/whatwg-url.test.ts (2)
2-2: LGTM! Node.js test runner import.The explicit import of test functions maintains consistency with the project's standardization on Node.js's native test capabilities.
4-4: LGTM! Helper import updated to TypeScript.The import path correctly references the TypeScript version of the context helper, consistent with the project's migration pattern.
test/response/headers.test.ts (2)
2-2: LGTM! Built-in test runner adoption.The explicit import from
node:testfollows the project's consistent pattern of using Node.js's native test runner functions.
4-4: LGTM! TypeScript helper import.The import path has been appropriately updated to reference the TypeScript version of the context helper, maintaining alignment with the project's ESM and TypeScript migration.
test/response/message.test.ts (2)
2-2: LGTM: Migrated to Node.js native test runner.The explicit import of
describeanditfromnode:testaligns with the project's migration to use the native Node.js test runner, which is consistent with the minimum Node.js version requirement of 22.17.1+.
4-4: LGTM: Updated import path to TypeScript.The import path change from
context.jstocontext.tsis consistent with the project's migration to TypeScript source files.test/response/status.test.ts (2)
2-2: LGTM: Migrated to Node.js native test runner.The explicit import of test functions from
node:testis consistent with the project-wide migration to the native Node.js test runner, which is supported in Node.js 22.17.1+.
7-8: LGTM: Updated import paths to TypeScript.Both import path changes from
.jsto.tsextensions are consistent with the project's migration to TypeScript source files and align with the broader refactoring effort.test/application/currentContext.test.ts (2)
2-2: LGTM: Migrated to Node.js native test runner.The explicit import of
describeanditfromnode:testis consistent with the project-wide standardization on the native Node.js test runner.
5-5: LGTM: Updated import path to TypeScript.The import path change from
index.jstoindex.tsaligns with the project's migration to TypeScript source files.test/request/charset.test.ts (2)
2-2: LGTM: Migrated to Node.js native test runner.The explicit import of
describeanditfromnode:testfollows the consistent pattern of migrating to the native Node.js test runner across the project.
4-4: LGTM: Updated import path to TypeScript.The import path change from
context.jstocontext.tsis consistent with the project's migration to TypeScript source files.test/context/toJSON.test.ts (2)
2-2: LGTM: Migrated to Node.js native test runner.The explicit import of
describeanditfromnode:testis consistent with the project-wide adoption of the native Node.js test runner.
4-4: LGTM: Updated import path to TypeScript.The import path change from
context.jstocontext.tsaligns with the project's migration to TypeScript source files.test/context/assert.test.ts (1)
2-4: Runtime can’t load “.ts” files without a loaderThe test now imports helpers via
import context from '../test-helpers/context.ts';
and relies on Node’s built-in runner (node:test).
Vanilla Node (even 22.x) will not execute a plain.tsfile unless a loader such astsx,ts-node, or a pre-compilation step is configured.Double-check the test command in
package.json/ CI to ensure a loader is in place, otherwise the suite will fail at runtime.src/context.ts (1)
10-13: Type-only imports look goodThe switch to
.tsextensions is safe here becauseimport typestatements are erased from emitted JS, so no runtime path issues are introduced.test/context/state.test.ts (1)
2-7: Ensure build step rewrites the Koa import
import Koa from '../../src/index.ts';is a runtime import, not type-only. After tsc transpilation this path will remain.tsunless you compile the test file itself (which then rewrites to.js). If tests are executed un-transpiled with a loader, you’re fine; if they’re run after compilation, the relative path will break (*.jsfile won’t be found).Confirm the chosen strategy is consistent across the test suite.
test/request/idempotent.test.ts (2)
2-2: LGTM: Migration to explicit Node.js test importsThe explicit import of
describeanditfromnode:testimproves clarity and aligns with the project's migration to native Node.js testing APIs.
4-4: LGTM: TypeScript source import migrationThe import path change from
.jsto.tsis consistent with the project-wide migration to TypeScript source imports and ensures proper module resolution.src/index.ts (2)
1-1: LGTM: TypeScript source import for ApplicationThe import path change from
./application.jsto./application.tsaligns with the project's migration to TypeScript source imports and ensures consistency across the codebase.
5-9: LGTM: Comprehensive TypeScript source exportsAll export statements have been properly updated to reference TypeScript source files (
.ts) instead of JavaScript files. This change is consistent with the project-wide migration and maintains the same exported API surface.test/application/onerror.test.ts (2)
2-2: LGTM: Enhanced test importsThe explicit import of
describe,it, andafterEachfromnode:testimproves test clarity and standardizes the use of native Node.js testing APIs across the project.
7-7: LGTM: TypeScript source import alignmentThe import path change from
../../src/index.jsto../../src/index.tsis consistent with the project's migration to TypeScript source imports and ensures proper module resolution in the test environment.example/esm/package.json (1)
1-11: LGTM: Well-structured ES module exampleThe package configuration correctly sets up an ES module example with:
- Proper
"type": "module"declaration- Appropriate dependency on the alpha version of
@eggjs/koa- Simple start script for demonstration purposes
This provides a clear example for users migrating to ES modules with the new Koa version.
test/response/writable.test.ts (3)
1-1: LGTM: Improved import organizationMoving the
Servertype import to the top improves code organization and follows TypeScript import conventions.
4-4: LGTM: Native Node.js test importsThe explicit import of
describeanditfromnode:teststandardizes the use of native Node.js testing APIs and improves test clarity.
7-7: LGTM: TypeScript source import consistencyThe import path change from
../../src/index.jsto../../src/index.tsaligns with the project-wide migration to TypeScript source imports and ensures proper module resolution.test/request/inspect.test.ts (2)
2-2: 👍 Switched to built-innode:testAPI
Explicitly importingdescribe/itfromnode:testis the recommended way once the minimum Node version is ≥ 20. Your change aligns perfectly with the PR’s “Node 22+ only” direction.
6-6: Verify that.tshelper files are runnable under the new test runner
node:testwill attempt to load the file literally as../test-helpers/context.ts. Node itself cannot execute TypeScript; you need either
- a loader flag (e.g.
--import ts-node/register) or- the helpers pre-compiled to
.js.Double-check the test command in package.json / CI to make sure this path still resolves.
test/response/set.test.ts (1)
1-5: Imports modernised — make sure the runner still transpiles.tshelpers
Same comment as above:node:testplus a raw.tsspecifier requires an appropriate loader hook. If CI previously relied on Babel/Jest it may silently stop resolving.test/request/header.test.ts (1)
1-4: Same.tshelper path caveat applies – be sure the runner can loadcontext.ts.test/request/acceptsEncodings.test.ts (1)
1-5: Loader requirement for TypeScript helper
All updated tests now point to.tshelpers; verify that the chosen runner (e.g.tsx,ts-node, or a pre-build step) is wired up, otherwise Node will throwERR_UNKNOWN_FILE_EXTENSION.test/request/type.test.ts (1)
1-4: LGTM! Modernization changes align with ESM-only transition.The import updates are consistent and appropriate:
- Strict assertions provide better error messages and type safety
- Explicit test function imports align with native Node.js test runner
- TypeScript helper imports support the ESM-only approach
example/helloworld.ts (1)
1-11: LGTM! Clean TypeScript example demonstrating ESM-only usage.The example effectively demonstrates:
- Direct import from TypeScript source files (line 1)
- Modern async middleware pattern (lines 5-7)
- Simple server setup appropriate for documentation/learning
This aligns well with the ESM-only transition and provides a clear usage example.
test/request/subdomains.test.ts (1)
1-4: LGTM! Consistent modernization following established pattern.The import updates are identical to other test files in this PR, maintaining consistency across the test suite while supporting the ESM-only transition.
test/response/type.test.ts (1)
1-4: LGTM! Modernization changes consistent with test suite updates.The import pattern matches other test files in this PR. The default import of
context(vs named import) appears intentional based on the helper module's export structure.test/request/acceptsCharsets.test.ts (1)
1-4: LGTM! Final test file completes consistent modernization pattern.The import updates match the established pattern across all test files, successfully supporting the ESM-only transition with appropriate strict assertions and TypeScript module resolution.
test/response/socket.test.ts (1)
1-7: Imports modernised correctlySwitching to
node:assert/strict, explicitdescribe/itimports fromnode:test, and the.tshelper path aligns with the new ESM + TS test strategy. No further changes needed.test/response/has.test.ts (1)
1-5: Test harness update looks goodThe stricter assert and TypeScript helper import are consistent with the repo-wide migration; behaviour and coverage remain unchanged.
test/response/etag.test.ts (1)
1-5: LGTM – consistent with ESM/TS transitionImports are upgraded to strict-mode variants and reference the new
.tshelper. Tests still exercise the same edge-cases for ETag handling.test/response/vary.test.ts (1)
1-5: Imports updated correctlyNothing else changed; tests still validate
varyheader logic across scenarios. ✅test/request/host.test.ts (1)
1-5: Import updates consistent with rest of suite
node:assert/strict,node:test, and.tshelper path match the new ESM/TypeScript setup. Existing host-resolution scenarios remain intact.test/request/query.test.ts (1)
1-4: LGTM! Modernized test imports align with ESM-only migration.The import changes are well-executed:
node:assert/strictprovides better type safety and stricter assertions- Explicit imports from
node:testalign with modern Node.js testing patterns- Migration from
.jsto.tsextension reflects the TypeScript source consolidationtest/request/fresh.test.ts (1)
1-4: LGTM! Consistent import modernization.The import updates follow the same modernization pattern seen across the test suite, ensuring consistency in the ESM-only migration.
test/index.test.ts.snapshot (1)
1-15: LGTM! API surface snapshots ensure export consistency.The snapshots capture the expected public API after the ESM migration, including core Koa components (Application, Context, Request, Response) and HttpError. This helps prevent accidental breaking changes to the public interface.
test/response/remove.test.ts (1)
1-4: LGTM! Consistent import modernization maintained.The import updates maintain consistency with the broader test suite modernization effort.
test/request/ips.test.ts (1)
1-4: LGTM! Import modernization with appropriate named import.The import updates are consistent with the ESM migration. The named import
{ request }from the test helpers is appropriate and aligns with the TypeScript source consolidation.example/esm/helloworld.js (1)
1-13: Excellent ESM example implementation!This example effectively demonstrates the framework's ESM usage pattern:
- Proper ES module import syntax
- Clean middleware setup using arrow function
- Standard server listening on port 3000
- User-friendly console feedback
The implementation correctly uses the
Applicationclass API as defined insrc/application.tsand provides a clear, minimal example for ESM users.test/request/path.test.ts (1)
1-6: LGTM! Test modernization is well-executed.The updates effectively modernize the test setup:
node:assert/strictprovides better assertion behavior- Explicit
node:testimports improve clarity- TypeScript helper import aligns with codebase migration
These changes are consistent with the broader test suite modernization effort.
test/request/get.test.ts (1)
1-4: LGTM! Consistent test modernization.Same excellent modernization pattern as other test files - strict assertions, explicit node:test imports, and TypeScript helper usage.
test/application/response.test.ts (1)
1-6: LGTM! Test modernization with important main module update.The modernization follows the established pattern with strict assertions and explicit node:test imports. The change from
index.jstoindex.tsis particularly significant, indicating the codebase now treats TypeScript as the primary source format.test/request/origin.test.ts (2)
1-2: LGTM: Test infrastructure modernizationThe switch to strict assertions and explicit test function imports aligns well with the ESM migration and Node.js 22.17.1+ requirement.
5-5: LGTM: TypeScript file extension updateThe import path change from
.jsto.tsis consistent with the codebase migration to TypeScript-first source files.test/request/headers.test.ts (2)
1-2: LGTM: Consistent test modernizationThe strict assertions and explicit test imports follow the same pattern across test files, providing consistency in the test infrastructure.
4-4: LGTM: Named import with TypeScript extensionThe named import of
requesthelper with.tsextension is correct and consistent with the codebase migration.test/request/is.test.ts (1)
1-4: LGTM: Consistent modernization patternThe import changes follow the established pattern across test files - strict assertions, explicit test imports, and TypeScript helper imports. The comprehensive test coverage for
ctx.is()is preserved.test/request/length.test.ts (1)
1-4: LGTM: Standard modernization changesThe import updates are consistent with the ESM migration pattern - strict assertions, explicit Node.js test imports, and TypeScript file extensions for helpers.
test/request/accept.test.ts (2)
1-2: LGTM: Final file completes consistent patternThe strict assertions and explicit test imports complete the consistent modernization pattern across all reviewed test files.
6-6: LGTM: Mixed import pattern with TypeScript extensionThe combination of default import (
context) and named import with alias ({ request as Request }) from the TypeScript helper is syntactically correct and maintains the established modernization pattern.test/response/length.test.ts (3)
2-2: Good modernization to strict assertions.The import change to
node:assert/strictenables stricter assertion methods, which helps catch potential issues earlier and improves test reliability.
3-3: Good explicit import of test functions.Explicitly importing
describeanditfromnode:testclearly indicates the use of Node.js built-in test runner and improves code clarity.
5-5: Good migration to TypeScript helper imports.The change from
.jsto.tsimport path correctly reflects the TypeScript migration and ensures proper module resolution.test/response/body.test.ts (2)
2-3: LGTM - Consistent test modernization.The changes to use strict assertions and explicit Node.js test runner imports improve test reliability and clarity.
6-6: LGTM - Correct TypeScript import path.The change to
.tsextension correctly reflects the TypeScript migration.test/request/search.test.ts (2)
1-2: LGTM - Consistent test modernization.The changes to use strict assertions and explicit Node.js test runner imports align with the project-wide testing improvements.
4-4: LGTM - Correct TypeScript import path.The change to
.tsextension properly reflects the TypeScript migration.test/response/append.test.ts (2)
1-2: LGTM - Consistent test modernization.The import changes to use strict assertions and explicit Node.js test runner functions improve test quality and clarity.
4-4: LGTM - Correct TypeScript import path.The change to
.tsextension correctly reflects the TypeScript migration.example/cjs/helloworld.js (3)
1-1: LGTM - Correct CommonJS import.The CommonJS require syntax correctly imports the Application class from the @eggjs/koa package.
3-3: LGTM - Proper Application instantiation.The Application instance is created correctly, following the standard Koa pattern.
5-8: LGTM - Clear middleware example.The middleware demonstrates the basic Koa pattern effectively. The "CJS" suffix in the message clearly identifies this as the CommonJS example.
test/response/is.test.ts (1)
1-4: LGTM! Test modernization aligns with ESM-only migration.The changes properly modernize the test setup by:
- Using strict assertions for more rigorous testing
- Explicitly importing Node.js native test runner functions
- Updating import paths to TypeScript files
These changes are consistent with the Node.js 22.17.1+ requirement and ESM-only approach.
test/context/inspect.test.ts (1)
1-6: LGTM! Consistent test modernization.The import updates follow the same modernization pattern as other test files in this PR:
- Strict assertions for better test reliability
- Native Node.js test runner imports
- TypeScript file imports
The formatting improvement with the added blank line also enhances readability.
tsconfig.json (2)
3-12: Excellent TypeScript configuration for ESM-only setup.The comprehensive compiler options are well-chosen for the migration:
erasableSyntaxOnlyandverbatimModuleSyntaxensure proper ESM handlingrewriteRelativeImportExtensionsenables .ts imports while outputting .js- ES2024 target properly leverages Node.js 22.17.1+ features
- Clear separation of source (
./src) and output (./dist) directoriesThe commented
noEmitoption provides flexibility for type-checking vs compilation modes.
13-14: Proper directory inclusion/exclusion setup.Including only
srcwhile excludingtestandexampleensures the build process focuses on the source code, which aligns with the new build structure.test/application/index.test.ts (5)
2-4: LGTM! Proper imports for modernized testing.The addition of Node.js native test runner imports and the
onceutility fromnode:eventsproperly supports the async/await refactoring.
9-9: LGTM! TypeScript import path update.Updating the Koa import to use the
.tsextension aligns with the TypeScript migration across the codebase.
34-69: LGTM! Clean async/await pattern for event testing.The refactoring properly uses
await once()to wait for request and response events, making the test logic clearer and more reliable than the previous callback approach.
71-99: LGTM! Proper async handling for socket writability test.The conversion to async/await with
await once(app, 'request')and the simplifiedsetImmediatecall maintains the test's timing behavior while removing callback complexity.
122-122: Minor test data update noted.The cookie key change from
'customkey'to'custom-key'appears to be a minor test data adjustment, possibly for consistency with naming conventions.test/response/flushHeaders.test.ts (1)
2-10: LGTM! Consistent modernization imports.The import updates follow the established pattern: Node.js native test runner, event utilities, and TypeScript file paths. The addition of
AddressInfotype import is appropriate for the server address handling.test/application/respond.test.ts (5)
2-2: Good modernization to native Node.js testing.The explicit import of test functions from
'node:test'aligns well with the project's move to native Node.js testing modules and improves code clarity.
9-9: Correct import path update for TypeScript migration.The import path change from
.jsto.tsproperly reflects the project's migration to TypeScript source files.
10-10: Appropriate utility import for async error handling.The
onceimport from'node:events'is properly added to support the modernized async/await error handling patterns in the tests.
576-596: Excellent modernization of async error handling.The refactor from callback-based error handling to
async/awaitwithonce()makes the test more readable and follows modern async patterns. The logic correctly initiates a request and waits for the error event.
611-631: Consistent async pattern implementation.The test follows the same improved async/await pattern with
once()for error handling, maintaining consistency with other modernized tests in the file.package.json (8)
3-3: Appropriate version bump for breaking changes.The version bump to
3.0.0-alpha.0correctly reflects the major breaking changes (Node.js version requirement) and the alpha pre-release nature.
8-9: Correct alpha release configuration.Adding the
"tag": "alpha"to publish configuration ensures the alpha version won't be tagged aslatest, which is the proper approach for pre-release versions.
14-22: Excellent modernization of build and test scripts.The changes represent significant improvements:
- Native Node.js test runner with TypeScript strip support
c8for modern coverage reporting- TypeScript-based build system with cross-platform clean command
The clean script now uses
tsc --build --cleanwhich resolves the cross-platform issues mentioned in previous reviews.
77-77: Dependency updates support Node.js version requirement.The
@types/nodeupdate to version 24 properly supports the new Node.js >= 22.17.1 requirement.
84-84: Modern coverage tooling addition.Adding
c8provides modern coverage reporting that aligns with the native Node.js testing approach.
91-91: TypeScript version supports modern features.The TypeScript update to
^5.8.3provides modern language features and aligns with the project's modernization goals.
94-99: Simplified export structure for ESM-only package.The simplified exports configuration with single entry point
"./dist/index.js"aligns with the "ESM only" objective. Including bothdistandsrcin files provides access to source code alongside the compiled output.
5-5: Node.js engine requirement validatedThe specified minimum Node.js version (
>= 22.17.1) is correct and matches the latest LTS release (22.17.1), which was published on July 16, 2025. No further action is needed.test/request/href.test.ts (1)
7-8: Direct.tsimport requires an ESM TypeScript loader
import Koa from '../../src/index.ts'is fine only if tests run with a loader such astsx,ts-node/esm, or you pre-compile the sources.
Please ensure the test script (npm test, CI) starts Node with that loader (e.g.node --loader ts-node/esm …) or point the import at the emitted JavaScript.test/request/hostname.test.ts (1)
1-4: LGTM! Excellent modernization of test imports.The changes improve test reliability and align with modern Node.js practices:
- Strict assertions provide better error messages and type safety
- Explicit test function imports clarify dependencies on native Node.js test runner
- TypeScript import paths support the ESM-only migration
test/application/toJSON.test.ts (2)
1-4: LGTM! Consistent modernization of imports.The import changes align perfectly with the ESM-only migration and modern testing practices.
8-9: Good practice: Explicit environment setup.Setting
NODE_ENV = 'test'explicitly makes the test more reliable by removing dependency on external environment configuration.test/application/request.test.ts (1)
1-6: LGTM! Consistent with ESM modernization.The import changes follow the same beneficial pattern seen across the test suite.
test/application/inspect.test.ts (2)
1-5: LGTM! Consistent modernization approach.The import changes maintain consistency with the ESM-only migration strategy.
8-8: Excellent: Explicit environment configuration.Setting
NODE_ENVexplicitly improves test reliability and removes external dependencies.test/request/stale.test.ts (1)
1-4: LGTM! Completes the consistent modernization pattern.The import changes perfectly align with the ESM-only migration seen across all test files, demonstrating excellent consistency in the modernization effort.
[skip ci] ## [3.0.0](v2.22.2...v3.0.0) (2025-07-30) ### ⚠ BREAKING CHANGES * drop Node.js < 22.17.1 support <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added example projects demonstrating usage with both CommonJS and ES modules. * Introduced a new TypeScript example for quick server setup. * **Breaking Changes** * Minimum required Node.js version raised to 22.17.1. * Major version bumped to 3.0.0-alpha.0 with updated package structure and exports. * **Documentation** * Updated installation instructions and Node.js version requirements in the README. * Removed the AUTHORS file. * **Chores** * Updated development dependencies and scripts for modern Node.js and TypeScript. * Streamlined configuration files and ignored build artifacts. * Adjusted GitHub Actions workflows to test fewer Node.js versions and support all branches on merge groups. * **Tests** * Migrated tests to use Node.js native test runner. * Improved test strictness and modernized async control flow. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * esm only ([#24](#24)) ([e0c8218](e0c8218))
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
package.json (3)
16-17: Coverage script works but misses--all.Using
c8directly is fine; consider adding--allto include files not touched by tests for more accurate %.-"ci": "c8 -r html -r lcov -r text npm test", +"ci": "c8 --all -r html -r lcov -r text npm test",
94-95: Single entry inexportsis fine – adddefault?With
"type":"module"consumers usingimport pkg from '@eggjs/koa'may expect adefaultexport. If you intentionally re-export everything, great; otherwise add adefaultmapping.export * from './dist/index.js'; export { default } from './dist/index.js';
99-99: Includingsrcin the published tarball – intentional?If users shouldn’t import TS sources, drop
"src"fromfilesto reduce package size.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
package.json(3 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tsconfig.json
🔇 Additional comments (9)
package.json (9)
3-3: Version bump looks OK – remember downstream packages.
3.0.0-alpha.0correctly signals a breaking change + pre-release. Just verify any other packages in the monorepo (examples, plugins, etc.) have their peer/dep ranges updated.
5-6: Ensure CI & docs adopt the Node 22.17.1 floor.The new engine constraint is fine, but double-check:
• CI images use ≥ 22.17.1
•README, examples, and Dockerfiles mention the new minimum.
8-10:publishConfig.tag: "alpha"is appropriate.No issues – this prevents accidental
latestpromotion during the pre-release phase.
19-22: Clean/build script is now cross-platform – nice fix.
The previousrm -rf distportability issue is gone; usingtsc --build --cleanis the right call.
68-68: @eggjs/tsconfig major bump acknowledged.Assuming the 3.x config exports
moduleResolution: "nodenext"&target≥ 2023, this aligns with the ESM-only move.
77-77: Node types 24 vs runtime 22 – verify compatibility.
@types/node@24may expose APIs not present until Node 24. To avoid type/feature drift, consider pinning to^22.- "@types/node": "24", + "@types/node": "22",
84-84:c8@10is the correct major for Node 22.LGTM.
91-91: TypeScript 5.8.3 matches--experimental-strip-types.No issues.
14-15: Test-runner flags are valid
Both--test-force-exitand--test-update-snapshotsare supported by Node’s built-in test runner (confirmed in v24.3.0). You can keep your existing"test"script unchanged.Likely an incorrect or invalid review comment.
1 participant
BREAKING CHANGE: drop Node.js < 22.17.1 support
Summary by CodeRabbit
New Features
Breaking Changes
Documentation
Chores
Tests