feat: support cjs and esm both by tshy #63
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 55 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request introduces a comprehensive modernization and refactoring of the Changes
Poem
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 (
|
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: 3
🧹 Nitpick comments (14)
test/utils.ts (1)
Line range hint
4-38: Consider configurable wait durations and parallelized process kills.
- The current code imposes a fixed 5-second delay (
scheduler.wait(5000)). If you anticipate shorter or adjustable wait times across different test scenarios, consider making the duration configurable or environment-specific.- Process termination occurs sequentially in a for-loop. For potential speed enhancements, you could parallelize
process.killcalls (e.g., by creating an array of promises).src/helper.ts (2)
24-25: Refine process filtering logic for cross-platform robustness
Currently, the code checks!line.includes('/bin/sh') && line.includes('node')to filter processes. This heuristic can potentially miss or incorrectly include processes if "node" appears in unexpected parts of the command string (e.g., custom-nodes, interpreters named differently, etc.). Consider making this check more robust or configurable.
50-62: Leverageimport.meta.urlmore consistently
The code uses an inline TypeScript ignore directive forfileURLToPath(import.meta.url), which might be safe but slightly messy. Consider updating the rest of the module to consistently rely on ESM features (like top-level__dirnameemulation) or remove the inline ignore comment if no longer needed.src/commands/start.ts (2)
273-273: Avoid assignment in the same expression
Assigning tothis.#childand a local variable in one statement reduces clarity:const child = this.#child = spawn(command, eggArgs, options);Separate them to make each assignment explicit:
- const child = this.#child = spawn(command, eggArgs, options); + const child = spawn(command, eggArgs, options); + this.#child = child;🧰 Tools
🪛 Biome (1.9.4)
[error] 273-273: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
289-289: Avoid assignment in the same expression
As above, avoid chaining the assignments for readability:- const child = this.#child = spawn(command, eggArgs, options); + const child = spawn(command, eggArgs, options); + this.#child = child;🧰 Tools
🪛 Biome (1.9.4)
[error] 289-289: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
test/start.test.ts (2)
519-522: Consolidate duplicate test setup/teardown hooks
MultiplebeforeEachcalls in the same scope can lead to confusion or unintended interactions. Combine them or clarify their scopes to avoid thenoDuplicateTestHookslint warning.beforeEach(async () => { await cleanup(fixturePath); app && app.proc && app.proc.kill('SIGTERM'); await cleanup(fixturePath); });🧰 Tools
🪛 Biome (1.9.4)
[error] 519-522: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 520-520: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
520-520: Optional chain for safer property access
Instead ofapp && app.proc && app.proc.kill('SIGTERM');, consider an optional chain to simplify the logic, e.g.app?.proc?.kill('SIGTERM');.🧰 Tools
🪛 Biome (1.9.4)
[error] 520-520: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/index.ts (1)
3-3: Remove or uncomment unused export
The commented-out line// exports.StopCommand = ...suggests a deprecated or placeholder export. Consider removing it if no longer relevant, or uncomment and implement if needed.test/fixtures/pkg-config-esm/config/plugin.js (3)
27-37: Confirm i18n plugin usage.
If you plan to enable i18n in the future, remember to add relevant locale data or directories.
39-49: Watcher plugin might be environment-specific.
Ensure that the watcher plugin is toggled correctly for local development vs. production to avoid unnecessary overhead.
123-133: JSONP plugin is rarely needed today.
Many modern APIs avoid JSONP in favor of more secure approaches (CORS, etc.). If you really need it, keep it; otherwise, consider removing it..github/workflows/pkg.pr.new.yml (1)
1-23: New workflow for publishing from any commit.
This workflow looks correct for continuous publishing. Confirm that you have the necessary credentials in place and that publishing every commit follows your release process.README.md (2)
104-106: Consider documenting the WASI featureWhile the
--allow-wasioption is added, there's no explanation of its purpose or use cases.Consider adding a brief explanation of when and why users might need the WASI option, for example:
// will pass as `node --allow-wasi` - "node-options--allow-wasi": true + "node-options--allow-wasi": true // Enable WASI support for WebAssembly System Interface features
24-24: Consider updating command examplesThe command examples could be more comprehensive to showcase the new features.
Consider adding examples that demonstrate:
- TypeScript/sourcemap usage
- New node options (like WASI)
- Common production configurations
Also applies to: 55-55, 85-85
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
test/fixtures/example/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**test/fixtures/example/node_modules/yadan/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config-esm/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config-esm/node_modules/custom-framework/package.jsonis excluded by!**/node_modules/**test/fixtures/pkg-config-esm/node_modules/inject/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config-esm/node_modules/inject/package.jsonis excluded by!**/node_modules/**test/fixtures/pkg-config-sourcemap/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config-sourcemap/node_modules/inject/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**test/fixtures/pkg-config/node_modules/inject/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (36)
.eslintrc(1 hunks).github/PULL_REQUEST_TEMPLATE.md(0 hunks).github/workflows/nodejs.yml(1 hunks).github/workflows/pkg.pr.new.yml(1 hunks).gitignore(1 hunks)README.md(6 hunks)bin/dev.cmd(1 hunks)bin/dev.js(1 hunks)bin/egg-scripts.js(0 hunks)bin/run.cmd(1 hunks)bin/run.js(1 hunks)index.js(0 hunks)lib/cmd/start.js(0 hunks)lib/helper.js(0 hunks)lib/start-cluster(0 hunks)package.json(1 hunks)scripts/start-cluster.cjs(1 hunks)scripts/start-cluster.mjs(1 hunks)src/baseCommand.ts(1 hunks)src/commands/start.ts(1 hunks)src/helper.ts(1 hunks)src/index.ts(1 hunks)src/types.ts(1 hunks)test/fixtures/example/app.js(0 hunks)test/fixtures/example/app/router.js(1 hunks)test/fixtures/pkg-config-esm/config/config.default.js(1 hunks)test/fixtures/pkg-config-esm/config/plugin.js(1 hunks)test/fixtures/pkg-config-esm/inject1.js(1 hunks)test/fixtures/pkg-config-esm/inject2.js(1 hunks)test/fixtures/pkg-config-esm/package.json(1 hunks)test/fixtures/ts-pkg/app/controller/home.ts(1 hunks)test/fixtures/ts/app/controller/home.ts(1 hunks)test/start.test.ts(10 hunks)test/stop.test.js(0 hunks)test/utils.ts(2 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (8)
- bin/egg-scripts.js
- .github/PULL_REQUEST_TEMPLATE.md
- index.js
- test/stop.test.js
- lib/start-cluster
- lib/cmd/start.js
- lib/helper.js
- test/fixtures/example/app.js
✅ Files skipped from review due to trivial changes (11)
- test/fixtures/pkg-config-esm/inject2.js
- test/fixtures/pkg-config-esm/inject1.js
- bin/run.cmd
- .gitignore
- bin/run.js
- test/fixtures/ts-pkg/app/controller/home.ts
- tsconfig.json
- bin/dev.cmd
- test/fixtures/ts/app/controller/home.ts
- test/fixtures/pkg-config-esm/package.json
- test/fixtures/pkg-config-esm/config/config.default.js
🧰 Additional context used
🪛 LanguageTool
README.md
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2341 characters long)
Context: ...s [![NPM version][npm-image]][npm-url] 
[![Test coverage][codecov-image]][codecov-url] [![npm download][download-image]][download-url] [
🪛 Biome (1.9.4)
src/baseCommand.ts
[error] 9-9: Shouldn't redeclare 'Flags'. Consider to delete it or rename it.
'Flags' is defined here:
(lint/suspicious/noRedeclare)
src/commands/start.ts
[error] 273-273: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 289-289: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
test/start.test.ts
[error] 519-522: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 520-520: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
bin/dev.js (1)
1-1: Ensure compatibility with Node.js 18.19.0+
The shebang line uses modern Node.js features (--loader ts-node/esm), which might not work in earlier Node.js versions. This aligns with the PR objective of dropping Node.js support below 18.19.0. Just be sure to confirm that the project documentation clearly states this requirement.
test/utils.ts (2)
1-2: Imports appear valid and aligned with Node.js 18+ requirements.
Utilizing node:timers/promises is permissible since the project now requires Node.js 18.19.0 or higher. The import looks good.
40-46: Function design looks compact and reasonable.
The replaceWeakRefMessage function addresses a specific console message gracefully. No issues noted here.
src/commands/start.ts (1)
308-358: Verify potential race conditions with isReady
checkStatus waits for this.isReady to become true, which is setin a child process message event handler. Verify that no race condition or missed event can occur if the child quickly sends a message before this code is fully ready. If needed, consider a more robust approach to manage readiness—e.g., using a promise-based handshake or synchronous signals.
src/types.ts (1)
1-9: Well-defined interface structure for PackageEgg.
This interface clearly captures the properties relevant to package configuration, including optional booleans (framework, typescript, declarations), a tscompiler string, and the possibility of revert, require, and import being either a string or an array of strings. This design is flexible and straightforward.
test/fixtures/example/app/router.js (1)
2-2: Good modernization of route handlers to async functions.
Switching from generator functions to async provides better native support for asynchronous operations, without needing co or other libraries. Please ensure that any downstream code or tests expecting generator functions are properly updated to accommodate async.
Also applies to: 6-6, 10-10
scripts/start-cluster.mjs (1)
1-14: Impressive approach for ESM-based cluster start logic.
You’re leveraging dynamic imports (importModule) and the debug API effectively. The code looks maintainable, and the approach is consistent with modern JavaScript/ESM usage. Proceeding with Node.js 18.19.0+ ensures that top-level awaits and experimental modules are no longer a concern. Looks good!
scripts/start-cluster.cjs (1)
1-15: Thoughtful CommonJS variant for backward compatibility.
It’s beneficial to provide a CJS script to preserve coverage for environments or tools that still rely on CommonJS loading. The logic mirrors the ESM variant closely, so it’s straightforward to maintain. This setup should provide a smooth experience for users in different module ecosystems.
test/fixtures/pkg-config-esm/config/plugin.js (9)
1-3: Export default object is straightforward and well-structured.
This top-level export provides a clear object definition that organizes plugin configurations.
4-13: Validate default enable behavior and plugin path usage.
Each plugin defaults to enable: false. Ensure that the application or tests handle the scenario where these plugins are disabled by default. Also, consider whether the placeholders 'xxxxx' in path are needed or can be removed if not used.
15-25: Maintain consistency with official plugin configuration.
Ensure that 'egg-session' is valid and that the default path 'xxxxx' is correct or intentionally a placeholder.
51-61: Multipart plugin placeholders.
Check if 'xxxxx' is a placeholder for an actual plugin path or if the package is fully sufficient to load it.
75-85: Development plugin usage.
Since you’re disabling the development plugin, remember to enable it only in non-production environments if that’s your standard practice.
87-97: Rotating logs is beneficial.
The default logrotator plugin is also disabled. Confirm if you have an alternative log rotation mechanism in place to avoid unbounded log growth.
99-109: Scheduling tasks.
Disabling the schedule plugin is fine if you’re not using scheduled tasks. If needed, ensure you have the schedule logic in place and enable it later.
111-121: Static server plugin.
Even though it’s disabled, check if you rely on static assets. If you plan to serve them via a CDN or a different approach, that’s fine; otherwise, be sure to enable and configure it properly.
135-145: View plugin usage.
If you plan to render views on the server side, remember to enable and configure it. Otherwise, you might remove it if it’s unnecessary.
.eslintrc (1)
2-5: Extending multiple configs ensures granular linting.
It’s a good approach to separate TypeScript rules and Node prefix rules. Confirm that these rulesets don’t conflict with each other and that they match the standards of your codebase.
.github/workflows/nodejs.yml (1)
14-16: Including Node 18.19.0 is consistent with dropped support for earlier versions.
Ensure the documentation and package metadata reflect this minimum version requirement. This is a breaking change, so be sure to communicate it clearly in release notes.
package.json (5)
41-49: Build and test scripts properly configured
The scripts section is well-organized with proper TypeScript support:
- Pre/post hooks for test and CI
- Lint with TypeScript support
- Clean script for build artifacts
- Type checking with attw
61-70: CLI configuration properly migrated to Oclif
The Oclif configuration is correctly set up with:
- Proper command directory structure
- Binary aliases for backward compatibility
- Correct dirname configuration
71-99: Dual CJS/ESM package configuration looks good
The configuration properly supports both CommonJS and ES Modules with:
- Correct exports field configuration
- Proper TypeScript output paths
- Module type declaration
Let's verify the dual package hazard patterns:
✅ Verification successful
Dual package hazard patterns are correctly handled
The codebase consistently uses .js extensions in import statements, which is the correct approach for dual CJS/ESM packages. This is important because:
- Node.js requires explicit
.jsextensions in ESM - The TypeScript compiler will properly handle these extensions for both CJS and ESM output
The verification shows:
- No usage of
require()with.jsextensions that could cause issues - All imports consistently use
.jsextensions in the source files - Import paths align with the package.json exports configuration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for dual package hazard patterns
# Check if any .js files use require() with .js extension
rg "require\(['\"].*\.js['\"]"
# Check if any import statements use .js extension
rg "from ['\"].*\.js['\"]"
Length of output: 613
8-35: Dependencies updated for TypeScript and Oclif migration
The dependency changes align with the migration to TypeScript and Oclif. However, we should verify the compatibility of the new dependencies.
Let's check for any potential issues with the new dependencies:
✅ Verification successful
Let me try a different approach to verify the dependencies.
Dependencies are properly integrated and compatible
Based on the verification results:
- The new dependencies
@eggjs/utilsand@oclif/coreare correctly imported and used in the codebase - TypeScript configuration extends
@eggjs/tsconfigand has appropriate settings for modern Node.js - The imports and usage patterns align with the expected migration to TypeScript and Oclif:
@oclif/coreis used for command-line interface features (Args, Flags, Command)@eggjs/utilsprovides framework-specific utilities (getFrameworkPath, importResolve)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify new dependencies
# Check for any security advisories
for pkg in "@eggjs/utils" "@oclif/core"; do
echo "Checking advisories for $pkg..."
npm audit $pkg
done
# Verify TypeScript-related packages compatibility
echo "Checking TypeScript packages compatibility..."
npm info typescript@5 peerDependencies
npm info @eggjs/tsconfig@1 peerDependencies
Length of output: 1194
Script:
#!/bin/bash
# Check if there's a package-lock.json file
if [ -f "package-lock.json" ]; then
echo "package-lock.json exists"
else
echo "package-lock.json not found"
fi
# Check the actual versions being used in the codebase for new dependencies
echo -e "\nChecking imports of new dependencies..."
rg --type ts --type js "@eggjs/utils|@oclif/core" -B 2 -A 2
# Look for migration guides or documentation
echo -e "\nChecking for migration documentation..."
fd -e md -e txt . -X grep -l -i "migration\|upgrade"
# Check TypeScript configuration
echo -e "\nChecking TypeScript configuration..."
if [ -f "tsconfig.json" ]; then
cat tsconfig.json
fi
Length of output: 1840
2-5: Package configuration changes look good
The package rename and Node.js version requirement align with the PR objectives. The public access configuration is correctly set for the scoped package.
Let's verify the Node.js version compatibility with dependencies:
Also applies to: 38-40
✅ Verification successful
Node.js version compatibility check completed
The Node.js version requirement of >=18.19.0 in the package is compatible with all dependencies:
- Most dependencies require Node.js 16.0.0 or higher (node-homedir, runscript, utility)
- @oclif/core requires Node.js 18.0.0 or higher
- Some older dependencies have lower requirements (common-bin: >=14.0.0, mz-modules: >=6.0.0)
- @eggjs/utils and source-map-support don't specify Node.js version requirements
Since no dependency requires a Node.js version higher than 18.19.0, and the most strict requirement from @oclif/core (>=18.0.0) is satisfied, the version compatibility is verified.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check Node.js compatibility of key dependencies
# Check package.json files of direct dependencies for their Node.js version requirements
for pkg in "@eggjs/utils" "@oclif/core" "common-bin" "mz" "mz-modules" "node-homedir" "runscript" "source-map-support" "utility"; do
echo "Checking $pkg..."
npm info $pkg engines.node
done
Length of output: 2046
README.md (1)
1-15: Documentation updates look good
The badges and package references are correctly updated to reflect the new package name.
🧰 Tools
🪛 LanguageTool
[style] ~3-~3: Using many exclamation marks might seem excessive (in this case: 4 exclamation marks for a text that’s 2341 characters long)
Context: ...s [![NPM version][npm-image]][npm-url] [![Test coverage][codecov-image]][codecov-url] [![npm download][download-image]][download-url] [
| import { execute } from '@oclif/core'; | ||
|
|
||
| await execute({ development: true, dir: import.meta.url }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for the top-level await.
Currently, if an error occurs inside await execute(...), the process might terminate with an unhandled promise rejection, potentially obscuring error details. Consider wrapping the call in a try...catch block to handle errors gracefully or log them for troubleshooting:
try {
await execute({ development: true, dir: import.meta.url });
+} catch (err) {
+ console.error('Failed to execute dev script:', err);
+ process.exit(1);
}📝 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.
| import { execute } from '@oclif/core'; | |
| await execute({ development: true, dir: import.meta.url }); | |
| import { execute } from '@oclif/core'; | |
| try { | |
| await execute({ development: true, dir: import.meta.url }); | |
| } catch (err) { | |
| console.error('Failed to execute dev script:', err); | |
| process.exit(1); | |
| } |
| type Flags<T extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & T['flags']>; | ||
| type Args<T extends typeof Command> = Interfaces.InferredArgs<T['args']>; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Rename or remove the redeclared Flags type
You are redefining type Flags<T extends typeof Command> while also importing Flags from @oclif/core. This creates confusion and triggers static analysis warnings. Rename it to something like CommandFlags<T> or remove it if unneeded.
- type Flags<T extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & T['flags']>;
- type Args<T extends typeof Command> = Interfaces.InferredArgs<T['args']>;
+ type CommandFlags<U extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & U['flags']>;
+ type CommandArgs<U extends typeof Command> = Interfaces.InferredArgs<U['args']>;📝 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.
| type Flags<T extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & T['flags']>; | |
| type Args<T extends typeof Command> = Interfaces.InferredArgs<T['args']>; | |
| type CommandFlags<U extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & U['flags']>; | |
| type CommandArgs<U extends typeof Command> = Interfaces.InferredArgs<U['args']>; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 9-9: Shouldn't redeclare 'Flags'. Consider to delete it or rename it.
'Flags' is defined here:
(lint/suspicious/noRedeclare)
| /** | ||
| * security middlewares and extends | ||
| * @member {Object} Plugin#security | ||
| * @property {Boolean} enable - `true` by default | ||
| * @since 1.0.0 | ||
| */ | ||
| security: { | ||
| enable: false, | ||
| package: 'egg-security', | ||
| path: 'xxxxx', | ||
| }, |
There was a problem hiding this comment.
Security plugin is disabled by default.
Review if disabling security by default is intentional. It can invite potential vulnerabilities.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (10)
test/fixtures/cluster-config/app/router.js (1)
2-4: Maintain consistent style across route handlers.
Changing the generator function to an async function aligns with modern JavaScript practices. Keep the usage ofasync/awaitconsistent in all route handlers throughout the codebase to streamline readability and future maintenance.src/commands/start.ts (4)
22-85: Consider splitting the large class into smaller, focused modules or methods.
While the singleStartclass is logically cohesive, it is fairly extensive. Extracting certain functionalities into separate files or utility modules (e.g., a dedicated environment configuration module, a logging module, etc.) can enhance clarity, maintainability, and testability.
274-274: Avoid assignment to class property within the declaration of a local variable.
This might be confusing and can be flagged by linters. Instead, assign to your class property on a separate line to improve clarity.Apply this diff to separate assignments:
- const child = this.#child = spawn(command, eggArgs, options); + const child = spawn(command, eggArgs, options); + this.#child = child;🧰 Tools
🪛 Biome (1.9.4)
[error] 274-274: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
290-290: Avoid assignment to class property within the declaration of a local variable.
Same reasoning as in line 274—consider performing the assignments on two separate lines.- const child = this.#child = spawn(command, eggArgs, options); + const child = spawn(command, eggArgs, options); + this.#child = child;🧰 Tools
🪛 Biome (1.9.4)
[error] 290-290: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
362-384: Add basic test coverage for these utility functions.
stringifyandgetRotateLogcontain file manipulation and JSON logic that should be tested to guard against path or file permission errors.Do you want me to open a new issue or provide sample tests illustrating how to test these helpers?
test/fixtures/egg-scripts-config/app.js (1)
1-1: Consider using ES module imports.
Although CommonJSrequirestill works, the codebase appears to be migrating to modern ES module syntax. You can replace this require statement with an import statement to align with the rest of the ES module changes in the project.-const { scheduler } = require('node:timers/promises'); +import { scheduler } from 'node:timers/promises';🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/status/app.js (1)
1-1: Switch to ES module syntax where possible.
You may consider using the import statement rather thanrequireto ensure consistency, especially if the rest of the codebase is being modernized to use ES modules.-const { scheduler } = require('node:timers/promises'); +import { scheduler } from 'node:timers/promises';🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/trace-warnings/app.js (1)
1-1: Refactor to ES import or confirm usage is allowed.
The linter flags theeventsmodule as restricted and also warns aboutrequireusage. If the codebase supports ES modules, consider importing from'node:events', or confirm that the local ESLint rule is intentional.-const EventEmitter = require('events'); +import { EventEmitter } from 'node:events';🧰 Tools
🪛 eslint
[error] 1-1: 'events' module is restricted from being used.
(no-restricted-modules)
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/start.test.ts (2)
521-524: Avoid duplicate test hooks and consider optional chaining.
You have multiplebeforeEachhooks in the samedescribe, and inside them you are conditionally killing the child process. This triggers a linter warning about duplicate hooks and also references a pattern that might be simplified using optional chaining. For example:beforeEach(async () => { - if (app && app.proc) { - app.proc.kill('SIGTERM'); - } + app?.proc?.kill('SIGTERM'); await cleanup(fixturePath); });🧰 Tools
🪛 Biome (1.9.4)
[error] 521-524: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 522-522: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
500-510: Potential security check for malicious arguments in--stderr.
Here, the test verifies that; touch ${malicious}does not execute. This is good. In a real environment, consider sanitizing user inputs for potentially malicious strings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/fixtures/egg-scripts-config/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**test/fixtures/status/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (11)
src/commands/start.ts(1 hunks)test/fixtures/cluster-config/app/router.js(1 hunks)test/fixtures/cluster-config/config/config.default.js(0 hunks)test/fixtures/cluster-config/config/config.prod.js(0 hunks)test/fixtures/custom-node-dir/app/router.js(1 hunks)test/fixtures/custom-node-dir/config/config.default.js(0 hunks)test/fixtures/egg-scripts-config/app.js(1 hunks)test/fixtures/status/app.js(1 hunks)test/fixtures/status/config/config.default.js(0 hunks)test/fixtures/trace-warnings/app.js(1 hunks)test/start.test.ts(8 hunks)
💤 Files with no reviewable changes (4)
- test/fixtures/custom-node-dir/config/config.default.js
- test/fixtures/status/config/config.default.js
- test/fixtures/cluster-config/config/config.default.js
- test/fixtures/cluster-config/config/config.prod.js
🧰 Additional context used
🪛 eslint
test/fixtures/egg-scripts-config/app.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/trace-warnings/app.js
[error] 1-1: 'events' module is restricted from being used.
(no-restricted-modules)
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/status/app.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🪛 Biome (1.9.4)
src/commands/start.ts
[error] 274-274: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 290-290: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
test/start.test.ts
[error] 521-524: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 522-522: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (8)
test/fixtures/custom-node-dir/app/router.js (1)
2-4: Validate the appropriate usage of async.
Although you've converted the route handler to an async function, there's no current usage of await. This is acceptable, but remember that if the logic inside this route expands to include asynchronous operations, the async keyword will be essential to handle them properly.
src/commands/start.ts (1)
309-359: Validate edge cases for the startup check.
If the server never sends a ready signal but also never logs an error to stderr, the while loop could be misleading. Ensure that long startup times or partial failures do not lead to a hidden deadlock.
test/fixtures/egg-scripts-config/app.js (1)
8-9: Validate the numeric conversion of process.env.WAIT_TIME.
Currently, parseInt(process.env.WAIT_TIME) can result in NaN if the environment variable is missing or not numeric. To avoid unexpected behavior, ensure that the result is validated or that a default is provided.
test/fixtures/status/app.js (1)
8-9: Ensure the wait time is a valid number.
Calling parseInt on an undefined or invalid process.env.WAIT_TIME can return NaN, potentially causing unexpected wait behavior. Consider providing a fallback or validating the input.
test/fixtures/trace-warnings/app.js (2)
4-4: Approved:
A simple console.log('app loaded'); statement can be helpful. Keep it if needed for debugging and clarity.
5-5: Approved:
Using EventEmitter with event.setMaxListeners(1) is fine here and aligns with the test's purpose of verifying MaxListenersExceededWarning behavior.
test/start.test.ts (2)
739-747: Verify exit handling for child processes.
Using once(app.proc, 'exit') is a clean approach to wait for process termination. Ensure you handle edge cases—for instance, if the process fails to exit in a timely manner.
474-486: Validate daemon logs.
Your assertions check the master stdout and stderr logs. This is a reliable way to confirm that the application started successfully. Just ensure that rotating logs do not risk overwriting crucial debugging data.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
test/stop.test.ts (1)
78-79: Consider verifying directory cleanup success.After removing the
logDirdirectory, you might also want to assert or verify that it has been successfully removed to ensure a clean test environment.src/commands/stop.ts (2)
63-66: Consider SIGKILL fallback.If some processes remain stuck after the timeout, a fallback to a stronger signal (e.g., SIGKILL) might be beneficial for ensuring cleanup in stubborn cases. You could optionally add a message indicating a fallback signal if the worker process remains alive.
70-88: Potential consolidation of master- and worker-kill logic.You're running separate kills for master and worker processes. Consider merging them if the kill logic remains identical. A single pass might simplify code and reduce complexity.
test/utils.ts (2)
32-32: Graceful shutdown suggestion.Currently, the non-master processes are forced to shut down using
SIGKILL. If time permits, consider a graceful approach withSIGTERMfollowed by a fallback toSIGKILLto allow them to clean up resources.
44-50: Confirm necessity of WeakRef message replacement.The function strips out WeakRef and FinalizationRegistry messages from stderr. Confirm if you want to keep ignoring these warnings or if you prefer to track them for potential issues.
test/start.test.ts (1)
519-519: Use optional chaining.Replace the chain of
app && app.proc && app.proc.kill('SIGTERM');with the optional chaining operator for better readability and safety.- app && app.proc && app.proc.kill('SIGTERM'); + app?.proc?.kill('SIGTERM');🧰 Tools
🪛 Biome (1.9.4)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/commands/stop.ts(1 hunks)test/fixtures/stop-timeout/app/router.js(1 hunks)test/start.test.ts(8 hunks)test/stop.test.ts(4 hunks)test/utils.ts(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/start.test.ts
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
test/fixtures/stop-timeout/app/router.js (3)
2-4: Good adoption of async/await.
Switching from generator functions to async functions is a modern approach and aligns well with Node.js versions 8 and above (and certainly with 18+). This change improves readability and maintainability, so it looks good.
6-8: Consistent async usage across routes.
Updating these routes to async is consistent with the rest of the file. This ensures uniformity and avoids mixing generator functions and async/await in the same codebase.
10-12: Retain clarity over this context in async functions.
The use of this.body in an async function is valid if referencing the Koa context. However, confirm that no new lint or style checks flag the usage of this in async code. If you decide to switch to ctx => {}, ensure the rest of the code is compatible with how Koa binds context.
test/stop.test.ts (1)
30-30: No critical issues spotted.
The tests in this file accurately validate multiple scenarios for stopping the Egg.js application without daemon mode. Everything looks structured and clear, with beforeEach and afterEach hooks properly cleaning up. Good work!
| let app: Coffee; | ||
|
|
||
| beforeEach(async () => { | ||
| await utils.cleanup(fixturePath); | ||
| await cleanup(fixturePath); | ||
| }); | ||
|
|
||
| beforeEach(async () => { | ||
| app && app.proc && app.proc.kill('SIGTERM'); | ||
| await utils.cleanup(fixturePath); | ||
| await cleanup(fixturePath); | ||
| }); |
There was a problem hiding this comment.
Avoid duplicate setup/teardown hooks.
You have two consecutive beforeEach hooks, which can lead to confusion or undesired side effects (lines 512-515 and 515-520). Consider merging these hooks or combining their logic into a single hook.
- beforeEach(async () => {
- await cleanup(fixturePath);
- });
- beforeEach(async () => {
- app && app.proc && app.proc.kill('SIGTERM');
- await cleanup(fixturePath);
- });
+ beforeEach(async () => {
+ app?.proc?.kill('SIGTERM');
+ await cleanup(fixturePath);
+ });📝 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.
| let app: Coffee; | |
| beforeEach(async () => { | |
| await utils.cleanup(fixturePath); | |
| await cleanup(fixturePath); | |
| }); | |
| beforeEach(async () => { | |
| app && app.proc && app.proc.kill('SIGTERM'); | |
| await utils.cleanup(fixturePath); | |
| await cleanup(fixturePath); | |
| }); | |
| let app: Coffee; | |
| beforeEach(async () => { | |
| app?.proc?.kill('SIGTERM'); | |
| await cleanup(fixturePath); | |
| }); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (12)
test/stop.test.ts (6)
15-21: Consider increasing the wait time for better test reliabilityThe
waitTimeof 3000ms might be too short for slower systems or CI environments. Consider:
- Making it configurable through environment variables
- Increasing the default value to at least 5000ms
- Adding retry logic for the HTTP health checks instead of fixed waits
- const waitTime = 3000; + const waitTime = process.env.TEST_WAIT_TIME ? parseInt(process.env.TEST_WAIT_TIME, 10) : 5000;
22-30: Enhance cleanup reliabilityThe cleanup logic could be more robust to prevent test interference:
after(async () => { + try { + // Ensure all test processes are terminated + await new Promise(resolve => setTimeout(resolve, 1000)); + const kill = process.platform === 'win32' ? 'taskkill /F /T /IM node.exe' : 'pkill -f egg-scripts'; + require('child_process').execSync(kill, { stdio: 'ignore' }); + } catch (err) { + // Ignore errors if no processes found + } await fs.rm(homePath, { force: true, recursive: true }); });
41-46: Improve assertion precisionThe string matching assertions could be more precise:
- assert(app.stdout.match(/custom-framework started on http:\/\/127\.0\.0\.1:7001/)); + assert(app.stdout.match(/custom-framework started on http:\/\/127\.0\.0\.1:7001\s*$/)); + + // Add timeout to HTTP request - const result = await request('http://127.0.0.1:7001'); + const result = await request('http://127.0.0.1:7001', { timeout: 5000 });
64-67: Centralize Windows-specific test logicThe Windows compatibility checks are scattered throughout the test file. Consider:
- Creating a helper function for platform-specific assertions
- Documenting why certain checks are skipped on Windows
// Add to test/utils.ts export function assertProcessSignals(output: string) { if (isWindows) { // Document why these assertions are skipped on Windows return; } assert.match(output, /\[master] master is killed by signal SIGTERM, closing/); assert.match(output, /\[master] exit with code:0/); assert.match(output, /\[app_worker] exit with code:0/); } // Use in tests assertProcessSignals(app.stdout);Also applies to: 104-107, 187-191, 242-246, 297-301, 319-323
Line range hint
332-374: Enhance symlink test robustnessThe symlink test could be improved with better error handling and more specific checks:
beforeEach(async function() { + const isRoot = process.getuid && process.getuid() === 0; + if (isWindows && !isRoot) { + console.log('Symlink tests require admin privileges on Windows'); + this.skip(); + } + try { await fs.symlink(fixturePath, baseDir, 'dir'); } catch (err) { - console.log(`test skiped, can't create symlink: ${err}`); + const message = err instanceof Error ? err.message : String(err); + console.log(`Test skipped: Unable to create symlink (${message})`); + if (err.code === 'EPERM') { + console.log('Try running tests with administrator privileges'); + } this.skip(); }
Line range hint
1-374: Consider architectural improvements for better test maintainabilityWhile the test file is well-structured, consider these architectural improvements:
- Extract common test setup into shared fixtures
- Create custom test helpers for common assertions
- Use a retry mechanism for timing-sensitive operations
- Add explicit timeouts for all async operations
This would make the tests more maintainable and reliable across different environments and platforms.
test/ts.test.ts (3)
41-41: Consider using optional chaining for clarityThe statement
app && app.proc.kill('SIGTERM')could be replaced with optional chaining to be more idiomatic in modern JavaScript/TypeScript:- app && app.proc.kill('SIGTERM'); + app?.proc?.kill('SIGTERM');🧰 Tools
🪛 Biome (1.9.4)
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
50-50: Consider waiting for the "egg started" message in the test harnessRelying on a fixed delay (
await scheduler.wait(waitTime)) may lead to flaky tests on slower or overloaded environments. Instead, you could wait for the "egg started" message or rely on an event that fires when the application is ready, ensuring reliability.Also applies to: 64-64, 78-78, 110-110
101-101: Use optional chaining to align with modern JavaScriptJust like at line 41, you can apply optional chaining here for improved readability:
- app && app.proc.kill('SIGTERM'); + app?.proc?.kill('SIGTERM');🧰 Tools
🪛 Biome (1.9.4)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/fixtures/subdir-as-basedir/base-dir/app/router.js (1)
2-4: Consider switching fromthistoctxfor clearer context usage.In modern Egg.js or Koa contexts, handlers are typically written as:
app.get('/', async ctx => { ctx.body = `hi, ${ctx.app.config.framework || 'egg'}`; });Using
ctxcan make it more apparent which context is being used and helps avoid confusion in certain scenarios.src/commands/start.ts (2)
274-274: Separate the child assignment from the property assignment.Static analysis flags the statement
const child = this.#child = spawn(command, eggArgs, options);as suspicious. Assigning multiple variables in a single expression can reduce clarity. Consider splitting it into two lines to improve readability and maintainability.
- const child = this.#child = spawn(command, eggArgs, options); + const child = spawn(command, eggArgs, options); + this.#child = child;Also applies to: 290-290
🧰 Tools
🪛 Biome (1.9.4)
[error] 274-274: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
253-283: Robust daemon handling.The message listener for
'egg-ready'properly setsthis.isReadyand detaches the process. If you foresee additional states (e.g., partial startup or multiple steps initialization), consider expanding the message event handling to capture them for advanced debugging or logging.🧰 Tools
🪛 Biome (1.9.4)
[error] 274-274: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
.github/workflows/nodejs.yml(1 hunks)package.json(1 hunks)src/commands/start.ts(1 hunks)test/egg-scripts.test.js(0 hunks)test/egg-scripts.test.ts(1 hunks)test/fixtures/subdir-as-basedir/base-dir/app/router.js(1 hunks)test/fixtures/ts-pkg/package.json(1 hunks)test/fixtures/ts/package.json(1 hunks)test/fixtures/ts/tsconfig.json(1 hunks)test/start.test.ts(8 hunks)test/stop.test.ts(4 hunks)test/ts.test.js(0 hunks)test/ts.test.ts(1 hunks)
💤 Files with no reviewable changes (2)
- test/egg-scripts.test.js
- test/ts.test.js
✅ Files skipped from review due to trivial changes (3)
- test/fixtures/ts/tsconfig.json
- test/fixtures/ts-pkg/package.json
- test/fixtures/ts/package.json
🧰 Additional context used
🪛 Biome (1.9.4)
test/ts.test.ts
[error] 41-41: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 101-101: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/commands/start.ts
[error] 274-274: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 290-290: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
test/start.test.ts
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (19)
test/stop.test.ts (1)
1-14: Well-structured module imports and ESM setup!
The import statements follow modern ESM best practices with proper use of the node: protocol for native modules and correct setup of __filename/__dirname for ESM compatibility.
.github/workflows/nodejs.yml (1)
14-17: LGTM! Node.js version matrix aligns with new requirements.
The changes appropriately:
- Include Node.js 18.19.0 to match the new minimum version requirement
- Add cross-platform testing with ubuntu and macos
- Enable code coverage reporting
Let's verify the Node.js version requirement is consistent across the codebase:
✅ Verification successful
Node.js version requirement is consistently specified across the codebase
The verification confirms that:
- The Node.js version
>=18.19.0is correctly specified inpackage.json - This aligns perfectly with the GitHub Actions workflow matrix that includes
18.19.0and newer versions (18,20,22)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for Node.js version specifications across the codebase
echo "Checking Node.js version specifications..."
rg -g '!node_modules' -g '!dist' "18\.19\.0|>=18\.19\.0"
Length of output: 190
package.json (7)
2-6: LGTM! Package identity and publishing configuration are properly set up.
The package rename and public access configuration are correctly implemented.
19-35: LGTM! Development dependencies are aligned with modern tooling.
The dev dependencies have been updated to support:
- TypeScript development
- Modern ESLint configuration
- Type checking
37-39: LGTM! Node.js engine requirement matches PR objectives.
The Node.js version requirement (>=18.19.0) aligns with the PR's breaking change announcement.
61-69: LGTM! CLI configuration is properly set up with oclif.
The oclif configuration is correctly set up for the command-line interface.
8-18: Verify compatibility of updated dependencies.
The dependencies have been modernized, but we should verify compatibility:
- New: @eggjs/utils, @oclif/core
- Updated: node-homedir, source-map-support, utility
- Removed: await-event, egg-utils
Let's check for any breaking changes in the updated dependencies:
✅ Verification successful
Dependencies update appears compatible with proper migration
The verification shows:
node-homedirv2 only has a breaking change for Node.js < 16 support drop- Removed packages (
await-event,egg-utils) have no remaining usage in the codebase - New packages are properly integrated:
@eggjs/utilsis used for framework path and import resolution functionality (likely replacingegg-utils)@oclif/coreis properly integrated in commands and base command structure
The dependencies modernization has been done correctly with proper migration from old to new packages.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential breaking changes in node-homedir
echo "Checking node-homedir changes from v1 to v2..."
gh api repos/node-modules/node-homedir/releases | jq '.[].body' | grep -i "breaking"
# Check for any remaining usage of removed packages
echo "Checking for residual usage of removed packages..."
rg -l 'await-event|egg-utils' src/
Length of output: 881
Script:
#!/bin/bash
# Let's check for any imports or usage of removed packages in more detail
echo "Checking for await-event usage..."
ast-grep --pattern 'require("await-event")' || ast-grep --pattern 'from "await-event"'
echo "Checking for egg-utils usage..."
ast-grep --pattern 'require("egg-utils")' || ast-grep --pattern 'from "egg-utils"'
# Check usage of new packages to ensure they're properly integrated
echo "Checking @eggjs/utils usage..."
rg "@eggjs/utils" -A 2
echo "Checking @oclif/core usage..."
rg "@oclif/core" -A 2
Length of output: 2223
74-102: Verify dual module format configuration.
The dual module format (CJS/ESM) configuration looks correct with:
type: "module"for ESM default- Proper exports configuration for both formats
- Correct type definitions paths
Let's verify the exports configuration:
✅ Verification successful
Dual module format configuration is correctly implemented
The verification shows that the package is properly configured for dual CJS/ESM support:
- The codebase consistently uses ESM syntax with
.jsextensions in imports - Build setup uses
tshyfor dual-format compilation (as seen inprepublishOnlyscript) - Package exports field correctly defines:
- ESM paths:
dist/esm/index.jswith types - CJS paths:
dist/commonjs/index.jswith types
- ESM paths:
- Entry point (src/index.ts) uses proper ESM syntax
- All source files consistently use ESM imports/exports
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the exports configuration using @arethetypeswrong/cli
echo "Checking exports configuration..."
npx @arethetypeswrong/cli .
Length of output: 200
Script:
#!/bin/bash
# Let's verify the configuration through direct inspection of the build setup and related files
# Check if there are any build scripts in package.json
echo "=== Build Scripts ==="
cat package.json | grep -A 10 '"scripts":'
# Check for build configuration files
echo -e "\n=== Build Config Files ==="
fd -e js -e json -e yaml -e yml tsconfig
fd -e js -e json -e yaml -e yml rollup.config
fd -e js -e json -e yaml -e yml webpack.config
# Check the entry point file
echo -e "\n=== Entry Point Content ==="
cat src/index.ts || echo "Entry point not found"
# Check if there are any ESM-specific imports/exports
echo -e "\n=== ESM Usage Check ==="
rg "import\s+.*\s+from\s+" -t ts -t js
rg "export\s+.*\s+from\s+" -t ts -t js
Length of output: 6039
40-49: Verify the new build and test scripts.
The scripts have been modernized to support TypeScript and dual module formats.
Let's verify the build output structure:
✅ Verification successful
Build and test scripts are properly configured for TypeScript and dual module formats
The verification confirms that:
- TypeScript setup is complete with proper configuration and source files
- Build tooling is correctly set up with
tshyandtshy-afterfor dual CJS/ESM output - Test infrastructure uses
egg-binwith TypeScript support - All necessary dependencies are in place including:
typescriptand@types/*for TypeScript supporttshyecosystem for dual-format builds@arethetypeswrong/clifor type checking- Testing tools:
egg-bin,@types/mocha
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if tshy configuration matches the expected output structure
echo "Checking build output paths..."
fd -t d '^(commonjs|esm)$' dist/
Length of output: 198
Script:
#!/bin/bash
# Let's check the tshy configuration in package.json and tsconfig.json
echo "Checking tshy configuration..."
cat package.json | jq '.tshy'
echo -e "\nChecking TypeScript configuration..."
fd -e json -x cat {} \; -E "package.json" | grep -A 5 '"compilerOptions"'
echo -e "\nChecking if the project uses TypeScript..."
fd -e ts
Length of output: 1220
Script:
#!/bin/bash
# Let's verify the test and build tooling dependencies
echo "Checking dependencies..."
cat package.json | jq '.dependencies, .devDependencies'
echo -e "\nChecking test files structure..."
fd -e test.ts
echo -e "\nChecking if egg-bin is properly configured..."
rg "egg-bin" -A 5
Length of output: 1422
test/fixtures/subdir-as-basedir/base-dir/app/router.js (2)
6-8: Ensure that all config properties exist.
When referencing app.config.env and app.config.pre, consider validating or handling scenarios where these properties might be missing or undefined. This prevents potential runtime errors.
10-12: Verify the exposure of system environment paths.
Returning process.env.PATH may reveal system details that could be sensitive. Ensure this route is secured or restricted if disclosing PATH is not strictly necessary.
test/egg-scripts.test.ts (3)
1-2: Imports look appropriate for ESM migration.
The imports for coffee and the helper file appear correct. The usage of ESM syntax is consistent with the PR objective to support modern Node.js environments.
5-5: Verify correct resolution of eggBin path.
getSourceFilename('../bin/run.js') is a new helper function likely introduced in this PR. Ensure that the returned path is valid under all operating systems and symlinked environments.
Would you like me to generate a verification script to search references to getSourceFilename across the codebase to confirm usage correctness?
7-25: Comprehensive CLI help test coverage.
The show help test thoroughly checks the help command across multiple subcommands (--help, start -h, and stop -h). Ensuring these tests remain up-to-date with future CLI modifications is vital to avoid regressions in user-facing instructions.
src/commands/start.ts (1)
309-359: Graceful shutdown logic.
The code nicely kills the child process and waits before exiting. Confirm that downstream or child processes are also cleaned up. If your system uses internal queues or asynchronous I/O, ensure no leftover resources remain.
test/start.test.ts (4)
518-521: Duplicate beforeEach hooks.
This duplication was already flagged in a previous review. Merging or refactoring them into a single hook can streamline test setup.
- beforeEach(async () => {
- await cleanup(fixturePath);
- });
- beforeEach(async () => {
- app && app.proc && app.proc.kill('SIGTERM');
- await cleanup(fixturePath);
- });
+ beforeEach(async () => {
+ app?.proc?.kill('SIGTERM');
+ await cleanup(fixturePath);
+ });🧰 Tools
🪛 Biome (1.9.4)
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
500-507: Validate insecure --stderr argument usage.
The test ensures that appending shell commands (like touch) to the --stderr argument fails. This check helps protect against injection attacks. Good job on verifying this scenario.
829-853: Ignore stderr scenario.
You correctly test the --ignore-stderr flow and confirm that the process exits with code 0. This ensures that critical startup processes can proceed even with non-fatal stderr logs.
Line range hint 878-883: Timeout test coverage.
The test properly validates the scenario where the server fails to start before timeout. This kind of edge-case coverage is crucial for production environments.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
test/stop.test.ts (1)
Line range hint
514-521: Merge duplicate beforeEach hooks.There are two consecutive
beforeEachhooks that can be combined to improve readability and maintainability.Apply this diff to merge the hooks:
-beforeEach(async () => { - await cleanup(fixturePath); -}); - -beforeEach(async () => { - app && app.proc && app.proc.kill('SIGTERM'); - await cleanup(fixturePath); -}); +beforeEach(async () => { + app?.proc?.kill('SIGTERM'); + await cleanup(fixturePath); +});
♻️ Duplicate comments (1)
test/start.test.ts (1)
512-521:⚠️ Potential issueMerge duplicate beforeEach hooks.
There are two consecutive
beforeEachhooks that can be combined to improve readability and maintainability.Apply this diff to merge the hooks:
-beforeEach(async () => { - await cleanup(fixturePath); -}); - -beforeEach(async () => { - app && app.proc && app.proc.kill('SIGTERM'); - await cleanup(fixturePath); -}); +beforeEach(async () => { + app?.proc?.kill('SIGTERM'); + await cleanup(fixturePath); +});🧰 Tools
🪛 Biome (1.9.4)
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🧹 Nitpick comments (1)
src/commands/stop.ts (1)
43-96: Consider improving timeout handling clarity.While the implementation is generally solid, the timeout handling could be more explicit:
- The comment "wait for 5s to confirm whether any worker process did not kill by master" could be clearer.
- Consider adding a log message before and after the timeout wait to better indicate the waiting state.
this.killProcesses(pids); -// wait for 5s to confirm whether any worker process did not kill by master +this.log(`Waiting ${flags.timeout}ms for worker processes to be terminated by master...`); await scheduler.wait(flags.timeout); +this.log('Timeout completed, checking for remaining worker processes...');
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/fixtures/egg-revert/node_modules/custom-framework/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (3)
src/commands/stop.ts(1 hunks)test/start.test.ts(8 hunks)test/stop.test.ts(4 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
test/start.test.ts
[error] 518-521: Disallow duplicate setup and teardown hooks.
Disallow beforeEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 519-519: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
src/commands/stop.ts (4)
1-17: LGTM! Well-structured imports and platform-specific handling.
The code demonstrates good practices:
- Using
node:prefix for Node.js built-in modules - Proper handling of platform-specific paths
- Support for both CommonJS and ESM worker paths
19-41: LGTM! Well-defined command structure.
The command configuration is clear and well-documented:
- Proper type parameters for class inheritance
- Clear descriptions for all flags and arguments
- Reasonable default timeout value (5000ms)
98-100: LGTM! Clean helper method implementation.
The method provides a good abstraction over the process finding functionality with proper typing and visibility.
102-104: LGTM! Clean process termination implementation.
The method provides a good abstraction over process termination with appropriate default signal and visibility.
[skip ci] ## [4.0.0](v3.1.0...v4.0.0) (2024-12-31) ### ⚠ 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 ## Release Notes - **New Features** - Added support for ECMAScript modules (ESM). - Enhanced CLI with more robust start and stop commands. - Improved TypeScript integration and type safety. - Introduced new commands for stopping an Egg.js server application. - Added new configuration options for logging and process management. - **Improvements** - Updated package configuration for better modularity. - Modernized test infrastructure with TypeScript support. - Refined error handling and logging mechanisms. - Enhanced process management capabilities. - Improved documentation with updated installation instructions and usage examples. - **Breaking Changes** - Renamed package from `egg-scripts` to `@eggjs/scripts`. - Requires Node.js version 18.19.0 or higher. - Significant changes to project structure and module exports. - **Bug Fixes** - Improved process management for server start and stop operations. - Enhanced cross-platform compatibility. - Fixed issues with asynchronous route handlers in various applications. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#63](#63)) ([d9d0bc6](d9d0bc6))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
Summary by CodeRabbit
Release Notes
New Features
Improvements
Breaking Changes
egg-scriptsto@eggjs/scripts.Bug Fixes