feat: support cjs and esm both by tshy #26
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
WalkthroughThe pull request represents a comprehensive modernization of the Changes
Sequence DiagramsequenceDiagram
participant App as Egg Application
participant Static as Static Middleware
participant FileSystem as File System
participant Cache as LRU Cache
App->>Static: Configure static serving
Static->>FileSystem: Check directory existence
FileSystem-->>Static: Confirm directories
Static->>Cache: Initialize file cache
App->>Static: Receive HTTP request
Static->>FileSystem: Retrieve static file
FileSystem-->>Static: Return file contents
Static->>App: Serve file response
Poem
Finishing Touches
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, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/[email protected], npm/[email protected], npm/[email protected] |
commit: |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (2)
test/static.test.ts (1)
Line range hint
42-42: Fix Content-Range assertion failuresMultiple tests are failing due to content length mismatch. The tests expect a content length of 20 bytes, but the actual content appears to be 21 bytes. This could be due to:
- Line ending differences (CRLF vs LF)
- Changes in the test fixture content
Verify the content of
foo.jsin the fixtures and ensure consistent line endings:#!/bin/bash # Check the actual content length and line endings find test/fixtures -name 'foo.js' -exec wc -c {} \; find test/fixtures -name 'foo.js' -exec file {} \;Also applies to: 124-124, 170-170
🧰 Tools
🪛 Biome (1.9.4)
[error] 8-11: Do not export from a test file.
(lint/suspicious/noExportsInTest)
CHANGELOG.md (1)
Line range hint
1-16: Add changelog entry for current changesPlease add a changelog entry for the current changes that includes:
- Breaking change: Drop support for Node.js < 18.19.0
- Feature: Support for both CJS and ESM using tshy
- Package rename to @eggjs/static
Add the following entry at the top of the changelog:
# Changelog +## [3.0.0](https://github.com/eggjs/egg-static/compare/v2.3.1...v3.0.0) (2025-01-xx) + +### ⚠ BREAKING CHANGES + +* Drop support for Node.js versions earlier than 18.19.0 +* Package renamed to @eggjs/static + +### Features + +* Support both CommonJS (cjs) and ECMAScript Modules (esm) using tshy + ## [2.3.1](https://github.com/eggjs/egg-static/compare/v2.3.0...v2.3.1) (2023-02-13)🧰 Tools
🪛 Markdownlint (0.37.0)
20-20: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
26-26: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
32-32: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
41-41: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
48-48: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
53-53: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
58-58: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
63-63: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
69-69: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
75-75: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
80-80: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
85-85: Expected: atx; Actual: setext
Heading style(MD003, heading-style)
24-24: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
30-30: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
36-36: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
39-39: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
45-45: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
46-46: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
51-51: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
56-56: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
61-61: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
66-66: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
67-67: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
72-72: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
73-73: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
78-78: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
83-83: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
88-88: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
23-23: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
29-29: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
35-35: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
38-38: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
44-44: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
🧹 Nitpick comments (6)
src/config/config.default.ts (1)
7-17: Consider documenting maxFiles limit rationale.The configuration includes a
maxFiles: 1000limit. Consider adding a comment explaining the rationale behind this specific limit to help future maintainers understand if/when it needs adjustment.src/types.ts (1)
46-57: Consider migration guide for deprecated dirs property.Since
dirsis marked as deprecated in favor ofdir, consider adding a comment with a migration example to help users transition their configurations.Example addition to the JSDoc:
/** * static files store dirs * @deprecated use `dir` instead + * @example + * // Old way + * dirs: ['/path1', { prefix: '/static2', dir: '/path2' }] + * // New way + * dir: ['/path1', { prefix: '/static2', dir: '/path2' }] */src/app/middleware/static.ts (2)
20-20: Consider improving type safetyThe type assertion
as anycould be replaced with a proper type definition.- return range(ctx as any, next); + return range(ctx as Parameters<typeof range>[0], next);
27-45: Consider adding path validationWhile the implementation handles both string and object configurations well, consider adding validation for:
- Absolute vs relative paths
- Path traversal protection
- Directory existence before spread operation
test/static.test.ts (1)
9-11: Consider moving test utility to a separate fileThe
getFixturesutility function is currently exported from the test file, which is flagged by static analysis. Consider moving it to a separate test utilities file to maintain better organization.🧰 Tools
🪛 Biome (1.9.4)
[error] 8-11: Do not export from a test file.
(lint/suspicious/noExportsInTest)
README.md (1)
48-53: Enhance TypeScript configuration exampleThe TypeScript configuration example could be improved with type information and comments explaining the available options.
Consider enhancing the example:
// {app_root}/config/config.default.ts -export default { +import { EggAppConfig, PowerPartial } from 'egg'; + +export default () => { + const config = {} as PowerPartial<EggAppConfig>; + + // Static file serving configuration + // See https://github.com/eggjs/koa-static-cache for all available options + config.static = { + // maxAge: 31536000, + // dir: 'path/to/static/files', + // dynamic: true, + // preload: false, + }; + + return config; - static: { - // maxAge: 31536000, - }, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
.eslintrc(1 hunks).github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).github/workflows/release.yml(1 hunks).gitignore(1 hunks)CHANGELOG.md(1 hunks)History.md(0 hunks)README.md(3 hunks)app.js(0 hunks)app/middleware/static.js(0 hunks)config/config.default.js(0 hunks)config/config.prod.js(0 hunks)package.json(1 hunks)src/app.ts(1 hunks)src/app/middleware/static.ts(1 hunks)src/config/config.default.ts(1 hunks)src/config/config.prod.ts(1 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)test/static.test.ts(5 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (6)
- .github/PULL_REQUEST_TEMPLATE.md
- History.md
- config/config.prod.js
- app.js
- config/config.default.js
- app/middleware/static.js
✅ Files skipped from review due to trivial changes (2)
- src/index.ts
- src/typings/index.d.ts
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 1800 characters long)
Context: ...c [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] [
[uncategorized] ~44-~44: Possible missing article found.
Context: ... all the files are lazy loaded.** - In non-production environment, assets won't be...
(AI_HYDRA_LEO_MISSING_A)
🪛 Biome (1.9.4)
test/static.test.ts
[error] 8-11: Do not export from a test file.
(lint/suspicious/noExportsInTest)
🪛 GitHub Actions: CI
test/static.test.ts
[error] 42-42: Test failure: Content-Range assertion failed - expected 'bytes 0-10/20', got 'bytes 0-10/21'
[error] 124-124: Test failure: Content-Range assertion failed - expected 'bytes 0-10/20', got 'bytes 0-10/21'
[error] 170-170: Test failure: Content-Range assertion failed - expected 'bytes 0-10/20', got 'bytes 0-10/21'
🪛 Markdownlint (0.37.0)
CHANGELOG.md
20-20: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
26-26: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
32-32: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
41-41: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
48-48: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
53-53: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
58-58: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
63-63: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
69-69: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
75-75: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
80-80: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
85-85: Expected: atx; Actual: setext
Heading style
(MD003, heading-style)
24-24: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
30-30: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
36-36: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
39-39: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
45-45: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
46-46: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
51-51: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
56-56: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
61-61: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
66-66: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
67-67: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
72-72: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
73-73: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
78-78: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
83-83: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
88-88: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
23-23: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
29-29: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
35-35: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
38-38: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
44-44: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (16)
src/config/config.prod.ts (1)
1-8: LGTM! Production configuration follows best practices.The configuration uses appropriate values for production:
maxAge: 31536000(1 year) for optimal browser cachingbuffer: truefor improved performancesrc/config/config.default.ts (1)
1-4: LGTM! Modern import usage.Using the
node:protocol for built-in modules is a good practice in modern Node.js.src/app.ts (1)
8-17: Verify middleware ordering edge cases.The current implementation ensures 'static' middleware is before 'bodyParser', but consider these scenarios:
- What happens if other core middleware needs to be before 'static'?
- Should we validate that 'static' isn't already in the middleware array?
Let's check the codebase for any other middleware ordering requirements:
✅ Verification successful
Current middleware ordering implementation is correct and sufficient.
The implementation correctly ensures that static middleware is placed before bodyParser, which is the only critical ordering requirement found in the codebase. The code already handles both cases (bodyParser present/absent) appropriately, and the static middleware is guaranteed to be added exactly once.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for middleware ordering patterns in the codebase rg -A 3 "coreMiddleware\.(indexOf|push|splice)"Length of output: 361
Script:
#!/bin/bash # Search for static middleware configuration and documentation echo "=== Searching for static middleware configuration ===" rg -i "static.*middleware" -A 3 echo -e "\n=== Checking for middleware documentation ===" fd "README|middleware" --type f --exec cat {} echo -e "\n=== Checking if static middleware exists multiple times ===" rg -w "static" --type tsLength of output: 6564
src/types.ts (2)
3-39: LGTM! Well-documented interface with comprehensive options.The
StaticDirOptionsinterface is well-structured with detailed JSDoc comments for each option.
59-64: LGTM! Proper module augmentation.The module augmentation for
@eggjs/corecorrectly extends the configuration types.src/app/middleware/static.ts (1)
1-9: Well-structured imports following modern practices!Good use of:
- Node.js prefixed imports
- Separate type imports
- ESM import syntax
.eslintrc (1)
2-5: Good ESLint configuration for TypeScript migration!The updated configuration properly supports:
- TypeScript-specific linting rules
- Node.js import prefix enforcement
.gitignore (1)
5-11: Comprehensive updates to .gitignore!Good additions for:
- TypeScript build artifacts (.tshy*)
- Development tools cache (.eslintcache)
- Build output (dist)
- System files (.DS_Store)
- Package manager files
tsconfig.json (1)
1-10: Well-configured TypeScript settings for dual CJS/ESM support!The configuration properly enables:
- Strict type checking for better type safety
- ES2022 target which aligns with Node.js 18.19.0+ requirement
- NodeNext module system for proper ESM/CJS interop
.github/workflows/release.yml (1)
Line range hint
1-14: Verify required secrets are configuredThe workflow requires NPM_TOKEN and GIT_TOKEN secrets for publishing. Please ensure these are properly configured in the repository settings.
Run this script to check the secrets configuration:
.github/workflows/nodejs.yml (1)
Line range hint
1-16: Node.js versions properly aligned with requirements!The CI workflow correctly tests against Node.js 18.19.0+ as per the PR objectives.
Verify the CODECOV_TOKEN secret is configured:
package.json (4)
2-6: Package name and access properly configuredThe package is correctly scoped under @eggjs with public access.
8-14: Proper module format configuration in eggPluginThe exports field correctly specifies paths for:
- ESM imports
- CommonJS requires
- TypeScript source
73-92: Excellent dual package hazard handling!The exports field correctly:
- Provides separate paths for CJS and ESM
- Includes proper type definitions for both formats
- Sets appropriate main/module/types fields
31-54: Verify dependency compatibilityPlease verify the compatibility of dependencies with the dual CJS/ESM format, particularly:
- @eggjs/core@^6.2.13
- @eggjs/koa-static-cache@^6.0.0
Run this script to check package exports:
✅ Verification successful
Dependencies are properly configured for dual CJS/ESM format
The package is correctly set up with dual format support through tshy, with proper exports configuration for both ESM and CommonJS. The TypeScript configuration also aligns with this dual format approach.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if dependencies support both CJS and ESM # Note: This uses @arethetypeswrong/cli which is already in devDependencies for pkg in "@eggjs/[email protected]" "@eggjs/[email protected]"; do echo "Checking $pkg..." npx attw $pkg doneLength of output: 554
Script:
#!/bin/bash # Check package.json and tshy configuration for dual format support echo "=== Package Configuration ===" cat package.json | grep -A 2 '"type":\|"exports":\|"tshy":' echo -e "\n=== TypeScript Configuration ===" fd -e json -e jsonc tsconfig && cat tsconfig*Length of output: 868
README.md (1)
8-8: 🛠️ Refactor suggestionAdd minimum Node.js version requirement
Since this PR drops support for Node.js versions earlier than 18.19.0, this should be reflected in the Node.js version badge or explicitly mentioned in the README.
Add a note about the minimum Node.js version requirement:
[](https://nodejs.org/en/download/) + +> **Note:** Requires Node.js >= 18.19.0✅ Verification successful
Review comment is accurate and the suggestion is essential
The review comment correctly identifies that Node.js 18.19.0 is the minimum supported version, as confirmed by:
package.jsonengines field:">= 18.19.0"- CI workflow testing against versions:
18.19.0,20, and22The suggested README update would help users quickly identify version requirements without checking package.json.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash # Check package.json for Node.js version requirement echo "=== Package.json Node.js version requirement ===" cat package.json | jq '.engines' # Check GitHub workflows for Node.js version configuration echo -e "\n=== GitHub workflow Node.js versions ===" fd -e yml -e yaml . .github/workflows --exec cat {} # Search for version-related checks in source code echo -e "\n=== Version checks in source code ===" rg -i "node.*version|engines|>= *1[0-9]|version.*require"Length of output: 1663
| if (!existsSync(newOptions.dir)) { | ||
| mkdirSync(newOptions.dir, { recursive: true }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for directory creation
The mkdirSync operation could fail due to permissions or other filesystem issues. Consider adding try-catch with appropriate error handling.
- if (!existsSync(newOptions.dir)) {
- mkdirSync(newOptions.dir, { recursive: true });
- }
+ if (!existsSync(newOptions.dir)) {
+ try {
+ mkdirSync(newOptions.dir, { recursive: true });
+ } catch (err) {
+ app.coreLogger.error('[@eggjs/static] failed to create directory %s: %s',
+ newOptions.dir, err.message);
+ throw err;
+ }
+ }📝 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.
| if (!existsSync(newOptions.dir)) { | |
| mkdirSync(newOptions.dir, { recursive: true }); | |
| } | |
| if (!existsSync(newOptions.dir)) { | |
| try { | |
| mkdirSync(newOptions.dir, { recursive: true }); | |
| } catch (err) { | |
| app.coreLogger.error('[@eggjs/static] failed to create directory %s: %s', | |
| newOptions.dir, err.message); | |
| throw err; | |
| } | |
| } |
[skip ci] ## [3.0.0](v2.3.1...v3.0.0) (2025-01-12) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Updated package to `@eggjs/static` - Enhanced TypeScript support - Improved static file serving configuration - **Chores** - Updated GitHub Actions workflows - Modernized project configuration - Updated Node.js version support to 18.19.0, 20, and 22 - **Documentation** - Updated README with new package details - Simplified changelog and documentation - **Refactor** - Migrated from CommonJS to ES modules - Restructured project file organization <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#26](#26)) ([ab7d6fb](ab7d6fb))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
New Features
@eggjs/staticChores
Documentation
Refactor