feat: support cjs and esm both by tshy #117
Conversation
BREAKING CHANGE: drop Node.js < 18.19.0 support part of eggjs/egg#3644
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 5 minutes and 30 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 (2)
WalkthroughThis pull request represents a comprehensive refactoring of the Changes
Sequence DiagramsequenceDiagram
participant Master
participant AgentWorker
participant AppWorker
Master->>AgentWorker: Fork Agent
Master->>AppWorker: Fork Application
AgentWorker-->>Master: Agent Ready
AppWorker-->>Master: Application Ready
Master->>Master: Manage Workers
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #117 +/- ##
==========================================
+ Coverage 92.57% 93.49% +0.91%
==========================================
Files 15 1 -14
Lines 2021 169 -1852
Branches 337 22 -315
==========================================
- Hits 1871 158 -1713
+ Misses 150 11 -139 ☔ View full report in Codecov by Sentry. |
commit: |
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (49)
test/fixtures/apps/before-close/app.js (1)
1-1: Consider replacingrequirewith ES module import syntax.
While this is perfectly valid in a CommonJS context, this PR aims to introduce ESM support. Migrating to an ES module import (e.g.,import { scheduler } from 'node:timers/promises';) will ensure consistency with ESM usage.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/before-close/agent.js (1)
1-1: Consider migrating to ES module import for uniformity.
Switching to an ESM import statement aligns with the PR’s overall goal of adding ESM support.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/worker-close-timeout/agent.js (1)
1-1: ESM import could be considered for future-proofing.
Rewriting thisrequirestatement as animport { scheduler } from 'node:timers/promises';is more in line with the PR's ESM direction.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/worker-close-timeout/app.js (1)
1-1: Recommend adopting ES module syntax.
Replacingrequirewithimportwould unify the codebase under ESM conventions.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
src/utils/worker_manager.ts (1)
76-76: Exception handling logic
Resettingthis.exceptiononly when both agent and worker counts are nonzero is sensible if the application requires both to remain alive. However, consider partial states: if either agent or workers are intentionally zero, it may increment the exception unnecessarily.src/utils/mode/impl/worker_threads/agent.ts (1)
52-52: Assignment within an expressionconst agentWorker = this.instance = new AgentThreadWorker(worker);This double assignment may reduce readability. Consider splitting into two lines for clarity:
- const agentWorker = this.instance = new AgentThreadWorker(worker); + const agentWorker = new AgentThreadWorker(worker); + this.instance = agentWorker;🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: 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)
src/utils/terminate.ts (1)
13-20:terminatefunction
Collecting the main PID and its children, then callingkillProcessandkillChildrenconcurrently is clean. Consider logging or propagating errors if either kill step fails.src/utils/mode/base/app.ts (3)
10-15: Encapsulate the worker instance more tightly.Consider using a private or protected access modifier for
instanceto prevent accidental external modifications, reinforcing encapsulation.
51-53: Clarify the “clean” method requirements.Since
clean()throws an error indicating it must be overridden, consider making it explicitly abstract, ensuring subclasses provide their own implementations.
94-108: Ensure deeper testing of constructor parameters.Highlighted constructor parameters (
MasterOptions,log,logger,messenger, andisProduction) are crucial fields. Confirm that there are unit or integration tests covering various configurations to ensure robust handling of edge cases (e.g., missing or invalid parameters).src/utils/mode/impl/process/app.ts (2)
9-31: Expose “AppProcessWorker” status transitions.Currently, we rely on
clean()or event listeners for transitions. Providing specific methods to handle transitions (e.g., from “starting” to “online” to “exiting”) could lead to more transparent state management and easier debugging.
Line range hint
78-93: Consider optimizing debug concurrency.You incrementally increase
debugPortfor each forked worker. Validate that parallel debug sessions on many workers do not conflict with ephemeral port usage in local dev environments, especially if the number of workers is large.src/utils/mode/impl/worker_threads/app.ts (3)
21-22: Propagate the “exitedAfterDisconnect” property accurately.Here it's a constant that always returns
true. If desired, consider reflecting the real state for clarity, or document the rationale for always returningtrue.
37-38: Revisit exit code management strategy.Returning a constant zero for
exitCodemay mask errors. Consider storing the worker’s termination code to facilitate debugging or post-exit reporting.
Line range hint
81-117: Evaluate unconditional refork logic.After a worker exits, you refork after a 1-second sleep. In an environment with repeated crashes, this could lead to thrashing. Consider implementing a maximum refork threshold or backoff strategy for better stability.
src/utils/messenger.ts (1)
172-176: Prevent silent failures.If
this.#hasParentisfalse, messages are dropped. Consider logging or buffering messages to ensure the application or user is aware that the parent isn’t receiving them.src/utils/options.ts (1)
90-169: parseOptions Function
Overall logic is solid. It merges default options, verifies existence of required files, and handles deprecated HTTPS fields.One small improvement is to fail fast or provide a fallback if
parseIntresults inNaNfor fields likeportorworkers. Right now, the code silently proceeds; consider adding a validation or warning message.src/app_worker.ts (1)
82-82: Separate the assignment for clarity.
This chained assignment can make debugging harder. Prefer splitting it into two lines for better readability and maintainability.- const port = app.options.port = options.port || listenConfig.port; + app.options.port = options.port || listenConfig.port; + const port = app.options.port;🧰 Tools
🪛 Biome (1.9.4)
[error] 82-82: 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)
src/master.ts (1)
279-279: Use Number.isNaN for safer numeric checks.
isNaN(parseInt(ip[i]))can lead to unexpected type coercions. Replace withNumber.isNaN(...)to avoid subtle pitfalls.- if (!isNaN(parseInt(ip[i]))) { + if (!Number.isNaN(parseInt(ip[i], 10))) { s += ip[i]; }🧰 Tools
🪛 Biome (1.9.4)
[error] 279-279: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.(lint/suspicious/noGlobalIsNan)
test/fixtures/apps/framework/index.js (1)
1-1: Consider upgrading to ES module imports
You’re requiringeggand then exporting viaexports. While this is valid, it partially mixes CommonJS and ES module conventions. If your environment fully supports ESM, consider usingimport { startCluster } from 'egg';for consistency.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/framework/lib/agent.js (1)
2-2: Use ES module imports for consistency
You’ve destructuredAgentfromeggusingrequire. If you aim to fully adopt ESM, consider switching toimport { Agent } from 'egg';.🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/egg/index.js (1)
1-2: ESLint rule disabled
Disabling@typescript-eslint/no-var-requiresis valid if you needrequire()for bridging CommonJS. However, it’s a good practice to remove or reevaluate this in the future once you fully switch to ESM.test/fixtures/apps/script-start/start-server.js (1)
Line range hint
1-20: Switch to ES module imports for consistency.While using
require('node:timers/promises')is valid, consider adopting ES module imports for uniformity throughout the codebase. Thescheduler.wait(3000)call is an appropriate replacement for a manual sleep function.🧰 Tools
🪛 eslint
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
src/index.ts (2)
1-2: Validate Node.js version requirement.
You’ve introduced imports using ES module syntax here. Since this PR officially drops support for Node.js versions earlier than 18.19.0, it would be good to confirm that users who attempt to install this package on older Node.js versions see a clear error or warning, such as adding anenginesfield in thepackage.json.
12-19: Ensure robust error handling instartCluster.
Currently, the function awaitsMaster(options).ready()but does not handle rejections or exceptions that might arise. Consider wrapping the call in a try-catch or adding an error event listener to ensure any startup errors are properly reported to end users or logs.export async function startCluster(options: ClusterOptions) { - await new Master(options).ready(); + try { + await new Master(options).ready(); + } catch (err) { + // Provide helpful diagnostics or rethrow + throw new Error(`startCluster failed: ${err.message}`); + } }test/utils.ts (1)
8-21: Consider logging or debugging messages inclusterfunction.
When debugging complex test scenarios, it can be helpful to log or handle potential exceptions frommm.cluster(...). This function is critical for test setup and may benefit from additional instrumentation (e.g., logging startup steps or hooking into the mock cluster's lifecycle).test/worker_threads.test.ts (1)
11-11: Typing clarity withas any.
While pragmatic, it’s generally better to adjust your type definitions if possible rather than falling back toany. If the type signature forclustercan be refined, please consider doing so.test/https.test.ts (1)
6-6: Secure defaults noted.
InstantiatingHttpClientwithrejectUnauthorized: falsefor test purposes is fine, but remind users this is insecure in a production environment. A comment clarifying test-only usage might help avoid confusion.const httpclient = new HttpClient({ - connect: { rejectUnauthorized: false } + // In production, set "rejectUnauthorized: true" for secure connections + connect: { rejectUnauthorized: false }, });test/agent_worker.test.ts (7)
1-6: Consider grouping similar imports.
While it's absolutely fine to have separate imports, grouping related modules (e.g., all Node.js internal modules together, followed by external libraries) can improve readability.
44-44: Avoid duplication.
This call tocluster('apps/agent-die-onboot')is repeated. Consider a small helper function to unify the fixture creation if used frequently.
Line range hint
54-65: Consider shorter waiting time.
A 5-second wait might affect test suite performance. If feasible, reduce the wait time or replace it with a more targeted event-based approach.
89-89: Clarify test name.
it('should master exit when agent exit during app worker boot')could be more descriptive, for example:'it('should exit master when agent fails during app boot')'.
103-103: Testing coverage for frameworks with extra environment variables.
If additional frameworks or environment setups affect'agent-start-error', consider adding more test coverage with variations in environment.
123-123: Consistent error scenario naming.
Align naming for'agent-start-framework-ready-error'with'agent-start-framework-error'for consistency if there's no distinct difference between them.
146-146: Avoid “magic” string references to'apps/custom-logger'.
Consider a constant or utility for test fixture paths to minimize the chance of typos.test/options.test.ts (2)
37-52: Excellent coverage for HTTPS optional properties.
Passingcaandpassphraseextends coverage of secure setups. Confirm if a negative test is needed (e.g., missingkeyorcert).
131-145: Confirm cluster usage with additional app options.
While it verifies'foo: true', consider more test scenarios for other custom options.test/app_worker.test.ts (8)
10-13: Consider a single afterEach.
There are multipleafterEachcalls that do a close plus amm.restore(). Combining them into one block can be clearer and might avoid confusion.🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 13-13: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
29-29: Confirm app code with'worker-die'.
If'worker-die'also references agent behavior, ensure it doesn’t inadvertently test agent scenarios that belong inagent_worker.test.ts.
Line range hint
54-66: Ensure consistent error messages.
CustomError: mock errorvs the commented-out line with a more detailed message. Confirm you prefer the shorter message or if the docs link is needed.
100-100: Double-check unreferenced containers.
app.notExpect('stdout', /...7002/)is good, but confirm no references remain that tie to this port for other tests.
Line range hint
109-121: Consider a smaller wait time.
A 5-second wait can slow test runs. If possible, an event-based approach (listening to'disconnect'or'exited') might be more efficient.
Line range hint
158-175: Use consistent naming for environment-based tests.
'app-kill'is tested in local env. If you have more environment-specific usage, consider a macros or param-based approach.
205-212: Validating ephemeral port usage.
A dynamic port usage approach is robust, but ensure tests don’t inadvertently depend on a fixed port if multiple tests run in parallel.
Line range hint
322-341: Long wait intervals.
Re-check the 10-second wait usage. If possible, a more direct approach (watching'disconnect'events) can reduce test time.README.md (4)
24-24: Add Node.js version requirement to installation sectionGiven the breaking change in Node.js version requirements, consider adding a note about the minimum Node.js version (18.19.0) in the installation section.
npm i @eggjs/cluster + +# Requirements +- Node.js >= 18.19.0
40-41: Fix grammar in callback descriptionThe sentences have missing articles.
-You can specify a callback that will be invoked when application has started. -However, master process will exit when catch an error. +You can specify a callback that will be invoked when the application has started. +However, the master process will exit when it catches an error.🧰 Tools
🪛 LanguageTool
[uncategorized] ~40-~40: Possible missing article found.
Context: ...fy a callback that will be invoked when application has started. However, master process wi...(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~41-~41: You might be missing the article “the” here.
Context: ... when application has started. However, master process will exit when catch an error. ...(AI_EN_LECTOR_MISSING_DETERMINER_THE)
51-58: Enhance TypeScript example with type annotationsConsider adding type annotations to make the TypeScript example more educational.
```ts -import { startCluster } from '@eggjs/cluster'; +import { startCluster, ClusterOptions } from '@eggjs/cluster'; -startCluster({ +const options: ClusterOptions = { baseDir: '/path/to/app', framework: '/path/to/framework', -}); +}; + +startCluster(options);
80-82: Improve environment variables documentationThe environment variables section could be more consistent and detailed.
-`EGG_APP_CLOSE_TIMEOUT`: app worker boot timeout value - -`EGG_AGENT_CLOSE_TIMEOUT`: agent worker boot timeout value +| Environment Variable | Description | +| ------------------- | ----------- | +| `EGG_APP_CLOSE_TIMEOUT` | Timeout value (in milliseconds) for app worker boot process | +| `EGG_AGENT_CLOSE_TIMEOUT` | Timeout value (in milliseconds) for agent worker boot process |🧰 Tools
🪛 LanguageTool
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...R_ENV | ## EnvEGG_APP_CLOSE_TIMEOUT: app worker boot timeout value `EGG_AGE...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...timeout valueEGG_AGENT_CLOSE_TIMEOUT: agent worker boot timeout value ## Lic...(UNLIKELY_OPENING_PUNCTUATION)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (59)
.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(2 hunks)index.js(0 hunks)lib/agent_worker.js(0 hunks)lib/app_worker.js(0 hunks)lib/utils/mode/base/agent.js(0 hunks)lib/utils/mode/base/app.js(0 hunks)lib/utils/mode/impl/process/agent.js(0 hunks)lib/utils/options.js(0 hunks)lib/utils/terminate.js(0 hunks)lib/utils/timer.js(0 hunks)package.json(2 hunks)src/agent_worker.ts(1 hunks)src/app_worker.ts(1 hunks)src/dirname.ts(1 hunks)src/error/ClusterAgentWorkerError.ts(1 hunks)src/error/ClusterWorkerExceptionError.ts(1 hunks)src/error/index.ts(1 hunks)src/index.ts(1 hunks)src/master.ts(19 hunks)src/utils/messenger.ts(8 hunks)src/utils/mode/base/agent.ts(1 hunks)src/utils/mode/base/app.ts(1 hunks)src/utils/mode/impl/process/agent.ts(1 hunks)src/utils/mode/impl/process/app.ts(5 hunks)src/utils/mode/impl/worker_threads/agent.ts(6 hunks)src/utils/mode/impl/worker_threads/app.ts(6 hunks)src/utils/options.ts(1 hunks)src/utils/terminate.ts(1 hunks)src/utils/worker_manager.ts(3 hunks)test/agent_worker.test.ts(10 hunks)test/app_worker.test.ts(17 hunks)test/fixtures/apps/app-listen-port/app/router.js(1 hunks)test/fixtures/apps/app-listen-port/config/config.default.js(0 hunks)test/fixtures/apps/app-listen-without-port/config/config.default.js(0 hunks)test/fixtures/apps/before-close/agent.js(1 hunks)test/fixtures/apps/before-close/app.js(1 hunks)test/fixtures/apps/framework/index.js(1 hunks)test/fixtures/apps/framework/lib/agent.js(1 hunks)test/fixtures/apps/framework/lib/framework.js(1 hunks)test/fixtures/apps/options/agent.js(0 hunks)test/fixtures/apps/options/app.js(0 hunks)test/fixtures/apps/options/config/plugin.js(1 hunks)test/fixtures/apps/script-start/start-server.js(2 hunks)test/fixtures/apps/worker-close-timeout/agent.js(1 hunks)test/fixtures/apps/worker-close-timeout/app.js(1 hunks)test/fixtures/egg/index.js(1 hunks)test/https.test.ts(2 hunks)test/master.test.ts(51 hunks)test/options.test.js(0 hunks)test/options.test.ts(1 hunks)test/utils.js(0 hunks)test/utils.ts(1 hunks)test/worker_threads.test.ts(1 hunks)tsconfig.json(1 hunks)
💤 Files with no reviewable changes (16)
- test/fixtures/apps/app-listen-port/config/config.default.js
- test/fixtures/apps/app-listen-without-port/config/config.default.js
- test/fixtures/apps/options/app.js
- .github/PULL_REQUEST_TEMPLATE.md
- lib/utils/timer.js
- index.js
- test/fixtures/apps/options/agent.js
- test/utils.js
- lib/utils/mode/base/agent.js
- lib/utils/terminate.js
- lib/utils/mode/impl/process/agent.js
- lib/utils/options.js
- test/options.test.js
- lib/app_worker.js
- lib/agent_worker.js
- lib/utils/mode/base/app.js
✅ Files skipped from review due to trivial changes (4)
- .gitignore
- test/fixtures/apps/options/config/plugin.js
- tsconfig.json
- src/error/index.ts
🧰 Additional context used
🪛 eslint
test/fixtures/apps/framework/lib/agent.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/framework/index.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/worker-close-timeout/agent.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/script-start/start-server.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/worker-close-timeout/app.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/before-close/agent.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
test/fixtures/apps/before-close/app.js
[error] 1-1: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🪛 Biome (1.9.4)
test/worker_threads.test.ts
[error] 8-8: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/https.test.ts
[error] 13-13: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/app_worker.test.ts
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 13-13: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 203-203: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
src/utils/mode/impl/process/agent.ts
[error] 54-54: 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] 55-55: 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)
src/app_worker.ts
[error] 82-82: 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)
src/utils/mode/impl/worker_threads/agent.ts
[error] 52-52: 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)
src/master.ts
[error] 279-279: isNaN is unsafe. It attempts a type coercion. Use Number.isNaN instead.
See the MDN documentation for more details.
Unsafe fix: Use Number.isNaN instead.
(lint/suspicious/noGlobalIsNan)
test/agent_worker.test.ts
[error] 14-14: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 LanguageTool
README.md
[uncategorized] ~40-~40: Possible missing article found.
Context: ...fy a callback that will be invoked when application has started. However, master process wi...
(AI_HYDRA_LEO_MISSING_THE)
[uncategorized] ~41-~41: You might be missing the article “the” here.
Context: ... when application has started. However, master process will exit when catch an error. ...
(AI_EN_LECTOR_MISSING_DETERMINER_THE)
[uncategorized] ~80-~80: Loose punctuation mark.
Context: ...R_ENV | ## Env EGG_APP_CLOSE_TIMEOUT: app worker boot timeout value `EGG_AGE...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~82-~82: Loose punctuation mark.
Context: ...timeout value EGG_AGENT_CLOSE_TIMEOUT: agent worker boot timeout value ## Lic...
(UNLIKELY_OPENING_PUNCTUATION)
🔇 Additional comments (121)
test/fixtures/apps/before-close/app.js (1)
6-6: Use of scheduler.wait(10) is approved.
Replacing custom sleep calls with the built-in promise-based timer is a clean approach, and the syntax here looks correct.
test/fixtures/apps/before-close/agent.js (1)
6-6: Usage of scheduler.wait(10) is valid.
This is consistent with the shift away from custom sleep utilities toward Node's built-in promise-based timers.
test/fixtures/apps/worker-close-timeout/agent.js (1)
8-8: scheduler.wait(timeout * 2) usage looks good.
This ensures consistent code style across the application for asynchronous waits.
test/fixtures/apps/worker-close-timeout/app.js (1)
8-8: Asynchronous wait logic is straightforward.
Implementation of await scheduler.wait(timeout * 2) adheres to the updated approach for introducing delays in Node.js.
src/utils/worker_manager.ts (9)
1-3: Imports look good for ES modules
These import statements correctly adopt the new ES module syntax and seem consistent with the rest of the codebase.
7-12: Initialization of properties
Defining agent, workers, exception, and timer here ensures the fields are clear and typed. Confirm that timer is always cleared (e.g., in an exit or tear-down phase) to avoid potential memory leaks.
22-22: Explicitly typed setter for agent
This setter method looks good and clear. The parameter type ensures consistency.
34-34: Registering a worker in the Map
Storing each worker by its workerId in a Map structure is a clean approach.
38-38: Typed retrieval of a worker
Retrieving the worker via workerId: number ensures clarity and correctness.
42-42: Clean removal of a worker
deleteWorker implementation is straightforward and follows the same typed approach.
50-52: Listing workers
Returning all worker instances in an array is neat and straightforward. This provides a convenient overview of the active workers.
56-57: Verifying worker state
Ensure that each worker indeed has a state property set to 'listening' or other relevant statuses. You might want to guard against undefined states or unexpected states.
66-66: Optional chaining for agent status
Using optional chaining for agent?.status is succinct and prevents null/undefined errors. Looks good.
src/agent_worker.ts (9)
1-9: Imports and debug usage
These imports look appropriate for logging, utilities, and cross-file references. The debug utility helps track the agent worker lifecycle.
10-16: Documentation clarity
The JSDoc block explains the agent worker lifecycle well, covering exit scenarios. Good job clarifying the reason for exit codes.
17-30: Parsing options and loading modules
Destructuring framework, require, and startMode from the parsed process.argv object is neat. The dynamic require for each module in options.require is also clearly handled.
31-37: Worker selection logic
Selecting AgentThreadWorker or AgentProcessWorker based on startMode is straightforward. Ensure the fallback (else) is deliberate (i.e., defaults to process mode if startMode is undefined).
38-47: Agent creation and error handling
Creating a ConsoleLogger and importing Agent from the user-specified framework is a clean approach. The try...catch block handles initialization errors gracefully by logging and rethrowing the error.
49-54: startErrorHandler
This function logs the error details and then instructs AgentWorker to kill the process, ensuring the system exits. Well-defined approach for handling startup failures.
55-67: Agent readiness check
agent.ready is used to send a message upon successful startup. If startup fails, it is handled by the startErrorHandler. The logic is clear and ensures the master is informed.
68-73: Graceful exit configuration
Attaching a gracefulExit call ensures the agent closes resources before the thread or process exits. This approach is consistent with typical Egg.js worker lifecycle patterns.
75-75: Entry point invocation
Invoking main() at the end makes sense to start the agent worker logic.
src/utils/mode/base/agent.ts (3)
1-9: Imports and type definitions
Pulling in path, EventEmitter, and the various type definitions ensures the base agent classes have minimal yet sufficient dependencies.
10-52: BaseAgentWorker abstract class
This class defines a strong contract for agent worker functionality:
- Private fields
#instanceIdand#instanceStatusenforce encapsulation. - The abstract
get workerId()property ensures subclasses implement a unique ID. staticmethods (send,kill,gracefulExit) throwing errors remind implementers to override them.
56-90: BaseAgentUtils abstract class
This class extends EventEmitter, clarifying how to handle start times, logging, and messenger communication. The abstract kill enforces that child classes provide a termination implementation.
src/utils/mode/impl/worker_threads/agent.ts (10)
1-5: Imports for worker threads and graceful exit
Importing workerThreads and Worker from the Node.js standard library and referencing graceful-process for exit options is consistent for a worker-based approach.
7-10: AgentThreadWorker extends BaseAgentWorker
Good usage of generics to specify the Worker type, making it explicit that we’re dealing with worker threads.
12-13: send(message: MessageBody)
Forwarding messages with postMessage aligns with the worker_threads API. Straightforward and typed.
16-18: Static send approach
Adding senderWorkerId to the message for better traceability is a nice touch. Consider verifying that all recipients handle this additional property properly.
27-27: static kill()
Exiting only the current thread is a valid approach in worker_threads. This won’t necessarily shut down the main process, so verify that this is the intended scope.
38-39: AgentThreadUtils introduction
This utility class inherits from BaseAgentUtils. Using a private #worker reference to manage the single worker thread is a tidy design.
41-41: Explicit instance property
Storing the new AgentThreadWorker instance is clear. It helps maintain references for log messages and watchers.
71-71: Custom error logging
Wrapping errors with ClusterAgentWorkerError provides helpful context. This design is beneficial for debugging complex cluster failures.
75-75: Handling exit event
Emitting agent-exit to the master process ensures there’s a definitive communication about worker thread termination.
93-93: Terminating the thread
worker.terminate() is the canonical approach in worker_threads to gracefully shut down the thread. This method is consistent with the kill pattern.
src/utils/terminate.ts (7)
1-7: Utility imports
Using debuglog, sleep, once, and @fengmk2/ps-tree suggests a robust approach for terminating processes and sub-processes.
9-12: SubProcess interface
Extending ChildProcess with an optional process? property helps unify logic between direct ChildProcess references and possibly nested structures.
22-37: killProcess
Terminating the main process with SIGTERM then escalating to SIGKILL upon timeout is a standard approach. The process.on('exit', ...) references in some OS environments might not always fire if SIGKILL is used, but this is typical and expected.
39-59: killChildren
This function attempts a graceful shutdown of child processes first (SIGTERM) and then escalates to SIGKILL. Using a loop with a checkInterval is an effective design.
61-71: getChildPids
Using pstree to gather child processes is a well-established but system-dependent approach. The fallback to ignoring errors ensures a best-effort kill approach.
73-82: kill(pids: number[], signal: string)
Looping through each PID and applying the specified signal is straightforward. Logging errors and ignoring them ensures the process termination flow can continue.
84-96: getUnterminatedProcesses
Attempting process.kill(pid, 0) is a classic trick to check if a process is still alive. This function is succinct and works well for verifying which child processes remain.
src/utils/messenger.ts (1)
100-102: Validate additional message routing logic.
When determining the message destination by receiverWorkerId or receiverPid, confirm that the underlying worker references are valid. It might be prudent to log or handle when the worker does not match any known ID.
src/utils/options.ts (5)
1-6: Imports look good.
No issues found with import statements.
8-13: HTTPS Options Interface
The interface ClusterHTTPSSecureOptions is well-defined and aligns with Node.js TLS context options.
15-15: ClusterStartMode Type
Defining 'process' | 'worker_threads' for the cluster startup mode is clear and future-proof for the current use case.
18-80: ClusterOptions Interface
The interface is comprehensive and well-documented. The deprecated fields for HTTPS config ensure backward compatibility.
82-88: ParsedClusterOptions Interface
This interface correctly extends ClusterOptions to mark certain fields mandatory and typed. Looks good.
test/master.test.ts (1)
Line range hint 1-994: Extensive test coverage and environment handling look good.
No major issues identified. The test suite is robust and covers a variety of signals, environment variables, and cluster behaviors.
test/fixtures/apps/app-listen-port/app/router.js (1)
7-7: Accessing ctx.app.options.port
Switching from a private _options field to a public options field is a positive improvement for maintainability.
test/fixtures/apps/framework/index.js (1)
3-5: Named exports look good
These new named exports (startCluster, Application, Agent) maintain clarity and support the PR goal of bridging CJS and ESM. This is a valid approach for gradual migration.
test/fixtures/egg/index.js (1)
4-6: Named exports are well-structured
Exporting startCluster, Application, and Agent this way cleanly separates concerns. This helps clarify which entities the module provides.
src/dirname.ts (2)
1-2: Excellent usage of Node.js native imports
These top-level imports from node:url and node:path are clear and concise, matching the ESM style.
4-11: Good fallback logic for directory resolution
Using __dirname when available and falling back to fileURLToPath(import.meta.url) is a robust way to handle both CommonJS and ES modules. This approach aligns well with Node.js 18+ features.
src/error/ClusterWorkerExceptionError.ts (1)
1-17: Solid implementation of a custom error class.
The usage of Error.captureStackTrace is beneficial for debugging, and the provided message clearly indicates the condition that led to this exception. No immediate critical issues identified.
test/fixtures/apps/framework/lib/framework.js (1)
7-9: Ensure callers handle the new async behavior.
Changing loadConfig to an async method is a breaking change for any upstream callers expecting a synchronous operation. Verify that all call sites handle the returned promise and potential errors appropriately (e.g., adding try/catch if necessary).
src/error/ClusterAgentWorkerError.ts (1)
1-19: Custom error creation looks appropriate.
Passing the original error as a cause preserves the error context, and storing id, workerId, and status in a single object ensures clarity for logging and debugging. No immediate issues identified.
src/index.ts (3)
4-10: Documentation is concise and helpful.
The added block comment describing the cluster start flow is clear and effectively captures the main steps. This is an excellent practice for increasing code maintainability and clarity. Keep it up!
21-24: Export statements are well-structured.
Exporting all options from a single location reinforces good project organization and discoverability. Nicely done.
26-26: Kudos for grouping errors in a dedicated directory.
This explicit export from ./error/index.js helps streamline error management. It’s a good pattern when scaling a codebase.
test/utils.ts (3)
1-3: ES Modules usage.
Importing path and fileURLToPath from Node.js built-ins with ESM syntax looks good. No issues here.
5-7: Maintaining clarity with dynamic paths.
Defining __filename and __dirname in ESM ensures file references remain flexible. This is essential for cross-platform support.
23-25: An elegant getFilepath utility.
This function neatly centralizes test fixture resolution. Straightforward and helpful for test maintainability.
test/worker_threads.test.ts (3)
1-2: Great shift toward ES modules.
Using named imports from @eggjs/mock aligns with the ESM refactoring across the codebase.
4-5: Descriptive test suite name.
Renaming to test/worker_threads.test.ts clarifies the corresponding file context, which is beneficial when debugging test failures.
19-19: Proper error handling test.
It’s good to confirm the exit code and stderr messages in negative or error-based test scenarios. Thorough coverage fosters robust stability.
test/https.test.ts (10)
1-4: Switching to ES modules smoothly.
The new import statements for assert, mm, HttpClient, and local utilities are consistent with the updated ESM patterns.
8-9: Descriptive test block.
The updated describe text referencing test/https.test.ts helps contextualize test logs.
16-16: Avoid potential port conflicts in CI.
Using a static port like 8443 in tests sometimes conflicts with other local processes or parallel CI runs. Consider a random ephemeral port approach if collisions become an issue.
21-23: Transparent fixture references.
Providing explicit file paths for certificates is a neat approach, making these lines easy to maintain or update.
26-26: Effective cluster usage.
Invoking cluster with appropriate options is consistent and straightforward here.
38-38: Promoting code reuse.
Reusing the same fixture folder apps/https-server in repeated tests is a good approach for consistent coverage, especially since you test multiple scenarios (HTTPS only vs. HTTPS + HTTP).
44-46: Neat SSL config.
Storing key, cert, and CA in test fixtures is standard practice. This is a well-structured approach for integration tests requiring SSL.
49-49: Robust readiness check.
Waiting for app.ready() ensures the server is fully up before making requests. This pattern improves test consistency.
70-70: Named fixture references.
Using apps/https-server-config plus the new getFilepath approach is consistent and eliminates guesswork in test setup.
76-76: Parallel with the prior pattern.
Again, waiting for app.ready() ensures a stable test environment before making assertions. This is an excellent habit.
test/agent_worker.test.ts (8)
8-11: Validate test setup.
The mm.restore() call in afterEach is good practice for resetting mocks. Ensure that all test-specific mock side effects are cleaned up here.
18-21: Confirm environment variable usage.
Setting EGG_AGENT_DEBUG_PORT to 15800 is a good test scenario. Please ensure other references to the agent debug port are also tested (e.g., if set to a non-default).
26-29: Ensure test isolation.
When reusing the 'apps/agent-debug-port' test fixture, double-check that no leftover side effects from the previous test (it('support config agent debug port')) could interfere.
34-34: Check for potential concurrency issues.
Keep in mind that if other processes or tests use the 'apps/agent-die-onboot' fixture at the same time, it might introduce unexpected test flakiness.
74-82: Verify signals in test environment.
Using SIGKILL for tests on certain operating systems might behave differently. Ensure consistent test results across platforms.
114-114: Confirm error message clarity.
agent-start-framework-error includes a link in the error message. Ensure it's properly validated or replaced if future docs differ.
133-137: Validate process management in spawned tests.
Ensure that all spawned processes (coffee.spawn) are correctly terminated to avoid potential resource leaks.
152-154: Check file reading errors.
If logs/monitor.log doesn’t exist, the test might fail. Ensure failIfNoLog is the intended behavior.
test/options.test.ts (15)
1-9: Imports are well-organized.
Using node:-prefixed modules clarifies these are Node.js built-ins, which is a good modern practice.
10-12: File path initialization.
__filename and __dirname are used effectively for dynamic path resolution. Great job ensuring portability of tests.
16-21: Validate default port scenario.
These tests confirm zero input or null/undefined leads to no port assignment. Make sure there's a parallel test ensuring that an actual default port is never assigned inadvertently.
23-35: Check HTTPS config validations.
Tests confirm 8443 is used when https is provided. Also ensure error handling is tested if any of key or cert is invalid.
54-59: Enforce numeric port.
The code converts '6001' to number 6001. If a user passes a non-numeric string, is it handled gracefully or with an explicit error?
61-72: Environment-specific variables.
NO_DEPRECATION is set in production. That’s good, but confirm no other environment is unintentionally overwritten.
74-85: Edge cases for null/undefined ports
Tests handle null, undefined, and no port specified. Looks robust.
87-93: Confirm os.cpus usage.
The test ensures we don't call os.cpus when workers is manually specified. Good way to avoid CPU count overhead.
95-111: Debug flag checks.
All the debug usage scenarios (--inspect, --debug) are tested. Good coverage.
113-129: Environment variable defaults.
env logic is tested thoroughly, checking fallback to process.env.EGG_SERVER_ENV. Great coverage.
147-166: Framework path resolution tests.
Multiple robust scenarios for path existence and non-existence. Great approach verifying real vs. non-real framework paths.
168-186: NPM package resolution tests.
It’s good that you check both successful resolution and error scenario for missing packages.
188-194: Framework from pkg.egg.framework.
Verifying the rename logic inside package.json ensures you can override Egg with a custom framework. This is well done.
196-210: Thorough fallback for multiple node_modules paths.
This ensures error message clarity when frameworks aren't found in multiple fallback locations. Good job!
212-218: Default to 'egg'.
Proper test coverage for the normal case. This final check ensures a frictionless user experience.
test/app_worker.test.ts (14)
1-9: Implementation for TypeScript test migration.
Your updated imports and typed MockApplication usage are a welcome improvement for clarity and type safety.
Line range hint 17-21: Synchronous vs asynchronous resources.
When using .ready(), ensure no leftover asynchronous tasks are still pending. Occasionally, concurrency issues appear during teardown.
🧰 Tools
🪛 Biome (1.9.4)
[error] 12-12: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
[error] 13-13: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
38-38: Optional environment check for 'app-start-error'.
You set EGG_APP_WORKER_LOGGER_LEVEL. Check if other sub-environments might conflict with this variable.
Line range hint 71-83: Verify error hooking.
You test '...framework-ready-error' for correct error messaging. This is good coverage; just confirm consistency with '...framework-error'.
88-88: Ensure concurrency control.
As your test spawns or kills processes, confirm that parallel test runs won’t produce collisions.
131-138: Double teardown calls.
You call app.close() in after(), and then also do a restore. Ensure the order is correct to avoid any leftover references or mocks.
140-151: Properly testing kill signals.
Killing the worker might have OS-specific quirks. Glad to see it tested.
Line range hint 182-193: Timeout logic.
Testing 'app-start-timeout' is crucial. Ensure you also handle partial initialization states gracefully.
195-202: File system concurrency.
rm(sockFile, { force: true, recursive: true }) is helpful, but be aware of possible race conditions if parallel tests also manipulate that file.
Line range hint 216-242: Impressive coverage for custom port usage.
Ensuring you can read 'http://127.0.0.1:17010' and 'http://localhost:17010' covers typical real-world usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 203-203: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
Line range hint 255-274: Restricting hostname usage.
You bind specifically to the local IP address. Confirm what happens if ip() returns IPv6 by default or a different interface.
Line range hint 280-288: Unix domain sockets.
app-listen-path tests usage of domain sockets. Great coverage for non-port-based listening.
Line range hint 298-312: Port conflict tests.
EADDRINUSE scenario is well-tested. Good job verifying the error message is captured.
Line range hint 347-351: Graceful fail scenario.
Test verifies 'app-start-error' gracefully fails with 'don't fork'. Good coverage.
.eslintrc (1)
2-5: Great transition to TypeScript linting rules.
Extending from "eslint-config-egg/typescript" ensures best practices. Also, "eslint-config-egg/lib/rules/enforce-node-prefix" helps maintain consistency with Node.js imports.
.github/workflows/nodejs.yml (1)
15-15: LGTM! Node.js version matrix aligns with new requirements.
The updated version matrix correctly:
- Sets minimum version to 18.19.0
- Removes unsupported versions (14, 16)
- Includes latest LTS and current versions
Let's verify the Node.js version requirements are consistent across the codebase:
✅ Verification successful
Node.js version requirements are consistent across the codebase
The Node.js version matrix in the GitHub workflow (18.19.0, 18, 20, 22, 23) correctly aligns with the minimum version requirement specified in package.json (>= 18.19.0). The configuration ensures proper testing across supported Node.js versions while maintaining compatibility with the project's requirements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check Node.js version requirements in package.json and other config files
echo "Checking Node.js version requirements..."
rg -l "(?i)node.*(?:>=|>|version)" --type yaml --type json
Length of output: 164
Script:
#!/bin/bash
# Check Node.js version requirements in package.json
echo "Content of package.json:"
cat package.json | jq -r '.engines.node'
Length of output: 110
.github/workflows/pkg.pr.new.yml (1)
20-21: Verify dual module build process.
The prepublishOnly script runs tshy for dual CJS/ESM builds. Let's verify the build outputs and module resolution.
✅ Verification successful
Let me gather more information about the package.json configuration to verify the dual module build setup.
Dual module build process is properly configured
The package.json confirms proper dual CJS/ESM build setup:
- Uses tshy for building both formats (via
prepublishOnlyscript) - Has correct
exportsfield with bothimportandrequireconditions - Includes type declarations for both formats
- Properly configures entry points:
- CJS:
mainfield points to./dist/commonjs/index.js - ESM:
modulefield points to./dist/esm/index.js - Types:
typesfield points to./dist/commonjs/index.d.ts
- CJS:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify tshy configuration and build outputs
echo "Checking tshy configuration..."
rg -l "tshy" --type json
echo "Checking for dual module configuration..."
rg -l "\"exports\".*:.*{.*\"import\".*\"require\"" --type json
echo "Checking for type declarations..."
rg -l "\"types\".*:.*\.(d\.ts|ts)" --type json
Length of output: 531
Script:
#!/bin/bash
# Check package.json contents for build configuration
echo "Package.json contents:"
cat package.json
echo -e "\nChecking for build scripts:"
rg "\"scripts\"" -A 10 package.json
echo -e "\nChecking for tshy configuration files:"
fd -e json tshy
Length of output: 2907
package.json (3)
2-3: Breaking changes are properly reflected in version bump.
The package rename and major version bump to 3.0.0-beta.4 correctly indicate breaking changes:
- Package renamed to @eggjs/cluster
- Node.js >= 18.19.0 requirement
- Dual CJS/ESM module support
79-91: LGTM! Dual module support is properly configured.
The exports field correctly defines:
- ESM entry points with TypeScript declarations
- CJS entry points with TypeScript declarations
- Package.json subpath export
73-78: LGTM! tshy configuration is correct.
The tshy configuration properly defines the source entry points for building both CJS and ESM modules.
README.md (2)
4-4: LGTM: Badge and link updates are accurate
The badges and URLs have been correctly updated to reflect the new package name @eggjs/cluster. The addition of the Node.js version badge is particularly helpful given the new version requirement.
Also applies to: 8-8, 10-17
90-90: LGTM: Contributors badge updated correctly
The contributors badge has been properly updated to point to the new repository location.
| export class AgentProcessWorker extends BaseAgentWorker<ChildProcess> { | ||
| get workerId() { | ||
| return this.instance.pid!; | ||
| } | ||
|
|
||
| send(message: MessageBody) { | ||
| sendmessage(this.instance, message); | ||
| } | ||
|
|
||
| static send(message: MessageBody) { | ||
| message.senderWorkerId = String(process.pid); | ||
| process.send!(message); | ||
| } | ||
|
|
||
| static kill() { | ||
| process.exitCode = 1; | ||
| process.kill(process.pid); | ||
| } | ||
|
|
||
| static gracefulExit(options: gracefulExitOptions) { | ||
| gracefulExit(options); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Adapt or confirm error-handling strategy.
The worker’s static kill() method sets process.exitCode = 1 immediately, which can abruptly stop the process without graceful cleanup. Consider providing a more controlled shutdown sequence or a cleanup routine preceding process.kill(process.pid).
| const agentProcess = this.#agentProcess = fork(this.getAgentWorkerFile(), args, forkOptions); | ||
| const agentWorker = this.instance = new AgentProcessWorker(agentProcess); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate assignment in expressions for clarity.
Use separate statements to improve readability and avoid confusion.
- const agentProcess = this.#agentProcess = fork(this.getAgentWorkerFile(), args, forkOptions);
- const agentWorker = this.instance = new AgentProcessWorker(agentProcess);
+ this.#agentProcess = fork(this.getAgentWorkerFile(), args, forkOptions);
+ const agentProcess = this.#agentProcess;
+ const agentWorker = new AgentProcessWorker(agentProcess);
+ this.instance = agentWorker;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const agentProcess = this.#agentProcess = fork(this.getAgentWorkerFile(), args, forkOptions); | |
| const agentWorker = this.instance = new AgentProcessWorker(agentProcess); | |
| this.#agentProcess = fork(this.getAgentWorkerFile(), args, forkOptions); | |
| const agentProcess = this.#agentProcess; | |
| const agentWorker = new AgentProcessWorker(agentProcess); | |
| this.instance = agentWorker; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 54-54: 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] 55-55: 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)
| name: Publish Any Commit | ||
| on: [push, pull_request] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider restricting publish workflow triggers.
The workflow currently runs on all pushes and pull requests. Consider restricting it to specific branches or tags to prevent unintended publications.
-on: [push, pull_request]
+on:
+ push:
+ branches: [ master ]
+ tags: [ 'v*' ]📝 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.
| name: Publish Any Commit | |
| on: [push, pull_request] | |
| name: Publish Any Commit | |
| on: | |
| push: | |
| branches: [ master ] | |
| tags: [ 'v*' ] |
| "dependencies": { | ||
| "await-event": "^2.1.0", | ||
| "cfork": "^1.7.1", | ||
| "cluster-reload": "^1.0.2", | ||
| "@eggjs/utils": "^4.1.6", | ||
| "@fengmk2/ps-tree": "^2.0.1", | ||
| "cfork": "^2.0.0", | ||
| "cluster-reload": "^2.0.0", | ||
| "detect-port": "^2.0.1", | ||
| "egg-logger": "^3.3.0", | ||
| "egg-utils": "^2.4.1", | ||
| "get-ready": "^2.0.1", | ||
| "graceful-process": "^1.2.0", | ||
| "is-type-of": "^1.2.1", | ||
| "ps-tree": "^1.2.0", | ||
| "sendmessage": "^1.1.0", | ||
| "egg-logger": "^3.6.0", | ||
| "get-ready": "^3.2.0", | ||
| "graceful-process": "^2.0.0", | ||
| "sendmessage": "^3.0.1", | ||
| "terminal-link": "^2.1.1", | ||
| "utility": "^1.15.0" | ||
| "utility": "^2.2.0" |
There was a problem hiding this comment.
💡 Codebase verification
Some dependencies need updates for full dual module and Node.js compatibility
Based on the npm registry data, there are a few compatibility concerns:
-
egg-logger(v3.6.0):- Only specifies Node.js >= 14.17.0
- No exports field, indicating no explicit dual module support
-
terminal-link(v2.1.1):- Only specifies Node.js >= 12
- Has a basic exports field but not properly configured for dual modules
-
get-ready(v3.2.0) andutility(v2.2.0):- Specify Node.js >= 16.x
- While they have dual module support, they don't meet the Node.js >= 18.19.0 requirement
The remaining dependencies (@eggjs/utils, @fengmk2/ps-tree, cfork, cluster-reload, detect-port, graceful-process, sendmessage) all have proper dual module support and Node.js >= 18.19.0 compatibility.
🔗 Analysis chain
Verify dependency compatibility with dual modules.
Let's verify that all dependencies support both CJS and ESM modules and are compatible with Node.js >= 18.19.0.
The previous script didn't work because the node_modules directory is not available in the sandbox environment. Let's try a different approach by checking the packages directly from npm registry.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check dependencies for dual module support and Node.js compatibility
echo "Checking dependencies..."
deps=$(jq -r '.dependencies | keys[]' package.json)
for dep in $deps; do
echo "Checking $dep..."
# Check if package.json exists in node_modules
if [ -f "node_modules/$dep/package.json" ]; then
# Check for exports field (dual module support)
jq '.exports' "node_modules/$dep/package.json"
# Check engines field (Node.js compatibility)
jq '.engines' "node_modules/$dep/package.json"
fi
done
Length of output: 1655
Script:
#!/bin/bash
# Check dependencies from npm registry
deps=(
"@eggjs/utils"
"@fengmk2/ps-tree"
"cfork"
"cluster-reload"
"detect-port"
"egg-logger"
"get-ready"
"graceful-process"
"sendmessage"
"terminal-link"
"utility"
)
for dep in "${deps[@]}"; do
echo "Checking $dep..."
# Get the latest version info from npm
npm view "$dep" exports engines --json 2>/dev/null || echo "Package not found or error"
echo "---"
done
Length of output: 4920
[skip ci] ## [3.0.0](v2.4.0...v3.0.0) (2024-12-28) ### ⚠ BREAKING CHANGES * drop Node.js < 18.19.0 support part of eggjs/egg#3644 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes for @eggjs/cluster v3.0.0-beta.4 - **New Features** - Migrated project to TypeScript. - Added support for Node.js 18.19.0, 20, 22, and 23. - Enhanced type safety and module exports. - Improved worker thread and process management. - Introduced new error handling classes for better debugging. - **Breaking Changes** - Renamed package from `egg-cluster` to `@eggjs/cluster`. - Updated import/export syntax to ES modules. - Minimum Node.js version is now 18.19.0. - **Performance Improvements** - Refactored cluster and worker management. - Optimized error handling and logging. - **Bug Fixes** - Resolved various edge cases in worker initialization. - Improved graceful shutdown mechanisms. - **Documentation** - Updated README with new package name and usage examples. - Added TypeScript and ESM import examples. <!-- end of auto-generated comment: release notes by coderabbit.ai --> ### Features * support cjs and esm both by tshy ([#117](#117)) ([e15a4bf](e15a4bf))
BREAKING CHANGE: drop Node.js < 18.19.0 support
part of eggjs/egg#3644
Summary by CodeRabbit
Release Notes for @eggjs/cluster v3.0.0-beta.4
New Features
Breaking Changes
egg-clusterto@eggjs/cluster.Performance Improvements
Bug Fixes
Documentation