feat: support cjs and esm both by tshy #34
Conversation
WalkthroughThis pull request represents a comprehensive modernization and refactoring of the Egg.js development package. The changes involve transitioning from a JavaScript-based implementation to a TypeScript-based one, updating the project structure, enhancing type safety, and modernizing the development workflow. Key modifications include updating the ESLint configuration, revising GitHub Actions workflow, improving gitignore, updating package metadata, and restructuring the project's source and test files to use TypeScript and ES modules. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Agent as Agent
participant FileWatcher as File Watcher
participant Master as Master Process
App->>Agent: Initialize
Agent->>FileWatcher: Setup Watch Directories
FileWatcher-->>Agent: Monitor Changes
alt File Change Detected
FileWatcher->>Agent: Notify File Change
Agent->>Master: Request Worker Reload
Master->>App: Reload Worker
end
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 (
|
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
commit: |
[skip ci] ## [4.0.0](v3.0.2...v4.0.0) (2025-01-11) ### ⚠ 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 Based on the comprehensive changes, here are the release notes: ## Release Notes for @eggjs/development - **New Features** - Added TypeScript support for development environment - Enhanced file watching and reloading mechanisms - Improved configuration options for development mode - **Breaking Changes** - Migrated from `egg-development` to `@eggjs/development` - Requires Node.js version >=18.19.0 - Dropped support for Node.js 14 and 16 - **Improvements** - Refined ESLint configuration - Updated GitHub Actions workflow to focus on macOS testing - Added more robust file change detection - Enhanced type safety across the project - **Bug Fixes** - Improved handling of file reloading in different scenarios - Fixed potential issues with asset and service file monitoring - **Chores** - Updated dependencies - Migrated test suite to TypeScript - Restructured project configuration <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#34](#34)) ([7a63cd6](7a63cd6))
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🔭 Outside diff range comments (1)
README.md (1)
Line range hint
11-19: Update badge URLs to match new package nameThe badge URLs still reference the old package name
egg-developmentinstead of@eggjs/development.-[npm-image]: https://img.shields.io/npm/v/egg-development.svg?style=flat-square -[npm-url]: https://npmjs.org/package/egg-development +[npm-image]: https://img.shields.io/npm/v/@eggjs/development.svg?style=flat-square +[npm-url]: https://npmjs.org/package/@eggjs/development🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 2027 characters long)
Context: ...t [![NPM version][npm-image]][npm-url][![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] [
🧹 Nitpick comments (24)
test/fixtures/timing/app.js (1)
7-8: Consider using async operations for real-world scenarios.While synchronous operations are acceptable in tests, for real-world implementations consider using async alternatives to avoid blocking the event loop.
Here's how it could be implemented asynchronously:
- app.checkFile = () => { - return { - timing: fs.existsSync(path.join(__dirname, 'run/agent_timing_11111.json')), - config: fs.existsSync(path.join(__dirname, 'run/application_config.json')), - }; + app.checkFile = async () => { + const [timing, config] = await Promise.all([ + fs.promises.access(path.join(__dirname, 'run/agent_timing_11111.json')) + .then(() => true) + .catch(() => false), + fs.promises.access(path.join(__dirname, 'run/application_config.json')) + .then(() => true) + .catch(() => false), + ]); + return { timing, config }; };src/agent.ts (2)
5-5: Use standard Node.js methods instead of the customexistsfunctionYou are importing
existsfrom the'utility'package, but it might be better to use the standardfs.access()method from thefs/promisesmodule to check if the directory exists. This reduces external dependencies and utilizes built-in Node.js functionality.Apply this diff to use
fs.access():-import { exists } from 'utility'; ... -const stat = await exists(rundir); -if (!stat) return; +try { + await fs.access(rundir); +} catch { + return; +}Also applies to: 18-20
31-31: Simplify property access by removingReflect.getInstead of using
Reflect.get(agent.options, 'mode'), you can access themodeproperty directly. This simplifies the code and improves readability.Apply this diff:
-if (agent.options && Reflect.get(agent.options, 'mode') === 'single') { +if (agent.options?.mode === 'single') {src/utils.ts (2)
8-10: Consider addressing the ts-ignore technical debtThe ts-ignore comment suggests potential type-safety issues that should be addressed.
Consider using proper type assertions or fixing the underlying type issue instead of suppressing it.
17-19: Enhance file pattern matching robustnessThe current regex pattern could be more explicit and maintainable.
Consider this improvement:
-export function isTimingFile(file: string) { - return /^(agent|application)_timing/.test(file); +export function isTimingFile(file: string) { + const TIMING_FILE_PATTERN = /^(agent|application)_timing[^/]*$/; + return TIMING_FILE_PATTERN.test(file); }This change:
- Makes the pattern more explicit with a named constant
- Ensures full string match with
$- Prevents matching paths with
/using[^/]*test/not-reload.test.ts (2)
4-4: Avoid shadowing built-in globals.The
escapeimport shadows the globalescapefunction. Consider renaming it to be more specific, e.g.,escapeRegExp.-import { escape, getFilepath, DELAY } from './utils.js'; +import { escape as escapeRegExp, getFilepath, DELAY } from './utils.js'; // Update usage -app.notExpect('stdout', new RegExp(escape(`reload worker because ${filepath} change`))); +app.notExpect('stdout', new RegExp(escapeRegExp(`reload worker because ${filepath} change`)));🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
Line range hint
10-22: Consolidate duplicate afterEach hooks.There are multiple
afterEachhooks that can be consolidated into a single hook for better maintainability.- afterEach(() => app.close()); - afterEach(mm.restore); - // for debounce - afterEach(() => scheduler.wait(500)); + afterEach(async () => { + await app.close(); + mm.restore(); + // for debounce + await scheduler.wait(500); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 22-22: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/types.ts (1)
27-29: Enhance reloadPattern documentation.The documentation for
reloadPatterncould be more descriptive about the pattern format and provide examples./** - * whether to reload, use https://github.com/sindresorhus/multimatch + * Pattern(s) to determine whether to reload when files change. + * Uses https://github.com/sindresorhus/multimatch for pattern matching. + * @example ['src/**/*.ts', '!src/**/*.spec.ts'] - Watch all TypeScript files except tests + * @example '*.{js,ts}' - Watch all JavaScript and TypeScript files */src/config/config.default.ts (1)
13-22: Consider using thesatisfiesoperator for better type safety.Instead of using type assertion with
as, consider using thesatisfiesoperator for better type checking while maintaining type inference.- } as DevelopmentConfig, + } satisfies DevelopmentConfig,The
satisfiesoperator ensures the object matches the type without widening the type information, making it safer than type assertion.test/fast_ready_false.test.ts (2)
9-12: Consolidate duplicate afterEach hooks.Similar to the previous test file, there are multiple
afterEachhooks that can be consolidated.- afterEach(() => app.close()); - afterEach(mm.restore); - // for debounce - afterEach(() => scheduler.wait(500)); + afterEach(async () => { + await app.close(); + mm.restore(); + // for debounce + await scheduler.wait(500); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 10-10: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 12-12: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
20-20: Extract wait times to named constants.The different wait times (100ms and 300ms) should be extracted to named constants for better maintainability and clarity of intent.
+const LOG_WRITE_DELAY = 100; +const READY_DELAY = 300; - await scheduler.wait(100); + await scheduler.wait(LOG_WRITE_DELAY); - await scheduler.wait(300); + await scheduler.wait(READY_DELAY);Also applies to: 31-31
test/custom.test.ts (1)
17-19: Consider combining afterEach hooksThe duplicate
afterEachhooks can be combined into a single hook for better maintainability:- afterEach(mm.restore); - // for debounce - afterEach(() => scheduler.wait(500)); + afterEach(async () => { + mm.restore(); + // for debounce + await scheduler.wait(500); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/app/middleware/egg_loader_trace.ts (1)
12-14: Add error handling and caching for template loadingThe template loading could benefit from error handling and caching for better performance:
+ let templateCache: string | undefined; + return async (ctx, next) => { if (ctx.path !== '/__loader_trace__') { return await next(); } - const template = await fs.readFile(getSourceFile('config/loader_trace.html'), 'utf8'); + try { + if (!templateCache) { + templateCache = await fs.readFile(getSourceFile('config/loader_trace.html'), 'utf8'); + } + const data = await loadTimingData(app); + ctx.body = templateCache.replace('{{placeholder}}', JSON.stringify(data)); + } catch (err) { + ctx.status = 500; + ctx.body = { error: 'Failed to load timing data' }; + app.logger.error(err); + } };test/absolute.test.ts (1)
19-21: Clarify the commented debug optionThe commented debug option needs clarification. Either remove it or document why it's commented out:
- // debug: true, + // debug: false, // Enable only when debugging test failurestest/timing.test.ts (2)
24-24: Extract magic numbers into named constantsThe magic number 3000 should be extracted into a named constant for better maintainability:
+ const MIN_EXPECTED_JSON_LENGTH = 3000; - assert(jsonString[1].length > 3000); + assert(jsonString[1].length > MIN_EXPECTED_JSON_LENGTH);
35-35: Remove commented console.logRemove the commented console.log statement as it adds noise to the code:
- // console.log(last);test/process_mode_single.test.ts (2)
21-21: Consider removing the type assertion.The type assertion
as anysuggests incomplete typing. Consider defining a proper interface for the configuration object.interface AppConfig { env: string; baseDir: string; plugins: { [key: string]: { enable: boolean; path: string; }; }; }
39-39: Replace hard-coded wait time with a constant.Hard-coded wait times can make tests flaky. Consider using a shared constant from utils.
- await scheduler.wait(1000); + await scheduler.wait(DELAY);test/development.test.ts (1)
17-19: Consolidate afterEach hooks.Multiple
afterEachhooks can be confusing. Consider combining them into a single hook.- afterEach(mm.restore); - // for debounce - afterEach(() => scheduler.wait(500)); + afterEach(async () => { + mm.restore(); + // for debounce + await scheduler.wait(500); + });🧰 Tools
🪛 Biome (1.9.4)
[error] 19-19: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/development-ts.test.ts (1)
9-15: Consider using before instead of beforeEach.The test setup uses
beforeEach, while similar tests usebefore. This could impact performance as the app is recreated for each test.- beforeEach(() => { + before(() => { mm.env('local'); app = mm.cluster({ baseDir: 'development-ts', }); return app.ready(); });test/override.test.ts (3)
8-8: Use optional chaining in after hook.The current check
app && app.close()can be simplified using optional chaining.- after(() => app && app.close()); + after(() => app?.close());
29-29: Standardize RegExp usage.The file uses different styles for RegExp creation: direct literals (
/a\.js/) and constructor (new RegExp(escape(...))). Consider standardizing the approach.- app.expect('stdout', /a\.js/); + app.expect('stdout', new RegExp(escape('a.js')));Also applies to: 39-39, 58-58, 68-68
32-33: Remove redundant debug calls.The
app.debug()calls appear inconsistently before some tests. Either add them consistently or remove them if not necessary.- app.debug(); const filepath = getFilepath('override/app/no-trigger/index.js');Also applies to: 62-63
.github/workflows/nodejs.yml (1)
14-15: Consider testing on multiple platformsCurrently, the CI is limited to macOS. Consider adding Ubuntu to ensure cross-platform compatibility, especially since Node.js native ESM behavior can vary between platforms.
- os: 'macos-latest' + os: 'ubuntu-latest, macos-latest'
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (40)
.eslintrc(1 hunks).github/workflows/nodejs.yml(1 hunks).gitignore(1 hunks)README.md(4 hunks)agent.js(0 hunks)app.js(0 hunks)package.json(3 hunks)src/agent.ts(1 hunks)src/app.ts(1 hunks)src/app/middleware/egg_loader_trace.ts(1 hunks)src/config/config.default.ts(2 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)src/typings/index.d.ts(1 hunks)src/utils.ts(1 hunks)test/absolute.test.ts(1 hunks)test/custom.test.js(0 hunks)test/custom.test.ts(1 hunks)test/development-ts.test.ts(1 hunks)test/development.test.js(0 hunks)test/development.test.ts(1 hunks)test/fast_ready_false.test.ts(1 hunks)test/fixtures/development-ts/app/public/foo.js(1 hunks)test/fixtures/development-ts/app/router.ts(1 hunks)test/fixtures/development-ts/config/config.default.ts(1 hunks)test/fixtures/development-ts/package.json(1 hunks)test/fixtures/development-ts/tsconfig.json(1 hunks)test/fixtures/development/app/router.js(1 hunks)test/fixtures/development/config/config.default.js(0 hunks)test/fixtures/timing/app.js(1 hunks)test/fixtures/timing/app/router.js(1 hunks)test/not-reload.test.ts(2 hunks)test/override.test.ts(3 hunks)test/process_mode_single.test.js(0 hunks)test/process_mode_single.test.ts(1 hunks)test/timing.test.js(0 hunks)test/timing.test.ts(1 hunks)test/utils.js(0 hunks)test/utils.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- test/fixtures/development/config/config.default.js
- app.js
- test/utils.js
- test/custom.test.js
- agent.js
- test/process_mode_single.test.js
- test/timing.test.js
- test/development.test.js
✅ Files skipped from review due to trivial changes (6)
- test/fixtures/development-ts/package.json
- src/index.ts
- test/fixtures/development-ts/app/public/foo.js
- src/typings/index.d.ts
- .gitignore
- tsconfig.json
🧰 Additional context used
🪛 Biome (1.9.4)
test/utils.ts
[error] 11-11: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
test/custom.test.ts
[error] 19-19: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/override.test.ts
[error] 4-4: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 9-9: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 12-12: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/not-reload.test.ts
[error] 4-4: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 22-22: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/development.test.ts
[error] 5-5: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 19-19: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/absolute.test.ts
[error] 27-27: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/fast_ready_false.test.ts
[error] 10-10: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 12-12: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/development-ts.test.ts
[error] 5-5: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
[error] 17-17: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 19-19: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 5 exclamation marks for a text that’s 2027 characters long)
Context: ...t [![NPM version][npm-image]][npm-url] [![Test coverage][codecov-image]][codecov-url] [![Known Vulnerabilities][snyk-image]][snyk-url] [![npm download][download-image]][download-url] [
🔇 Additional comments (18)
test/fixtures/timing/app/router.js (1)
3-3: LGTM! Good improvement in the test fixture.The change to invoke
checkFile()as a function ensures that each request gets the current file system state, making the test more accurate for timing-related scenarios.test/fixtures/timing/app.js (1)
5-10: Good conversion to a function, but note about synchronous operations.Converting
checkFileto a function ensures fresh file system state on each call, which is good. However, be aware thatfs.existsSyncis being used, which is a synchronous operation.Since this is a test fixture, the synchronous operation might be acceptable, but let's verify if this pattern is used elsewhere in the codebase:
✅ Verification successful
Synchronous fs operations safely isolated to test fixtures
Verification confirms that
fs.existsSyncusage is contained within test fixtures only, with no occurrences in production code. The synchronous file system operation is acceptable in this context.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for existsSync usage in production code rg "fs\.existsSync" --type js --type ts --glob '!test/**'Length of output: 59
Script:
#!/bin/bash # First, verify our pattern works in test files rg "fs\.existsSync" --type js --type ts # Then check different fs import/require patterns rg "(?:import.*from ['|\"]fs['|\"]|require\(['|\"]fs['\"])" --type js --type ts --glob '!test/**'Length of output: 369
test/fixtures/development-ts/config/config.default.ts (2)
1-1: Ensure correct module resolution for TypeScriptImporting a JavaScript file using a relative path in a TypeScript file may cause module resolution issues, especially with ESM modules. Verify that the TypeScript compiler options (
tsconfig.json) are configured to handle this import correctly.Please check if the
compilerOptionsin yourtsconfig.jsoninclude the correct settings, such asallowJs: trueandesModuleInterop: true.
5-10: Configuration file looks goodThe configuration is properly defined with type casting to
EggAppConfig, ensuring type safety.test/fixtures/development-ts/app/router.ts (1)
1-11: Implementation aligns well with PR objectives!The router implementation correctly uses:
- ESM import syntax
- TypeScript types from egg
- Modern async/await pattern for route handlers
test/utils.ts (1)
1-9: Well-implemented path handling for dual ESM/CJS support!The implementation correctly:
- Uses node: protocol imports for better compatibility
- Handles both ESM and CJS dirname resolution
src/app.ts (1)
1-17: Excellent TypeScript implementation with modern features!The implementation demonstrates good practices:
- Proper interface implementation
- Private class fields for encapsulation
- Type-safe configuration access
test/not-reload.test.ts (1)
1-4: LGTM! Clean ES modules migration.The migration to ES modules is well-executed, using the recommended patterns:
- Node.js built-in modules use the
node:protocol- Type imports are properly separated
- Local imports use the
.jsextension🧰 Tools
🪛 Biome (1.9.4)
[error] 4-4: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
src/types.ts (1)
1-30: LGTM! Well-structured type definitions.The
DevelopmentConfiginterface is well-documented and properly typed. The optionalreloadPatternproperty allows for flexible configuration.test/custom.test.ts (1)
21-24: Reconsider skipping tests in CI environmentThe test is being skipped in CI, which could hide potential issues. Consider implementing a more robust solution that works in both CI and local environments.
test/absolute.test.ts (1)
12-13: Address the FIXME commentThe comment "FIXME: ONLY WATCH EXIST DIR" needs to be addressed:
Would you like me to help implement proper directory watching logic or create an issue to track this?
test/development-ts.test.ts (1)
54-54: Verify the expected reload count.There's an inconsistency in the expected reload count between this test (2) and the similar test in development.test.ts (4).
✅ Verification successful
The reload count difference appears to be intentional
The tests are identical in structure but operate in different environments:
development.test.tstests JavaScript files (.js) in the 'development' directorydevelopment-ts.test.tstests TypeScript files (.ts) in the 'development-ts' directoryThe different reload counts (4 vs 2) likely reflect the intended behavior for their respective environments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other reload count assertions rg -A 1 "assert\.equal\(count\(.*stdout.*reload worker" --type tsLength of output: 302
Script:
#!/bin/bash # Compare the two test files to understand context diff test/development.test.ts test/development-ts.test.ts # Search for reload-related code and setup rg -B 5 -A 5 "reload worker" test/development*.test.tsLength of output: 4829
package.json (4)
78-90: LGTM! Proper dual module format configurationThe exports configuration correctly defines both ESM and CommonJS entry points with their corresponding type definitions. The tshy configuration is properly set up for building both formats.
Also applies to: 71-77
54-61: LGTM! Comprehensive build and test scriptsThe scripts section is well organized with proper lifecycle hooks:
- Linting and cleaning before tests
- Type checking and dual format building before publish
- Security checks with attw
16-20:⚠️ Potential issueRemove duplicate exports field in eggPlugin section
The
exportsfield in theeggPluginsection appears to be misplaced. This configuration should only be at the root level where it's already correctly defined (lines 78-90)."eggPlugin": { "name": "development", "env": [ "local" ], "dependencies": [ "watcher" - ], - "exports": { - "import": "./dist/esm", - "require": "./dist/commonjs", - "typescript": "./src" - } + ] },Likely invalid or redundant comment.
29-48: Verify compatibility with updated dependenciesSeveral major version updates are included:
- @eggjs/core: ^6.2.11
- debounce: ^2.2.0
- New dev dependencies for TypeScript support
Let's verify the compatibility of these versions:
✅ Verification successful
Dependencies and TypeScript setup verified successfully
The updated dependencies are compatible and properly integrated:
- No security vulnerabilities found
- TypeScript configuration is complete and correct
- Project structure supports all dependency updates
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any known compatibility issues with the new versions gh api graphql -f query=' { securityVulnerabilities(first: 5, ecosystem: NPM, package: "debounce") { nodes { advisory { summary severity publishedAt } vulnerableVersionRange } } }' # Check if @eggjs/core ^6.2.11 is the latest compatible version npm view @eggjs/core versions --json | jq 'map(select(startswith("6.")))'Length of output: 667
Script:
#!/bin/bash # Check TypeScript configuration and egg compatibility cat package.json | jq '.scripts + .tshy' # Check if there are any TypeScript configuration files fd -e json -e jsonc tsconfig # Look for egg configuration files fd -g 'config.*' -t fLength of output: 1046
test/fixtures/development-ts/tsconfig.json (1)
1-10: LGTM! TypeScript configuration optimized for modern Node.jsThe TypeScript configuration is well-structured:
- Extends the standard @eggjs/tsconfig
- Uses modern module resolution (NodeNext)
- Enables strict type checking
- Targets ES2022 which is compatible with Node.js >= 18.19.0
.eslintrc (1)
2-5: LGTM! ESLint configuration updated for TypeScriptThe ESLint configuration properly extends:
- TypeScript-specific rules from eslint-config-egg
- Node prefix enforcement rules
| if (filepath.startsWith(ignorePath)) { | ||
| return true; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure cross-platform path compatibility
Using filepath.startsWith(ignorePath) may not work correctly on all platforms due to differences in path separators ('/' vs. '\\'). To ensure cross-platform compatibility, consider using path.relative() or path.normalize().
Update the condition to:
-if (filepath.startsWith(ignorePath)) {
+if (!path.relative(ignorePath, filepath).startsWith('..')) {This checks if filepath is within ignorePath regardless of the operating system.
Don't forget to import path if not already imported:
+import path from 'node:path';Committable suggestion skipped: line range outside the PR's diff.
| app.get('/foo.js', async function() { | ||
| this.body = 'foo.js'; | ||
| }); |
There was a problem hiding this comment.
Fix this binding in asynchronous route handlers
When using async function as route handlers, the this context is not bound as it is in generator functions. To access the context, you should use an arrow function with the ctx parameter instead of this.
Apply this diff to fix the context binding:
-app.get('/foo.js', async function() {
- this.body = 'foo.js';
+app.get('/foo.js', async (ctx) => {
+ ctx.body = 'foo.js';
});
...
-app.get('/foo', async function() {
- this.body = 'foo';
+app.get('/foo', async (ctx) => {
+ ctx.body = 'foo';
});Also applies to: 6-8
| export function escape(str: string) { | ||
| return str | ||
| .replace(/[|\\{}()[\]^$+*?.]/g, '\\$&') | ||
| .replace(/-/g, '\\x2d'); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename the escape function to avoid shadowing global property
The function name shadows the global escape property which could lead to confusion.
Consider renaming to something more specific like escapeRegExp:
-export function escape(str: string) {
+export function escapeRegExp(str: string) {
return str
.replace(/[|\\{}()[\]^$+*?.]/g, '\\$&')
.replace(/-/g, '\\x2d');
}📝 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.
| export function escape(str: string) { | |
| return str | |
| .replace(/[|\\{}()[\]^$+*?.]/g, '\\$&') | |
| .replace(/-/g, '\\x2d'); | |
| } | |
| export function escapeRegExp(str: string) { | |
| return str | |
| .replace(/[|\\{}()[\]^$+*?.]/g, '\\$&') | |
| .replace(/-/g, '\\x2d'); | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 11-11: Do not shadow the global "escape" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
| let filepath; | ||
| filepath = getFilepath('custom/app/service/a.js'); | ||
| await fs.writeFile(filepath, ''); | ||
| await scheduler.wait(DELAY); | ||
|
|
||
| await fs.unlink(filepath); | ||
| app.expect('stdout', /a\.js/); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add cleanup for created files
File operations should be cleaned up after each test to prevent test pollution:
+ afterEach(async () => {
+ const filepath = getFilepath('custom/app/service/a.js');
+ await fs.rm(filepath, { force: true });
+ const filepathTs = getFilepath('custom/app/service/b.ts');
+ await fs.rm(filepathTs, { force: true });
+ });Committable suggestion skipped: line range outside the PR's diff.
| const rundir = app.config.rundir; | ||
| const files = await fs.readdir(rundir); | ||
| const data = []; | ||
| const data: any[] = []; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper types instead of using 'any'
Consider creating an interface for the timing data structure instead of using any[]:
- const data: any[] = [];
+ interface TimingData {
+ type: string;
+ pid: string;
+ start: number;
+ end: number;
+ index: number;
+ range: [number, number];
+ title: string;
+ }
+ const data: TimingData[] = [];Committable suggestion skipped: line range outside the PR's diff.
| const jsonString = res.text.match(/data = (.*?);/); | ||
| assert(jsonString); | ||
| assert(jsonString[1].length > 3000); | ||
| const json = JSON.parse(jsonString[1]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve JSON extraction robustness
The current regex-based JSON extraction could be more robust:
- const jsonString = res.text.match(/data = (.*?);/);
- assert(jsonString);
- assert(jsonString[1].length > 3000);
- const json = JSON.parse(jsonString[1]);
+ const match = res.text.match(/data = ({.*?});/s);
+ assert(match, 'Failed to extract JSON data from response');
+ const jsonString = match[1];
+ assert(jsonString.length > 3000, 'JSON data is unexpectedly small');
+ try {
+ const json = JSON.parse(jsonString);
+ assert(Array.isArray(json), 'Parsed data is not an array');📝 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.
| const jsonString = res.text.match(/data = (.*?);/); | |
| assert(jsonString); | |
| assert(jsonString[1].length > 3000); | |
| const json = JSON.parse(jsonString[1]); | |
| const match = res.text.match(/data = ({.*?});/s); | |
| assert(match, 'Failed to extract JSON data from response'); | |
| const jsonString = match[1]; | |
| assert(jsonString.length > 3000, 'JSON data is unexpectedly small'); | |
| try { | |
| const json = JSON.parse(jsonString); | |
| assert(Array.isArray(json), 'Parsed data is not an array'); |
| function count(str: string, match: string) { | ||
| const m = str.match(new RegExp(match, 'g')); | ||
| return m ? m.length : 0; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Move count utility function to utils.js.
The count function is duplicated across test files. Consider moving it to the shared utils file.
// In utils.js
+ export function count(str: string, match: string) {
+ const m = str.match(new RegExp(match, 'g'));
+ return m ? m.length : 0;
+ }
// In this file
- function count(str: string, match: string) {
- const m = str.match(new RegExp(match, 'g'));
- return m ? m.length : 0;
- }Committable suggestion skipped: line range outside the PR's diff.
| ## Requirements | ||
|
|
||
| - egg >= 4.x |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add dual module format support to requirements
The requirements section should mention the dual module format support (ESM/CommonJS).
## Requirements
- egg >= 4.x
+- Supports both ESM and CommonJS module formats
+- Node.js >= 18.19.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.
| ## Requirements | |
| - egg >= 4.x | |
| ## Requirements | |
| - egg >= 4.x | |
| - Supports both ESM and CommonJS module formats | |
| - Node.js >= 18.19.0 |
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Based on the comprehensive changes, here are the release notes:
Release Notes for @eggjs/development
New Features
Breaking Changes
egg-developmentto@eggjs/developmentImprovements
Bug Fixes
Chores