feat: support cjs and esm both by tshy #279
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThis pull request represents a significant refactoring of the Changes
Sequence DiagramsequenceDiagram
participant CLI as Egg-Bin CLI
participant Commands as Command Modules
participant Framework as Egg Framework
CLI->>Commands: Select Command
Commands->>Commands: Parse Flags & Arguments
Commands->>Framework: Configure & Initialize
Framework-->>Commands: Ready
Commands->>Commands: Execute Command Logic
Commands-->>CLI: Return Result
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
♻️ Duplicate comments (1)
src/baseCommand.ts (1)
56-56:⚠️ Potential issueFix type redeclaration.
The
Flagstype is already imported from '@oclif/core'.Rename the type to avoid collision:
-type Flags<T extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & T['flags']>; +type CommandFlags<T extends typeof Command> = Interfaces.InferredFlags<typeof BaseCommand['baseFlags'] & T['flags']>;🧰 Tools
🪛 Biome (1.9.4)
[error] 56-56: Shouldn't redeclare 'Flags'. Consider to delete it or rename it.
'Flags' is defined here:
(lint/suspicious/noRedeclare)
🧹 Nitpick comments (10)
scripts/start-cluster.cjs (3)
1-3: Consider removing or refining the ESLint disable comment.Since this file explicitly uses CommonJS imports, disabling the
@typescript-eslint/no-var-requiresrule might be acceptable. However, if you ever transition this file to TypeScript, you can remove this comment and use native ESM imports.
7-13: Add error handling for parsing and dynamic imports.Parsing
process.argv[2]and dynamically importingoptions.frameworkmay fail when the arguments or imports are invalid. To increase robustness, consider wrapping them withtry/catchor adding validation logic. Additionally, you could return a non-zero exit code if these steps fail.async function main() { debug('argv: %o', process.argv); - const options = JSON.parse(process.argv[2]); - debug('start cluster options: %o', options); - const { startCluster } = await importModule(options.framework); - await startCluster(options); + let options; + try { + options = JSON.parse(process.argv[2]); + debug('start cluster options: %o', options); + const { startCluster } = await importModule(options.framework); + await startCluster(options); + } catch (err) { + console.error('Failed to parse options or import module:', err); + process.exit(1); + } }
15-15: Await any uncaught rejections in the main function.If
main()throws an error or rejects a promise, your script will exit silently. Consider appending.catch(...)to handle the error gracefully, providing diagnostic logs or a non-zero exit code on failure.main() + .catch(err => { + console.error('Error starting the cluster:', err); + process.exit(1); + });src/types.ts (2)
1-9: Add JSDoc documentation to improve type clarity.The
PackageEgginterface would benefit from documentation explaining its purpose and property usage.Consider adding documentation:
+/** + * Configuration options for Egg.js projects in package.json + */ export interface PackageEgg { + /** Whether this is a framework package */ framework?: boolean; + /** Whether TypeScript is enabled */ typescript?: boolean; + /** TypeScript compiler configuration */ tscompiler?: string; + /** Whether to generate TypeScript declarations */ declarations?: boolean; + /** Security reverts to apply */ revert?: string | string[]; + /** Modules to require (CommonJS) */ require?: string | string[]; + /** Modules to import (ESM) */ import?: string | string[]; }
6-8: Consider using tuple types for more specific arrays.The string array types could be more specific if they represent fixed sets of values.
If the arrays have a known set of possible values, consider using tuple types:
revert?: string | readonly string[]; require?: string | readonly string[]; import?: string | readonly string[];src/baseCommand.ts (2)
16-42: Consider using AbortController for graceful shutdown.The graceful shutdown implementation could benefit from using
AbortControllerfor better signal handling and cleanup.Consider refactoring to use
AbortController:const children = new Set<ChildProcess>(); -let hadHook = false; +const controller = new AbortController(); function graceful(proc: ChildProcess) { children.add(proc); - if (!hadHook) { - hadHook = true; + if (!controller.signal.aborted) { let signal: NodeJS.Signals; ['SIGINT', 'SIGQUIT', 'SIGTERM'].forEach(event => { - process.once(event, () => { + process.once(event, () => { signal = event as NodeJS.Signals; + controller.abort(); process.exit(0); }); });
131-138: Initialize properties in constructor.Class properties should be initialized in the constructor rather than using the non-null assertion operator.
Consider initializing properties in the constructor:
- protected flags!: Flags<T>; - protected args!: Args<T>; + protected flags: Flags<T>; + protected args: Args<T>; + + constructor(argv: string[], config: Config) { + super(argv, config); + this.flags = {} as Flags<T>; + this.args = {} as Args<T>; + }test/commands/dev.test.ts (1)
Line range hint
220-230: LGTM! Security warning test is properly versioned.The test case correctly handles version compatibility and verifies security warnings.
Consider adding a comment explaining why the test is limited to Node.js versions 18-20:
+ // This test is limited to Node.js 18-20 as the security fix is specific to these versions if (version < 18 || version > 20) return;test/ts.test.ts (2)
225-255: Consider using a more explicit CI environment check.The test correctly verifies custom compiler loading, but the CI detection could be more explicit.
Consider using a more comprehensive CI check:
- if (process.env.CI) { + if (process.env.CI === 'true' || process.env.GITHUB_ACTIONS === 'true') {
263-264: Standardize npm install command usage.There's inconsistent usage of
npxprefix across npm install commands. Some usenpx npminstallwhile others use justnpminstall.Standardize the npm install commands to consistently use or not use the
npxprefix:- await runScript('npminstall ts-node@10.9.2 --no-save', { cwd }); + await runScript('npx npminstall ts-node@10.9.2 --no-save', { cwd });Also applies to: 266-266, 295-298
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
scripts/start-cluster.cjs(1 hunks)src/baseCommand.ts(1 hunks)src/commands/dev.ts(1 hunks)src/types.ts(1 hunks)test/commands/dev.test.ts(7 hunks)test/fixtures/egg-require/package.json(1 hunks)test/fixtures/require-script.mjs(1 hunks)test/ts.test.ts(18 hunks)
✅ Files skipped from review due to trivial changes (2)
- test/fixtures/require-script.mjs
- test/fixtures/egg-require/package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/dev.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/baseCommand.ts
[error] 56-56: Shouldn't redeclare 'Flags'. Consider to delete it or rename it.
'Flags' is defined here:
(lint/suspicious/noRedeclare)
🔇 Additional comments (4)
src/baseCommand.ts (1)
219-220:
Uncomment important code.
The commented code for importing the TypeScript register is crucial for proper TypeScript support.
Uncomment and fix the TypeScript registration:
- // should require tsNodeRegister on current process, let it can require *.ts files
- // e.g.: dev command will execute egg loader to find configs and plugins
- // await importModule(tsNodeRegister);
+ // Register ts-node to handle TypeScript files
+ await importModule(tsNodeRegister);Likely invalid or redundant comment.
test/commands/dev.test.ts (2)
Line range hint 184-205: LGTM! Port detection logic is properly typed and handled.
Good improvements:
- Added type annotations for better type safety
- Properly converts port to string for environment variable
- Updated error message pattern to match new format
28-43: LGTM! ESM support test case looks comprehensive.
The test case properly verifies ESM support by checking:
- Hook script execution
- ESM-specific flags
- Worker initialization
- Framework configuration
Let's verify the hook script exists:
✅ Verification successful
Hook script verification successful
The hook script exists at the expected location and contains a simple console log statement that matches the test's expectation (expect('stdout', /start hook success/). The test case correctly verifies this output along with other ESM-specific checks.
Let me verify the ESM app structure to ensure completeness:
Let me check the package.json to verify ESM configuration:
ESM test case and fixture are properly configured
The verification confirms that:
- The hook script exists and outputs the expected message
- The demo-app-esm fixture is correctly set up with:
"type": "module"in package.json for ESM support- Required framework dependency (aliyun-egg)
- Minimal test structure that matches the test expectations
The test case correctly validates the ESM support by checking the appropriate stdout patterns against this fixture setup.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the hook script exists and check its content
fd -t f 'hook.js' -x cat {}
Length of output: 64
Script:
#!/bin/bash
# Check the demo-app-esm structure and package.json
fd -t f . --base-directory test/fixtures/demo-app-esm
Length of output: 146
Script:
#!/bin/bash
# Check package.json content for ESM configuration
cat test/fixtures/demo-app-esm/package.json
Length of output: 136
test/ts.test.ts (1)
Line range hint 200-207: LGTM! Flag rename improves clarity.
The change from --no-ts to --no-typescript makes the flag more explicit and self-documenting.
| // addNodeOptionsToEnv(`--require ${importResolve('tsconfig-paths/register', { | ||
| // paths: [ getSourceDirname() ], | ||
| // })}`, ctx.env); |
There was a problem hiding this comment.
Uncomment tsconfig-paths registration.
The commented code for registering tsconfig-paths is important for proper module resolution.
Uncomment and fix the tsconfig-paths registration:
- // see https://github.com/eggjs/egg-core/blob/master/lib/loader/egg_loader.js#L49
- // addNodeOptionsToEnv(`--require ${importResolve('tsconfig-paths/register', {
- // paths: [ getSourceDirname() ],
- // })}`, ctx.env);
+ // Register tsconfig-paths for proper module resolution
+ const tsconfigPaths = importResolve('tsconfig-paths/register', {
+ paths: [ getSourceDirname() ],
+ });
+ this.addNodeOptions(`--require ${tsconfigPaths}`);Committable suggestion skipped: line range outside the PR's diff.
| protected async forkNode(modulePath: string, forkArgs: string[], options: ForkNodeOptions = {}) { | ||
| const env = { | ||
| ...this.env, | ||
| ...options.env, | ||
| }; | ||
| const forkExecArgv = [ | ||
| ...this.globalExecArgv, | ||
| ...options.execArgv || [], | ||
| ]; | ||
| const NODE_OPTIONS = env.NODE_OPTIONS ? `NODE_OPTIONS='${env.NODE_OPTIONS}' ` : ''; | ||
| const forkExecArgvString = forkExecArgv.length ? ' ' + forkExecArgv.join(' ') + ' ' : ' '; | ||
| const forkArgsString = forkArgs.map(a => `'${a}'`).join(' '); | ||
| const fullCommand = `${NODE_OPTIONS}${process.execPath}${forkExecArgvString}${modulePath} ${forkArgsString}`; | ||
| if (options.dryRun) { | ||
| console.log('dry run: $ %s', fullCommand); | ||
| return; | ||
| } | ||
|
|
||
| options = { | ||
| stdio: 'inherit', | ||
| env, | ||
| cwd: this.flags.base, | ||
| ...options, | ||
| execArgv: forkExecArgv, | ||
| }; | ||
| const proc = fork(modulePath, forkArgs, options); | ||
| debug('Run fork pid: %o\n\n$ %s\n\n', | ||
| proc.pid, | ||
| fullCommand); | ||
| graceful(proc); | ||
|
|
||
| return new Promise<void>((resolve, reject) => { | ||
| proc.once('exit', code => { | ||
| debug('fork pid: %o exit code %o', proc.pid, code); | ||
| children.delete(proc); | ||
| if (code !== 0) { | ||
| const err = new ForkError(modulePath + ' ' + forkArgs.join(' ') + ' exit with code ' + code, code); | ||
| reject(err); | ||
| } else { | ||
| resolve(); | ||
| } | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add timeout handling to fork process.
The forkNode method should include a timeout mechanism to prevent hanging processes.
Add timeout handling:
protected async forkNode(modulePath: string, forkArgs: string[], options: ForkNodeOptions = {}) {
+ const timeout = options.timeout ?? 60000; // Default 60s timeout
// ... existing code ...
return new Promise<void>((resolve, reject) => {
+ const timer = setTimeout(() => {
+ proc.kill();
+ reject(new ForkError(`Timeout after ${timeout}ms: ${modulePath}`, -1));
+ }, timeout);
proc.once('exit', code => {
+ clearTimeout(timer);
debug('fork pid: %o exit code %o', proc.pid, code);
children.delete(proc);
// ... existing code ...
});
});
}📝 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.
| protected async forkNode(modulePath: string, forkArgs: string[], options: ForkNodeOptions = {}) { | |
| const env = { | |
| ...this.env, | |
| ...options.env, | |
| }; | |
| const forkExecArgv = [ | |
| ...this.globalExecArgv, | |
| ...options.execArgv || [], | |
| ]; | |
| const NODE_OPTIONS = env.NODE_OPTIONS ? `NODE_OPTIONS='${env.NODE_OPTIONS}' ` : ''; | |
| const forkExecArgvString = forkExecArgv.length ? ' ' + forkExecArgv.join(' ') + ' ' : ' '; | |
| const forkArgsString = forkArgs.map(a => `'${a}'`).join(' '); | |
| const fullCommand = `${NODE_OPTIONS}${process.execPath}${forkExecArgvString}${modulePath} ${forkArgsString}`; | |
| if (options.dryRun) { | |
| console.log('dry run: $ %s', fullCommand); | |
| return; | |
| } | |
| options = { | |
| stdio: 'inherit', | |
| env, | |
| cwd: this.flags.base, | |
| ...options, | |
| execArgv: forkExecArgv, | |
| }; | |
| const proc = fork(modulePath, forkArgs, options); | |
| debug('Run fork pid: %o\n\n$ %s\n\n', | |
| proc.pid, | |
| fullCommand); | |
| graceful(proc); | |
| return new Promise<void>((resolve, reject) => { | |
| proc.once('exit', code => { | |
| debug('fork pid: %o exit code %o', proc.pid, code); | |
| children.delete(proc); | |
| if (code !== 0) { | |
| const err = new ForkError(modulePath + ' ' + forkArgs.join(' ') + ' exit with code ' + code, code); | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| } | |
| protected async forkNode(modulePath: string, forkArgs: string[], options: ForkNodeOptions = {}) { | |
| const timeout = options.timeout ?? 60000; // Default 60s timeout | |
| const env = { | |
| ...this.env, | |
| ...options.env, | |
| }; | |
| const forkExecArgv = [ | |
| ...this.globalExecArgv, | |
| ...options.execArgv || [], | |
| ]; | |
| const NODE_OPTIONS = env.NODE_OPTIONS ? `NODE_OPTIONS='${env.NODE_OPTIONS}' ` : ''; | |
| const forkExecArgvString = forkExecArgv.length ? ' ' + forkExecArgv.join(' ') + ' ' : ' '; | |
| const forkArgsString = forkArgs.map(a => `'${a}'`).join(' '); | |
| const fullCommand = `${NODE_OPTIONS}${process.execPath}${forkExecArgvString}${modulePath} ${forkArgsString}`; | |
| if (options.dryRun) { | |
| console.log('dry run: $ %s', fullCommand); | |
| return; | |
| } | |
| options = { | |
| stdio: 'inherit', | |
| env, | |
| cwd: this.flags.base, | |
| ...options, | |
| execArgv: forkExecArgv, | |
| }; | |
| const proc = fork(modulePath, forkArgs, options); | |
| debug('Run fork pid: %o\n\n$ %s\n\n', | |
| proc.pid, | |
| fullCommand); | |
| graceful(proc); | |
| return new Promise<void>((resolve, reject) => { | |
| const timer = setTimeout(() => { | |
| proc.kill(); | |
| reject(new ForkError(`Timeout after ${timeout}ms: ${modulePath}`, -1)); | |
| }, timeout); | |
| proc.once('exit', code => { | |
| clearTimeout(timer); | |
| debug('fork pid: %o exit code %o', proc.pid, code); | |
| children.delete(proc); | |
| if (code !== 0) { | |
| const err = new ForkError(modulePath + ' ' + forkArgs.join(' ') + ' exit with code ' + code, code); | |
| reject(err); | |
| } else { | |
| resolve(); | |
| } | |
| }); | |
| }); | |
| } |
| path.join(cwd, './node_modules/egg'), | ||
| ); | ||
|
|
||
| const { stderr, code } = await coffee.fork(eggBin, [ 'dev', '--tsc', 'ts-node/register' ], { | ||
| cwd, | ||
| env: { | ||
| NODE_DEBUG: 'egg-bin*', | ||
| NODE_DEBUG: '@eggjs/bin*', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize debug namespace across tests.
The debug namespace varies between egg-bin* and @eggjs/bin*. This should be consistent with the new package name.
Standardize all debug namespaces to use @eggjs/bin*:
- NODE_DEBUG: 'egg-bin*',
+ NODE_DEBUG: '@eggjs/bin*',Also applies to: 280-280, 407-407
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
test/commands/dev.test.ts (1)
221-221: Consider a more flexible version check.The current version check
version !== 20might break with Node.js 21+. Consider using a range check instead.-if (version !== 20) return; +if (version < 20) return;test/commands/cov.test.ts (1)
206-206: Consider a more flexible version check.Similar to dev.test.ts, the version check should use a range check for better future compatibility.
-if (version !== 20) return; +if (version < 20) return;test/commands/test.test.ts (1)
Line range hint
328-336: Add comment explaining Node.js version constraint.The test for
egg.revertis conditionally run only on Node.js 20. Please add a comment explaining why this version constraint exists to improve maintainability.test/ts.test.ts (2)
256-259: Consider extracting repeated CI-specific install logic.The CI-specific npm install logic is duplicated across multiple test cases. Consider extracting this into a helper function to improve maintainability.
Example:
async function installDependencies(cwd: string, packages?: string) { const cmd = process.env.CI ? `npminstall ${packages || ''}` // don't use npmmirror.com on CI : `npminstall -c ${packages || ''}`; await runScript(cmd, { cwd }); }Also applies to: 385-388
Line range hint
23-27: Consider implementing a better debug strategy.There are numerous commented-out
.debug()calls throughout the test file. Consider implementing a more maintainable approach:
- Use a debug flag in test configuration
- Create a helper function that conditionally enables debugging
Example:
function debuggableTest(test: coffee.CoffeeScript) { return process.env.DEBUG ? test.debug() : test; }Also applies to: 33-38, 47-52, 58-63, 92-97
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(1 hunks)test/commands/cov.test.ts(6 hunks)test/commands/dev.test.ts(7 hunks)test/commands/test.test.ts(13 hunks)test/fixtures/example-ts-cluster/test/index.test.ts(1 hunks)test/ts.test.ts(18 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/fixtures/example-ts-cluster/test/index.test.ts
🔇 Additional comments (12)
package.json (4)
2-5: LGTM! Package metadata changes align with the PR objectives.
The package renaming, version bump to 7.0.0, and Node.js engine requirement changes are properly configured for this breaking change.
Also applies to: 17-18
38-38: Pin beta dependencies to specific versions.
The beta dependencies should be pinned to specific versions for better stability.
Also applies to: 47-47, 57-57
81-99: LGTM! Proper dual package configuration.
The configuration correctly supports both ESM and CommonJS:
- ESM is set as the default format
- Exports field properly configures both formats with their type definitions
- Build outputs are correctly mapped for both formats
113-120: LGTM! Proper oclif configuration.
The oclif configuration is correctly set up with ESM commands directory and additional help flag support.
test/commands/dev.test.ts (2)
28-43: LGTM! Comprehensive ESM support test coverage.
The test cases properly verify:
- ESM module loading with hooks
- Support for both --require and --import flags
- Correct environment configuration
Also applies to: 163-171
184-185: LGTM! Improved type safety with annotations.
The addition of type annotations for server and serverPort variables enhances code safety.
test/commands/cov.test.ts (1)
Line range hint 15-21: LGTM! Enhanced test coverage verification.
The assertCoverage function provides comprehensive verification of coverage artifacts.
test/commands/test.test.ts (3)
2-3: LGTM! Import statements and test suite setup look good.
The changes properly follow ES modules requirements by including .js extensions in imports.
Also applies to: 7-9
23-37: Well-structured test cases for edge scenarios!
The new test cases improve coverage by handling:
- Empty change sets (
--no changed files) - Invalid base directory scenarios
This helps ensure robust error handling.
Line range hint 211-220: Document reason for skipping parallel test.
The parallel test case has been skipped without explanation. Please add a comment explaining why this test is being skipped to help future maintainers understand the reasoning.
test/ts.test.ts (2)
Line range hint 304-305: Standardize debug namespace.
The debug namespace still uses egg-bin* instead of @eggjs/bin*. This should be updated for consistency with the new package name.
- NODE_DEBUG: 'egg-bin*',
+ NODE_DEBUG: '@eggjs/bin*',Line range hint 193-200: Document breaking change in flag name.
The flag has been renamed from --no-ts to --no-typescript for better clarity. This is a breaking change that should be documented in the changelog.
Run this script to check for usages of the old flag:
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/commands/test.test.ts (2)
328-330: Consider more flexible version handling.The current version check
if (version !== 20)seems overly restrictive, especially since the PR supports Node.js ≥ 18.19.0.Consider using a more flexible version check:
-if (version !== 20) return; +if (version < 18) return;
25-26: Consider environment-based debug control.Instead of commenting out multiple
.debug()calls, consider using an environment variable to control debug output.Example implementation:
-// .debug() +if (process.env.EGG_BIN_DEBUG) { + .debug() +}This would make it easier to enable debugging when needed without code changes.
Also applies to: 84-86, 98-99, 110-112
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/commands/test.test.ts(13 hunks)
🔇 Additional comments (6)
test/commands/test.test.ts (6)
2-3: LGTM! Import and path handling changes align with ESM requirements.
The changes properly implement ESM-compatible imports with .js extensions and introduce consistent path handling through helper functions.
Also applies to: 8-9
23-37: LGTM! Well-structured test cases for edge scenarios.
The new test cases properly cover important edge scenarios:
- Handling of no changed files with
-cflag - Error handling for non-existent base directory
Line range hint 190-199: LGTM! Critical test case for TypeScript ESM support.
This test case is essential for validating the PR's objective of supporting ESM modules, specifically verifying:
- TypeScript ESM module execution
- Clean execution without experimental warnings
Line range hint 211-220: Verify reason for skipping parallel test.
The parallel test case has been marked as skipped without explanation. This could impact the test coverage of parallel execution functionality.
Could you please:
- Explain why this test needs to be skipped
- Create a tracking issue if there are known problems with parallel execution
- Add a comment in the code explaining the skip reason
317-319: LGTM! Comprehensive timeout handling for different debug scenarios.
The implementation properly handles timeouts across various debugging contexts, including WebStorm debug mode.
Line range hint 332-334: LGTM! Important security warning verification.
The test properly verifies the display of security warnings related to CVE-2023-46809 (Marvin attack on PKCS#1 padding).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/ts.test.ts (1)
257-258: Standardize npminstall command usageThere are inconsistencies in how npminstall is used across different test cases:
- Some use
npx npminstallwhile others use justnpminstall- Different approaches to handling CI environment
Consider standardizing the npminstall command usage across all test cases for better maintainability.
Also applies to: 260-260, 291-294
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/ts.test.ts(16 hunks)
🔇 Additional comments (5)
test/ts.test.ts (5)
6-7: LGTM: Import statements properly updated for ESM compatibility
The changes correctly add .js extensions to imports and introduce helper functions, which aligns with ESM requirements and improves code organization.
18-18: LGTM: Improved path handling with helper functions
The changes properly utilize helper functions for path management, making the code more maintainable and less prone to path-related issues.
Also applies to: 22-22
43-43: LGTM: Comprehensive test coverage for TypeScript support
The test cases properly verify TypeScript functionality in various scenarios while using improved path handling methods.
Also applies to: 77-77
239-239: Standardize debug namespace to use '@eggjs/bin'*
The debug namespace should be updated to use '@eggjs/bin*' for consistency with the new package name.
245-246: Consider maintaining version checks
The version-specific assertions have been commented out. While this makes tests less brittle, it might miss version compatibility issues. Consider keeping a relaxed version check or documenting why specific version checks were removed.
Also applies to: 279-280
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
test/commands/dev.test.ts (1)
Enable the skipped test case - declarations feature is actively supported
The
declarationsfunctionality is still actively supported in the codebase:
- It's defined as a boolean option in
src/types.ts- Implemented in
baseCommand.tswith proper flag handling- Can be configured via package.json's
egg.declarations- Used in the example fixture
example-declarationsThe test should be enabled as it verifies an actively used feature. The test expectations also align with the current implementation:
- Checks for worker configuration
- Validates baseDir path
- Verifies framework identification
- Confirms typescript declarations generation
🔗 Analysis chain
Line range hint
57-67: Investigate skipped test case.This test case for declarations is currently skipped. We should either:
- Remove it if it's no longer relevant
- Fix and enable it if it's still needed
- Document why it's skipped
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if declarations functionality is still supported rg -l "declarations.*=.*true" --type tsLength of output: 67
Script:
#!/bin/bash # Let's check the implementation and usage of declarations option rg "declarations" --type ts -B2 -A2 # Also check if there are any related configuration files fd -e json -x cat {} \; | rg "declarations"Length of output: 1973
🧹 Nitpick comments (4)
test/fixtures/example-port/app.js (1)
1-1: Confirm the export style consistency with ESM vs. CommonJSSince the PR's main objective is to support both CJS and ESM, ensure that this CommonJS export aligns with the overall migration plan. If the rest of the project is using ESM, consider an ESM-compatible export mechanism to maintain consistency.
test/commands/dev.test.ts (1)
29-44: Enhance ESM test coverage.Consider adding assertions to verify the module type and import.meta properties to ensure proper ESM context.
it('should startCluster success on ESM', () => { const cwd = getFixtures('demo-app-esm'); const hook = path.join(cwd, 'hook.js'); return coffee.fork(eggBin, [ 'dev', '-r', hook ], { cwd, }) // .debug() .expect('stdout', /start hook success/) .expect('stdout', /'--import'/) .expect('stdout', /"workers":1/) .expect('stdout', /"baseDir":".*?demo-app-esm"/) .expect('stdout', /"framework":".*?aliyun-egg"/) .expect('stdout', /NODE_ENV: development/) + .expect('stdout', /"type":"module"/) + .expect('stdout', /import\.meta\.url/) .expect('code', 0) .end(); });test/ts.test.ts (2)
245-246: Improve version assertion flexibility.The commented version assertions suggest a need for more flexible version checking. Consider using semver ranges instead of exact version matches.
-// assert.match(stderr, /ts-node@10\.\d+\.\d+/); +assert.match(stderr, /ts-node@\d+\.\d+\.\d+/);Also applies to: 279-280
218-219: Document platform limitations.The Windows platform check should include a comment explaining why the test is skipped on Windows.
-if (process.platform === 'win32') return; +// Skip on Windows: Custom TypeScript compiler loading behaves differently on Windows +if (process.platform === 'win32') return;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/commands/dev.test.ts(5 hunks)test/fixtures/example-port/app.js(1 hunks)test/fixtures/example-port/app/router.js(0 hunks)test/fixtures/example-port/config/config.default.js(1 hunks)test/ts.test.ts(16 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/example-port/app/router.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/example-port/config/config.default.js
🔇 Additional comments (4)
test/fixtures/example-port/app.js (1)
2-5: Avoid using process.exit(0) for abrupt termination
Calling process.exit(0) here can prevent any asynchronous cleanup or pending I/O from completing. If there are no further tasks, it might be acceptable, but otherwise you may need a more graceful shutdown to allow tasks like database connections or file writes to complete before exit.
test/commands/dev.test.ts (1)
197-205: LGTM! Port handling is robust.
The code correctly handles port detection and type conversion using String().
test/ts.test.ts (2)
Line range hint 9-16: LGTM! Robust file copy implementation.
The code properly handles file copying with a fallback mechanism for older Node.js versions.
239-239: LGTM! Debug namespace is standardized.
The debug namespace has been correctly standardized to '@eggjs/bin*' across all test cases.
Also applies to: 274-274, 305-305
| .expect('stdout', /"port":6001/) | ||
| .expect('code', 0) | ||
| .end(); | ||
| }); | ||
| }); | ||
|
|
||
| it('should support egg.revert', () => { | ||
| if (version < 18 || version > 20) return; | ||
| if (version !== 20) return; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoded version check.
The hardcoded version check version !== 20 might break with future Node.js versions. Consider using a version range check instead.
-if (version !== 20) return;
+if (version < 20) return; // or use semver for more precise version checkingCommittable suggestion skipped: line range outside the PR's diff.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
===========================================
- Coverage 100.00% 95.80% -4.20%
===========================================
Files 13 21 +8
Lines 95 1050 +955
Branches 19 174 +155
===========================================
+ Hits 95 1006 +911
- Misses 0 44 +44 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
package.json(1 hunks)
🔇 Additional comments (4)
package.json (4)
38-43: Pin beta dependencies to specific versions for better stability
Multiple dependencies are using the generic "beta" tag:
"@eggjs/mock": "beta"(both in peer and dev dependencies)"egg": "beta"
Also applies to: 47-47, 57-57
81-99: Well-structured dual ESM/CJS support configuration
The module configuration follows best practices for dual package hazard handling:
- Proper
type: "module"declaration - Correct exports field configuration for both ESM and CJS
- Type declarations available for both module systems
Also applies to: 110-112
113-120: LGTM: Oclif configuration follows framework conventions
The Oclif setup is properly configured:
- Correct commands directory path
- Appropriate binary name
- Custom help flag support
69-79: Verify the build process compatibility
The build process uses modern tooling:
- tshy for TypeScript builds
- c8 for coverage
- Custom script copying step
Let's verify the build process works in CI:
✅ Verification successful
Let me gather more information about the CI setup and build process.
Let me check one more aspect about the test workflow being used.
Build process appears well-configured and compatible
The repository has a comprehensive CI setup that covers the build process:
- Main CI workflow runs tests across multiple OS (Ubuntu, macOS, Windows) and Node.js versions (18.19.0, 18, 20, 22, 23)
- Dedicated publish workflow that runs the build (
prepublishOnly) before publishing - Release workflow for master branch deployments
The build scripts in package.json align well with these CI workflows, particularly:
- The
prepublishOnlyscript is correctly triggered in the publish workflow - Test and coverage commands are available for the CI workflow
- The build process includes TypeScript compilation (tshy), type checking (attw), and script copying
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if CI workflow includes all necessary build steps
rg -A 5 "npm (run|test)" .github/workflows/
Length of output: 230
Script:
#!/bin/bash
# Check all workflow files and their content
fd . .github/workflows/ -e yml -e yaml --exec cat {}
# Check if there are any other test or build related workflows
rg -A 5 "(build|test|ci)" .github/workflows/
Length of output: 2267
Script:
#!/bin/bash
# Check the reusable workflow being referenced
gh api repos/node-modules/github-actions/contents/.github/workflows/node-test.yml | jq -r '.content' | base64 -d
# Also check if there are any build-related issues
rg -i "(build|test) (fail|error)" .github/issues/
Length of output: 317
| "name": "@eggjs/bin", | ||
| "version": "7.0.0-beta.4", |
There was a problem hiding this comment.
💡 Codebase verification
Breaking changes need to be documented in CHANGELOG.md and README.md
The package introduces significant breaking changes without proper documentation:
- Package rename from
egg-binto@eggjs/bin - Major version bump to 7.0.0-beta.4
- Node.js requirement increased to >= 18.19.0
- Switch to dual ESM/CJS support with Oclif framework
While the README.md shows the new package name in badges and installation instructions, there's no clear migration guide or documentation of breaking changes for users upgrading from previous versions. The CHANGELOG.md also lacks entries for version 7.0.0-beta.4.
- Add a "Breaking Changes" or "Migration Guide" section in README.md
- Document the changes in CHANGELOG.md under version 7.0.0-beta.4
- Include upgrade steps and new requirements
🔗 Analysis chain
Breaking changes need clear documentation
This update introduces significant breaking changes:
- Package renamed from
egg-binto@eggjs/bin - Major version bump to 7.0.0-beta.4
- Minimum Node.js version increased to 18.19.0
Let's verify if these breaking changes are documented:
Also applies to: 17-19
Let's gather more information to check for documentation about the specific breaking changes in the current version.
Let's check one more thing to see if there are any recent commits or PRs that might document these changes.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if breaking changes are documented in changelog or readme
rg -i "breaking|migration|upgrade" README.md CHANGELOG.md
Length of output: 1234
Script:
#!/bin/bash
# Check recent changes and migration guides
rg -i "7\.0\.0|@eggjs/bin|node.*18" README.md CHANGELOG.md -A 3
# Check package.json for any migration or upgrade notes
cat package.json
Length of output: 4710
Script:
#!/bin/bash
# Check recent commits and PRs for documentation
git log -n 10 --oneline
# Check if there's a migration guide or docs directory
fd -t f -e md
Length of output: 38
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
test/my-egg-bin.test.ts (2)
60-62: Consider disabling debug mode in CI environment.The test correctly verifies custom framework integration, but the enabled debug mode might generate excessive output in CI environments.
Consider conditionally enabling debug mode:
- .debug() + // Enable debug only when needed + // .debug()
Line range hint
1-74: Consider restructuring tests to match Oclif conventions.Since the project is migrating to Oclif, consider organizing test files according to Oclif's recommended structure, typically placing command tests under
test/commands/. This would improve maintainability and align with Oclif's best practices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/my-egg-bin.test.ts(4 hunks)test/ts.test.ts(16 hunks)
🔇 Additional comments (12)
test/ts.test.ts (8)
6-7: LGTM: Import and path management improvements
The changes properly support ESM by including the .js extension and introduce helper functions for consistent path management.
Also applies to: 18-18
22-22: LGTM: Consistent path management across test cases
The use of getFixtures() helper function across all test cases improves maintainability and reduces path construction duplication.
Also applies to: 32-32, 43-43, 77-77, 88-88, 160-160
172-173: LGTM: Improved test isolation
Using getFixtures() for temporary paths ensures proper test isolation and consistent path management.
193-194: LGTM: Improved flag naming clarity
The change from --no-ts to --no-typescript makes the flag more explicit and aligns with TypeScript conventions.
239-239: Standardize debug namespace
The debug namespace should be consistently updated to use '@eggjs/bin*' to align with the new package name.
Also applies to: 274-274, 305-305
245-246: LGTM: More maintainable assertions
Removing version-specific assertions in favor of more general ones reduces test brittleness and improves maintainability.
Also applies to: 279-280
257-260: LGTM: Improved CI handling clarity
The changes clearly distinguish between CI and non-CI environments with appropriate comments and consistent command patterns.
Also applies to: 387-390
242-244: LGTM: Enhanced test debugging and assertions
The changes improve test debugging capabilities and make assertions more maintainable.
Also applies to: 277-278, 308-308, 405-407
test/my-egg-bin.test.ts (4)
72-72: LGTM! Robust version check implementation.
The use of regex pattern for version checking provides better flexibility and aligns with Oclif's version output format.
1-6: LGTM! Verify helper function implementation.
The changes to use .js extensions and the getFixtures helper function align with ESM requirements and improve path handling consistency.
Let's verify the helper function implementation:
✅ Verification successful
Helper function implementation verified and working as expected
The getFixtures helper function is correctly implemented in test/helper.ts. It's a simple utility that joins the test directory's path with a 'fixtures' subdirectory and the provided filename, which is exactly what we need for consistent test file path handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the getFixtures helper function implementation
ast-grep --pattern 'export function getFixtures($_) {
$$$
}'
Length of output: 221
22-22: Verify nsp command implementation in the new Oclif structure.
The test updates reflect the migration to Oclif framework and add coverage for flag handling.
Let's verify the nsp command implementation:
Also applies to: 31-37
✅ Verification successful
NSP command implementation matches test expectations
The NSP command implementation in the Oclif structure is correctly aligned with the test cases:
- Command extends BaseCommand and has proper description "nsp check"
- Implements the
--fooflag with boolean type - Output messages match the test expectations:
- Base message: "run nsp check at baseDir: ..."
- Flag message: "foo is true" when --foo is used
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the nsp command implementation in the new Oclif structure
ast-grep --pattern 'class Nsp extends $_ {
$$$
}'
Length of output: 53
Script:
#!/bin/bash
# Let's try searching for nsp-related files and commands
fd -t f -e ts -e js nsp
# Search for nsp command patterns
rg -i "nsp" --type ts --type js -C 3
Length of output: 6095
Script:
#!/bin/bash
# Let's examine the content of the nsp command implementation
cat test/fixtures/my-egg-bin/cmd/nsp.ts
Length of output: 601
43-54: Verify command registration in the Oclif manifest.
The help text updates align with Oclif's format and indicate proper TypeScript integration.
Let's verify the command registration:
✅ Verification successful
Command registration is properly implemented with Oclif
The help text updates are correctly aligned with the actual command implementations. The verification shows:
- Commands are properly registered and extend BaseCommand which inherits from Oclif's Command class
- The help text matches the static descriptions in the command classes:
testcommand: "Run the test"devcommand: "Start server at local dev mode" with TypeScript compiler options- Commands properly define their flags and arguments using Oclif's flag system
The package.json also contains the correct Oclif configuration pointing to the commands directory.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for oclif manifest file and command registration
fd -e json oclif.manifest.json
# Verify command class implementations
ast-grep --pattern 'export default class $_ extends Command {
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Let's try a different approach to find command implementations
# Search for command files and their content
fd -e ts "dev.ts|test.ts|nsp.ts" -x cat {}
# Look for Command imports and class definitions
rg -l "import.*Command.*from.*\@oclif"
# Check package.json for oclif configuration
cat package.json | jq '.oclif'
Length of output: 57412
Script:
#!/bin/bash
# Let's try a different approach to find command implementations
# Look for command files in the commands directory
fd -t f . src/commands/
# Check command class implementations with proper escaping
rg -l "import.*Command.*from.*@oclif"
# Look for specific command implementations
ast-grep --pattern 'export default class $_ extends Command {
static description = $_;
$$$
}'
Length of output: 245
Script:
#!/bin/bash
# Let's examine the command implementations
cat src/commands/dev.ts
cat src/commands/test.ts
# Also check the base command implementation
cat src/baseCommand.ts
Length of output: 23974
[skip ci] ## [7.0.0](v6.13.0...v7.0.0) (2024-12-27) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 eggjs/egg#5257 use https://oclif.io <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Removed pull request template for contributors. - Introduced a new workflow for automating commit publishing. - Added scripts for executing Node.js applications in development mode. - Updated the README to reflect the new package name and other details. - Enhanced command classes for testing and development functionalities. - Added new utility functions for better path handling in tests. - Introduced new interface for package configuration. - Added support for TypeScript with updated configurations. - Implemented a new logging mechanism in hooks for better debugging. - Introduced a new class for managing server readiness in demo applications. - **Bug Fixes** - Adjusted import paths in tests for compatibility with new module structure. - **Documentation** - Updated README and various documentation links to reflect changes in package structure. - **Chores** - Updated package configurations, including versioning and dependencies. - Removed obsolete files and configurations from the project. - Enhanced test suite structure and clarity. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#279](#279)) ([7078748](7078748))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
eggjs/egg#5257
use https://oclif.io
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores