refactor: convert more tests to typescript #5379
Conversation
WalkthroughThis pull request introduces significant updates to the Egg.js framework, focusing on modernizing the codebase, improving type safety, and enhancing error handling. The changes span multiple areas including context handling, dependency management, logging, and testing infrastructure. Key modifications include transitioning from CommonJS to ES modules, updating type definitions, refactoring context and HTTP client implementations, and introducing new error classes for more precise error tracking. Changes
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (35)
test/fixtures/apps/logger-level-debug/app/router.js (1)
7-7: Consider using named constants or configuration for wait durations.Hardcoding the
1000milliseconds could lead to less flexible code. You may extract it into a constant or config prop for easier tuning.test/utils.ts (1)
93-93: Use a descriptive variable or comment for clarity.At a glance,
scheduler.wait(10000)might not be immediately obvious. Consider adding a brief comment indicating it’s a 10-second timeout for the/timeoutpath.test/fixtures/apps/demo/config/config.default.js (1)
22-29: Consider revisiting the logging configuration for debugging purposes.Disabling the console output (
consoleLevel: 'NONE') and ignoring certain errors (concentrateError: 'ignore') might complicate debugging. If this is purely for local or test usage, it should be fine. Otherwise, consider enabling at least some console output for easier troubleshooting.test/fixtures/apps/base-context-class/app/controller/home.js (1)
3-4: Avoid duplicating the context body between service and controller.Here, the controller sets
this.ctx.bodyto'hello'right after the service method does the same thing. If the intent is for the controller to override or confirm the service outcome, consider clarifying the flow or removing the redundant assignment.async show() { await this.service.home.show(); - this.ctx.body = 'hello'; + // Remove if the service is already setting the response. this.logger.debug('debug'); this.logger.info('appname: %s', this.config.name); this.logger.warn('warn'); this.logger.error(new Error('some error')); }src/lib/core/messenger/base.ts (2)
5-7: Enhance type-safety of theeggproperty.
Currently, theeggproperty is typed asEggApplicationCore, which is sufficiently broad yet offers less clarity about which fields are accessible. Consider narrowing or extending this type definition if your application or plugin structure requires deeper type checks.
17-29: Ensure errors are propagated if needed.
Currently, when an error occurs, it is logged, andemitreturnshasListeners. If you need known failures to bubble up, consider re-throwing or dispatching specialized events.test/lib/egg.test.ts (3)
25-45: Well-structured coverage fordumpConfig().
This test suite is thorough: verifying config files, dynamic config, and application info. Consider adding negative test cases that check for misconfiguration or partial writes.
127-136: Improved error simulation with mocks.
By mockingfs.writeFileSyncand asserting logfile warnings, you ensure the application handles I/O errors gracefully. Consider rewriting the callback-based test (done) into an async/await style for a consistent test approach.
366-408: Parallel unhandled rejection calls.
The testing approach for unhandled rejections (lines 380–395) is solid. If concurrency is a concern, consider verifying that multiple unhandled rejections triggered around the same time are each logged as expected, possibly with unique messages or correlation IDs.test/fixtures/apps/agent-throw/agent.js (1)
6-7: Async error handling in agent.
Defining a dedicated event for async errors is helpful for distinguishing log messages and debugging. To ensure consistent log formats, confirm that your logger or monitoring tooling can parse both sync and async error events.src/lib/error/MessageUnhandledRejectionError.ts (2)
1-4: Consider providing a generic type forargsinstead ofany[].Currently,
argsis typed asany[], which can lead to reduced type safety. If possible, refining the type could help catch potential errors at compile time.
7-11: Usepublicclass fields for clarity when using TypeScript.When refactoring to TypeScript, consider declaring these class fields (
eventandargs) explicitly aspublic, which is more descriptive and consistent with best practices.test/fixtures/apps/httpclient-next-overwrite/app.js (1)
18-18: Consolidating HttpClient references could simplify the codebase.Assigning
CustomHttpClientto bothapp.HttpClientandapp.HttpClientNextis a clean approach to maintain compatibility; however, make sure there is no conflicting behavior between different modules expecting distinct clients. If separate functionalities or configurations are needed in the future, you may want to keep them separate.src/lib/core/base_context_class.ts (1)
12-12: Consider the broader implications of makingpathNamea public declaration.By switching from a protected property to a
declareproperty,pathNamecan be externally accessible despite it not being formally initialized here. If there’s a desire to limit its usage or control mutations, consider an accessor or keep itprotected. Otherwise, ensuring its usage is well-documented and consistent across the codebase would be beneficial.src/app/extend/context.types.ts (1)
20-21: Optional: Add comments or JSDoc describing these new getters.The new logger getters add valuable functionality, but it may help future developers if you add brief comments indicating their intended use cases and any noteworthy side effects.
test/lib/core/custom_loader.test.ts (1)
7-7: Use typed variable declarations consistently.Declaring
appasMockApplicationis clear and aligns with the typed approach. Ensure this pattern is used consistently across all test files.test/lib/core/context_performance_starttime.test.ts (3)
4-6: Descriptive test naming is helpful for maintainability.The suite name conveys its purpose well: to validate
context_performance_starttime. Consider adding a brief comment or docstring for any unusual test logic here.
12-17: Context property validation is straightforward and effective.This test confirms
performanceStarttimeis properly set to a positive number, which is excellent. You might also consider edge cases or verifying updates toperformanceStarttimeif it changes under certain conditions.
19-24: Verifying controller usage ofperformanceStarttime.The test covers the controller’s handling of the performance timer. If there’s any scenario that modifies
performanceStarttimemid-execution, consider adding a test to ensure it remains accurate.test/lib/core/context_httpclient.test.ts (2)
9-13: Consolidate or rename duplicate “before” hooks
Static analysis flags these as duplicate test hooks. Consider merging them into a singlebeforeor renaming them (e.g.,beforeAll) to avoid confusion and comply with lint rules.before(async () => { - app = createApp('apps/context_httpclient'); - return app.ready(); + app = createApp('apps/context_httpclient'); + await app.ready(); -}); -before(async () => { + url = await startLocalServer(); });
20-20: Type casting usage
Castinghttpclienttoanybypasses strict typing. If possible, define the proper interface to preserve type safety.test/fixtures/apps/httpclient-tracer/app.js (1)
18-26: Centralize URL definitions
Line 18 loads the URL from an environment variable, while line 26 hardcodes the same URL. Consider reusing the variable for consistency and to ease maintenance.- res = await httpclient.request('https://registry.npmmirror.com', { + res = await httpclient.request(url, { method: 'GET', timeout: 20000, });test/lib/core/cookies.test.ts (1)
54-54: Casting configuration to any
In TS, you’re using{ encrypt: true } as any. Consider a more precise interface or type to avoid losing type safety.- } as any + } as { encrypt: boolean }test/lib/core/dnscache_httpclient.test.ts (2)
1-6: Consider removing or uncommenting unused imports for clarity.
All import statements here are commented out. If they’re not needed, consider removing them to keep the file tidy. Otherwise, uncomment them when you begin implementing these tests.
213-226: Ensure the “disableDNSCache” option’s coverage.
This test block confirms that the DNS cache is bypassed during requests. However, consider adding an assertion to confirm normal caching still works outside of "disableDNSCache" usage.test/lib/core/logger.test.ts (2)
22-23: Adjust environment manipulation in tests with caution.
Forcing environment variables (e.g.,EGG_LOG,HOME) can affect subsequent tests. Ensure each test suite fully restores them to avoid side effects.
101-115: Validate timely log flushing in non-buffered mode.
You’re already checking the log content after a wait period. Consider verifying the minimal required flush time to speed up test execution if possible.test/lib/core/httpclient.test.ts (3)
49-52: Encapsulate repeated error mocking logic.
Repeatedly mockingHttpClient.prototype.requestcould be refactored into reusable utility code for easier maintenance and clarity in other tests.
103-114: Avoid console.debug statements in tests.
Line 108 logsconsole.log(response). This can clutter test output. Remove or replace with a relevant assertion if needed.
208-223: Keep an eye on partial environment overrides.
We see repeated patterns for environment toggles. Provide a shared test utility to reduce duplication and the risk of forgetting to revert changes.src/lib/egg.ts (2)
98-99: Reusable naming for compatibility property.
RenamingHttpClientNextto something more descriptive (e.g.,HttpClientCompat) could clarify its purpose as a legacy alias for Egg 3.x.
444-444: Use optional chaining and nullish coalescing to simplify error logging.
Replace theerr && err.message || errusage with optional chaining for readability and maintainability.- this.coreLogger.error('[egg:unhandledRejection] %s', err && err.message || err); + this.coreLogger.error('[egg:unhandledRejection] %s', err?.message ?? err);🧰 Tools
🪛 Biome (1.9.4)
[error] 444-444: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
src/lib/core/utils.ts (1)
33-33: LGTM! Consider adding type narrowing for better type safety.The addition of URL instance handling is a good improvement. For better type safety, consider using type narrowing:
-if (isSymbol(value) || isRegExp(value) || value instanceof URL) { +if (isSymbol(value) || isRegExp(value) || (typeof URL !== 'undefined' && value instanceof URL)) {test/lib/agent.test.ts (2)
12-12: Add type assertion for better type safety.The app variable should have a more specific type than MockApplication.
-let app: MockApplication; +let app: MockApplication & { notExpect: (type: string, pattern: RegExp) => void };
25-25: Use path.join for cross-platform compatibility.Ensure log file paths are constructed using path.join for cross-platform compatibility.
-const body = fs.readFileSync(path.join(baseDir, 'logs/agent-throw/common-error.log'), 'utf8'); +const body = fs.readFileSync(path.join(baseDir, 'logs', 'agent-throw', 'common-error.log'), 'utf8');Also applies to: 39-39, 53-53
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
package.json(2 hunks)src/app/extend/context.types.ts(1 hunks)src/index.ts(1 hunks)src/lib/application.ts(2 hunks)src/lib/core/base_context_class.ts(1 hunks)src/lib/core/messenger/base.ts(1 hunks)src/lib/core/messenger/index.ts(1 hunks)src/lib/core/messenger/ipc.ts(1 hunks)src/lib/core/messenger/local.ts(1 hunks)src/lib/core/utils.ts(1 hunks)src/lib/egg.ts(2 hunks)src/lib/error/CookieLimitExceedError.ts(1 hunks)src/lib/error/MessageUnhandledRejectionError.ts(1 hunks)src/lib/error/index.ts(1 hunks)test/fixtures/apps/agent-throw/agent.js(1 hunks)test/fixtures/apps/agent-throw/app/router.js(1 hunks)test/fixtures/apps/app-throw/app/router.js(0 hunks)test/fixtures/apps/base-context-class/app/controller/home.js(1 hunks)test/fixtures/apps/base-context-class/app/router.js(0 hunks)test/fixtures/apps/base-context-class/app/service/home.js(1 hunks)test/fixtures/apps/base-context-class/config/config.default.js(0 hunks)test/fixtures/apps/base-context-class/config/config.unittest.js(0 hunks)test/fixtures/apps/demo/config/config.default.js(1 hunks)test/fixtures/apps/demo/config/config.unittest.js(1 hunks)test/fixtures/apps/dumpconfig/app.js(1 hunks)test/fixtures/apps/httpclient-next-overwrite/app.js(1 hunks)test/fixtures/apps/httpclient-tracer/app.js(1 hunks)test/fixtures/apps/keys-missing/config/config.unittest.js(1 hunks)test/fixtures/apps/logger-level-debug/app/router.js(1 hunks)test/lib/agent.test.js(0 hunks)test/lib/agent.test.ts(1 hunks)test/lib/application.test.ts(11 hunks)test/lib/core/context_httpclient.test.ts(1 hunks)test/lib/core/context_httpclient_timeout.test.js(0 hunks)test/lib/core/context_performance_starttime.test.js(0 hunks)test/lib/core/context_performance_starttime.test.ts(1 hunks)test/lib/core/cookies.test.ts(10 hunks)test/lib/core/custom_loader.test.ts(1 hunks)test/lib/core/dnscache_httpclient.test.js(0 hunks)test/lib/core/dnscache_httpclient.test.ts(1 hunks)test/lib/core/httpclient.test.ts(16 hunks)test/lib/core/httpclient_tracer_demo.test.ts(1 hunks)test/lib/core/logger.test.ts(9 hunks)test/lib/egg.test.js(0 hunks)test/lib/egg.test.ts(1 hunks)test/utils.ts(2 hunks)
💤 Files with no reviewable changes (9)
- test/fixtures/apps/base-context-class/config/config.default.js
- test/fixtures/apps/base-context-class/app/router.js
- test/fixtures/apps/base-context-class/config/config.unittest.js
- test/fixtures/apps/app-throw/app/router.js
- test/lib/core/context_httpclient_timeout.test.js
- test/lib/core/context_performance_starttime.test.js
- test/lib/core/dnscache_httpclient.test.js
- test/lib/agent.test.js
- test/lib/egg.test.js
✅ Files skipped from review due to trivial changes (4)
- test/fixtures/apps/dumpconfig/app.js
- src/lib/error/index.ts
- test/fixtures/apps/demo/config/config.unittest.js
- test/fixtures/apps/keys-missing/config/config.unittest.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/egg.ts
[error] 444-444: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/lib/core/httpclient.test.ts
[error] 28-30: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/lib/core/context_httpclient.test.ts
[error] 12-14: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (47)
test/fixtures/apps/logger-level-debug/app/router.js (1)
1-1: Use explicit import destructuring to clarify usage.
Importing only the necessary functions (like scheduler.wait()) from node:timers/promises is a good practice. Just ensure that scheduler is the correct import path for using wait() based on your Node.js version.
[approve]
test/utils.ts (1)
8-8: Ensure Node.js compatibility for scheduler.wait.
Using scheduler.wait() requires Node.js v16.14 or higher. Confirm your project’s Node.js engine requirement aligns with this.
test/fixtures/apps/base-context-class/app/service/home.js (1)
3-3: Transition to async/await looks good!
Converting * show() to async show() is a nice modernization. Ensure that any caller of this function, especially older generator-based code, has been updated to await this method.
✅ Verification successful
All callers properly await the async show() method
The verification shows that the only caller of show() is in test/fixtures/apps/base-context-class/app/controller/home.js and it's already using await with the async method:
await this.service.home.show();This confirms that the transition to async/await is complete and consistent across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for references to 'show()' in the codebase
rg -A 5 $'\\.show\\('
Length of output: 628
src/lib/core/messenger/base.ts (2)
8-11: Consider validating the egg argument.
If a consumer of this class mistakenly passes an invalid or incomplete object, the egg core logger usage in subsequent methods might fail at runtime. Adding a basic validation or guard here could prevent unexpected runtime errors.
13-15: Good usage of captureRejections with a dedicated error class.
This approach cleanly logs unhandled promise rejections with contextual information, which improves maintainability and debuggability.
src/lib/core/messenger/local.ts (1)
4-15: Transitioning from EventEmitter to BaseMessenger is well-executed.
Now that Messenger expects BaseMessenger functionalities, make sure any logic that previously relied on direct EventEmitter behavior (e.g., re-adding default listeners, custom error-handling overrides) aligns with the new setup.
test/lib/egg.test.ts (1)
316-364: Ensure all external listeners are removed on close.
The tests confirm that unhandledRejection listeners are removed. It might be beneficial to also confirm any other global listeners set by external libraries or plugins, if relevant, to avoid memory leaks.
test/fixtures/apps/agent-throw/agent.js (1)
3-4: Synchronous agent error message is now more descriptive.
Renaming it to "agent error in sync function" helps clarify debugging logs. Ensure any existing tests or usage referencing the old message have been updated.
src/lib/error/CookieLimitExceedError.ts (1)
5-6: Beware of logging sensitive cookie contents.
When possible, mask or truncate the cookie string in logs to mitigate PII leakage. Consider a configuration-based approach if you need more control over sensitive data.
src/lib/error/MessageUnhandledRejectionError.ts (1)
5-6: Ensure Node.js environment supports the cause option for Error constructors.
Node.js added support for { cause } in Error constructors starting in Node.js 16.9.0. Verify target runtimes support this feature or consider a compatible fallback for older environments.
test/fixtures/apps/agent-throw/app/router.js (1)
7-10: Confirm asynchronous exceptions are handled in agent.
The new /agent-throw-async route broadcasts an event leading to an asynchronous throw in agent code. Ensure that any unhandled rejections are properly captured by the newly introduced MessageUnhandledRejectionError or other error handling mechanisms. This helps maintain consistent error logging and avoids silent failures.
src/lib/core/messenger/index.ts (1)
12-15: Improved readability with local variable assignment.
By storing the messenger instance in a local variable prior to returning it, the code enhances readability and clarity. This makes debugging more straightforward if future logic or conditions are added around messenger instantiation.
test/lib/core/custom_loader.test.ts (3)
1-2: Imports look good; ensure that all references to the old import style are removed.
These new ESM-style imports from @eggjs/mock and local utilities are straightforward. Double-check and remove any lingering require usage to maintain consistency.
4-5: Retain descriptive test suite names.
The new test suite name clearly indicates what functionality is being tested. This helps keep test files organized and comprehensible.
9-9: Confirm custom-loader readiness logic.
Using app.ready() ensures asynchronous setup is complete. Check for potential race conditions in your tests or hooks that might rely on app prior to app.ready().
test/lib/core/context_performance_starttime.test.ts (2)
1-2: Good practice to use strict assertions and typed mocks in new tests.
The imports of assert and the typed MockApplication improve clarity and prevent subtle type errors.
7-10: Ensure reliable application state before each test.
before() is correctly used to initialize the app. If subsequent tests manipulate a shared state, consider beforeEach() or afterEach() for isolation. This helps keep tests independent.
test/lib/core/context_httpclient.test.ts (2)
1-2: Good transition to ES module imports
Switching from CommonJS to ES modules aligns with the broader move to TypeScript and modern Node.js convention.
5-6: Enhance type annotations
Adding url: string and app: MockApplication improves clarity and type safety in the test code.
src/index.ts (1)
17-19: Exporting error module
Re-exporting errors centralizes error handling into a single import path. This is a neat approach for better maintainability. Ensure that none of the exported errors conflict with existing exports.
test/lib/core/httpclient_tracer_demo.test.ts (4)
1-3: ES module imports and new utilities
The transition to ES module syntax and updated utility imports (createApp, startLocalServer) aligns with your overall TypeScript migration.
5-7: Add type annotations
Declaring url: string and app: MockApplication clarifies usage and prevents potential runtime errors.
10-10: Initialize app using createApp
This change standardizes setup across test suites. Well done.
14-14: Start local server
As with the other test, using startLocalServer() is consistent and improves environment test control.
src/lib/core/messenger/ipc.ts (3)
6-6: Use explicit imports for clarity
Good job importing BaseMessenger from ./base.js. If there are more shared abstractions, consider consolidating imports into a single index to simplify future maintenance.
13-13: Validate inheritance structure
Switching from EventEmitter to extending BaseMessenger can influence existing consumers of this class. Verify that all listeners and events are fully compatible with the new base class.
18-18: Pass the egg argument to base constructor
Using super(egg) ensures the egg context is passed correctly to BaseMessenger. Confirm that the base class properly initializes or stores the reference.
test/lib/application.test.ts (5)
6-6: Adopt new pending usage
Replacing or adopting pedding is a good step to unify asynchronous test handling. Ensure all references to older libraries are removed to avoid confusion.
7-7: Explicit import of CookieLimitExceedError
You're now importing CookieLimitExceedError for additional test coverage. This aligns well with verifying cookies that exceed length limits. Make sure all scenarios are covered.
44-44: Test timeouts in asynchronous flows
The startTimeout event test is improved by using typed app. Confirm that done is called only once, and consider adding a safety timer or a try/catch block to avoid indefinite waiting in case of unexpected failure.
226-231: Thorough check of CookieLimitExceedError properties
The test asserts the err.key, err.cookie, err.name, and err.message, which is good for verifying correctness. Confirm that no additional property validations (e.g., err.stack) are required for debugging.
Line range hint 242-258: Leverage typed test setup
You’re using typed events with a MockApplication instance. This ensures consistent introspection for request and response events. Nice work extending coverage for multiple event hooks.
test/lib/core/cookies.test.ts (5)
11-11: Improve type-safety with MockApplication
Defining app: MockApplication clarifies test setup and ensures any typed methods (like mockContext) are recognized.
41-41: Verify log file usage
Reading logs from common-error.log ensures that CookieLimitExceedError is indeed captured. Double-check file rotations or concurrent writes in production environments.
126-126: Expiration checks
Ensuring the cookie’s expires attribute is in the past effectively removes it. The test correctly validates that the new Date is beyond the expiry date. This is good practice to confirm removal.
Also applies to: 140-140
196-197: Type casting headers
Casting res.headers['set-cookie'] to a string array is appropriate for TypeScript. Ensure this assumption holds across all test runners and Node.js versions.
230-233: Chained cookie checks
Using the joined cookies in subsequent requests thoroughly tests encryption and decryption logic. Great approach to comprehensive coverage.
src/lib/application.ts (2)
15-15: Newly introduced CookieLimitExceedError
Importing this specialized error class is a clean approach to differentiate cookie-limit issues from other errors. Ensure it’s used consistently in all relevant places.
276-279: Specific error handling for cookieLimitExceed
Creating a CookieLimitExceedError with name and value provides clarity in logs. Good use of typed errors to help debug production issues.
test/lib/core/dnscache_httpclient.test.ts (1)
44-66: Verify DNS failure scenario effectiveness in testing environment.
These lines are reliant on specific DNS configurations and providers. Ensure that the test does not produce false negatives in diverse environments or CI systems with varying DNS behaviors.
test/lib/core/logger.test.ts (3)
10-13: Ensure proper cleanup when tests finish.
The afterEach block closes the app, but adding more test sequences might require additional teardown checks. Confirm that resources like file descriptors or mock environment variables are reset properly.
53-59: Confirm environment-based logging levels.
Favor narrower environment overrides or mocking methods to avoid unintended side effects on other tests or local development sessions.
Line range hint 155-169: Test coverage note for cluster-based log outputs.
These tests use process forking (app.debug/coverage). Confirm that coverage is aggregated properly across child processes if measuring test coverage in CI.
test/lib/core/httpclient.test.ts (3)
28-30: Multiple “before” hooks detected in same describe block.
Mocha allows it, but it can sometimes introduce confusion or potential conflicts. Consider merging or refactoring if they serve complementary setup logic.
🧰 Tools
🪛 Biome (1.9.4)
[error] 28-30: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
158-173: Enhance readiness checks for repeated environment manipulations.
It’s recommended to reset process.env to a known state after each test suite to prevent undesired coupling when environment variables are changed.
Line range hint 497-526: Check concurrency readiness for “before app ready multi httpclient request tracer.”
Making multiple requests before app.ready() can lead to concurrency edge cases. Thoroughly test error conditions to ensure the tracer remains consistent across parallel requests.
package.json (1)
81-81: Verify compatibility with pedding v2.0.0.
The major version bump of pedding from v1 to v2 could introduce breaking changes.
Run the following script to check for breaking changes:
✅ Verification successful
Let me check the actual usage of pedding in the codebase to verify compatibility:
Breaking changes in pedding v2.0.0 are compatible with this codebase
The update to pedding v2.0.0 is safe because:
- The main breaking change is dropping support for Node.js < 18.19.0, which aligns with this project's requirements
- The codebase is already using the correct
pendingimport (not the deprecatedpeddingfunction name) - Usage is limited to test files only, not affecting production code
- The package maintains both CJS and ESM support through tshy, ensuring compatibility with the current import style
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for breaking changes in pedding v2.0.0
# Get the changelog or release notes
gh api repos/node-modules/pedding/releases/latest --jq .body
# Find usage patterns that might need updates
rg -l 'pedding' --type ts
Length of output: 1491
Script:
#!/bin/bash
# Check how pedding is used in the test file
rg -C 5 'pedding' test/lib/application.test.ts
# Look for any other potential usages with different patterns
rg -l 'pending' --type ts
Length of output: 574
| setTimeout(() => { | ||
| const body = fs.readFileSync(path.join(baseDir, 'logs/agent-throw/common-error.log'), 'utf8'); | ||
| assert.match(body, /nodejs\.MessageUnhandledRejectionError: event: agent-throw-async, error: agent error in async function/); | ||
| app.notExpect('stderr', /nodejs.AgentWorkerDiedError/); | ||
| done(); | ||
| }, 1000); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace fixed timeouts with a more reliable approach.
Using fixed timeouts (1000ms) for log file checks could lead to flaky tests. Consider using a polling mechanism or the pedding package that was just updated to v2.0.0.
Example approach using polling:
async function waitForLog(logPath: string, pattern: RegExp, timeout = 5000): Promise<void> {
const start = Date.now();
while (Date.now() - start < timeout) {
try {
const content = fs.readFileSync(logPath, 'utf8');
if (pattern.test(content)) return;
} catch (err) {
// ignore file not found
}
await new Promise(resolve => setTimeout(resolve, 100));
}
throw new Error(`Timeout waiting for log pattern: ${pattern}`);
}Also applies to: 38-44, 52-58
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (15)
test/lib/core/view.test.ts (1)
122-125: Improve assertion message clarity
When using multi-line template comparisons, consider providing a custom assertion message for easier debugging if the assertion fails.test/lib/core/logger.test.ts (8)
22-23: Consolidate environment variable mocking
Repeated usage ofmm.env('xxx')andmm(process.env, 'EGG_LOG', '')can be refactored for clarity. A helper function could centralize environment configuration setup, ensuring reusability.Also applies to: 38-39, 53-53, 66-66, 79-79, 91-91
27-27: Type safety for logger options
Casting withas anymight obscure potential type errors. If possible, define or import the correct interface for logger instances, ensuring thorough type safety in this converted TypeScript file.Also applies to: 29-31, 44-47, 56-59, 69-72, 82-85, 94-95
101-115: File read checks
Reading log files at lines 113-115 is good for verifying logger output. Consider more robust comparisons or partial matching strategies in future refactors to catch subtle logging changes without needing exact string matches.
122-123: Separate environment-based tests
For production, local, and other environment tests, you might increase readability by grouping them in distinct describe blocks so each environment scenario is isolated. This ensures clear scoping of environment mocks.Also applies to: 140-140
201-201: Expand coverage on error redirection
The test checks multiple logger outputs (logger, coreLogger, errorLogger, customLogger). Consider adding coverage for newly introduced error classes, if applicable, to ensure consistent handling across loggers.
219-219: Agent’s logger reference
Asserting thatapp.agent.logger.options.file === app.agent.coreLogger.options.fileensures the agent’s logger is consistent with the core logger. Consider clarifying this assertion or adding more scenarios for agent-based logging.
244-246: Multiple new test apps
You’re creating additional test apps (logger-level-debug,custom-logger), which is fine. Keep an eye on test run time and resource usage; consider merging or reusing apps if performance becomes a concern.Also applies to: 266-268
258-258: Ensure log content checks
Readingfoo-web.logfor theDEBUGstring is a minimal check. If the log output changes in the future (e.g., structured JSON), you might need a more flexible approach than raw string matching.src/lib/application.ts (1)
271-279: Confirm deprecation logic for generator functions.The
toAsyncFunctionmethod now throws for generator functions. This is a functional approach to enforce your best practice. Document it in your migration or upgrade guide for users who rely on generator-based code.test/lib/core/singleton.test.ts (5)
6-7: Consider restricting theanytype forconfig.
Defining a more specific interface or type instead ofanywould improve type safety and readability.
16-16: Parameter could be more strictly typed.
If you know the shape ofconfig, consider adding an interface to avoidanyusage.
20-21: Enhance type definitions in async function.
Similar to thecreatefunction above, consider specifying a more detailed type thananyfor theconfigparameter inasyncCreate.
27-28: Avoid using the delete operator for performance reasons.
Static analysis flags these lines for performance issues. Consider assigningundefinedto remove the properties instead of deleting them.- delete (DataService as any).prototype.createInstance; - delete (DataService as any).prototype.createInstanceAsync; + (DataService as any).prototype.createInstance = undefined; + (DataService as any).prototype.createInstanceAsync = undefined;🧰 Tools
🪛 Biome (1.9.4)
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
214-214: Configuringappwithanyis accepted for test mocks.
While usinganyhelps quickly set up mock objects, a typed approach can still catch potential config issues early.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
package.json(3 hunks)src/lib/application.ts(2 hunks)src/lib/core/utils.ts(2 hunks)test/fixtures/apps/multiple-view-engine/app.js(0 hunks)test/fixtures/apps/multiple-view-engine/ejs.js(2 hunks)test/fixtures/apps/multiple-view-engine/nunjucks.js(1 hunks)test/lib/core/logger.test.ts(9 hunks)test/lib/core/router.test.ts(2 hunks)test/lib/core/singleton.test.ts(15 hunks)test/lib/core/utils.test.ts(8 hunks)test/lib/core/view.test.ts(3 hunks)
💤 Files with no reviewable changes (1)
- test/fixtures/apps/multiple-view-engine/app.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- src/lib/core/utils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/lib/core/utils.test.ts
[error] 47-47: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 47-47: Shouldn't redeclare 'a'. Consider to delete it or rename it.
'a' is defined here:
(lint/suspicious/noRedeclare)
[error] 48-48: Shouldn't redeclare 'a'. Consider to delete it or rename it.
'a' is defined here:
(lint/suspicious/noRedeclare)
test/lib/core/singleton.test.ts
[error] 27-27: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 28-28: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
🔇 Additional comments (50)
test/lib/core/view.test.ts (4)
1-4: Use consistent ESM import style across test files
Switching from CommonJS to ES modules is good. However, ensure all test files follow a uniform import style, especially when referencing project-relative paths. Double-check for any leftover require statements across the test suite to maintain consistency.
6-7: Validate mocking approach
Using mm.restore in afterEach is a recommended approach. Just make sure any additional mocks or spies throughout the test suite are properly restored as well, preventing any conflict in subsequent tests.
10-13: Ensure base directory correctness
The usage of getFilepath('apps/multiple-view-engine') sets the base directory. Double-check the folder structure if tests are failing due to path resolution issues.
82-84: No naming collisions for 'app' variable
Since app is redeclared, confirm that there is no collision or reuse with other declarations in this file or across test files.
test/lib/core/logger.test.ts (5)
1-9: ESM Import consistency
Similar to view.test.ts, these imports have been converted to ES modules. Confirm that related helpers or utilities in this directory are also updated to ensure consistent ESM usage, avoiding any potential import mismatches.
10-13: Shortened wait time
Waiting 500ms instead of 5000ms helps speed up test execution, which is beneficial. However, verify that it’s still sufficient for all resources to close properly in high-latency environments or continuous integration.
155-155: Cluster-based tests
Using cluster('apps/logger') with console event checks is comprehensive. Confirm that this cluster approach is consistent with how the rest of your test suite integrates advanced scenarios like multi-process or agent-based logging.
Also applies to: 169-169
182-182: Asynchronous test finalization
Ending the cluster-based test via .end(async (err) => ...) is valid. If you encounter flakiness, consider implementing a top-level await or a more robust error-handling strategy.
Also applies to: 186-186, 190-190
226-226: Verify trace integration
Including a mock trace ID is a great approach. Ensure that any recently introduced tracing or APM integrations are tested to guarantee that logs properly incorporate trace information in different environments.
test/lib/core/router.test.ts (3)
1-3: Ensure correct import references and type definitions.
The import references look consistent, and MockApplication typing is correct. However, double-check if .../utils.js is the correct relative path and that the MockApplication type is compatible with usage throughout this test file.
5-6: Clarify test description.
The test suite description has been updated to reflect the .ts extension. Confirm that all references and test-running scripts account for the new file extension.
129-147: No issues found; descriptive coverage is good.
This block of tests thoroughly validates query string handling for multi-valued parameters. Good job ensuring coverage of edge cases (e.g., foo&?). The code is consistent with the existing pattern.
test/lib/core/utils.test.ts (14)
16-16: Validate optional array parameter usage.
Check if the second parameter [] is indeed intended to ignore or transform specific object properties. Ensure a consistent approach to ignoring properties across your tests and that it aligns with how convertObject is implemented.
22-22: Utilize assert.strictEqual or assert.equal consistently.
You're already using assert.equal in other lines; the usage is consistent. This line is good to go.
29-30: Ensure regexp conversion covers edge cases.
The function call and assertion for RegExp are straightforward. Just ensure any edge case (e.g., multiline flags) is tested if relevant.
37-38: Date conversion coverage.
The line shows date conversion into a '<Date>' placeholder. This is fine if it's the intended behavior. Confirm that time zone or locale checks are not required in downstream logic.
51-55: Function name placeholders and reflection.
Using Reflect.set to rename _name is a reasonable approach for debugging. The conversion to '<Function ...>' or '<GeneratorFunction ...>' is consistent with your pattern. No immediate changes needed here.
67-70: Ensure consistent naming for errors.
If additional error classes or code references are introduced, keep them consistent. This test verifies the output is <Error> or <TestError>. Looks good.
81-82: Anonymous vs. named classes.
Similar to function naming, the test output is consistent with your placeholders '<Class ...>'. No changes required.
91-91: Confirm correct use of SlowBuffer.from.
Double-check that using an extended Buffer.from logic is safe in all Node.js versions that you support.
94-97: Buffer representation checks.
These lines confirm the buffer length is correctly derived. The functionality is well-structured.
120-126: Assess ignoring behavior for multiple property types.
Verifying that each property is replaced with your custom placeholder is correct. This is consistent with the rest of the test.
140-144: Recursive object conversion.
It’s good that you’re validating nested objects. The placeholders are consistent. No issues found.
153-154: Anonymous class naming.
This test ensures an anonymous class is labeled '<Class anonymous>' vs. '<Class anonymousClassWithPropName>'. No issues found.
160-161: Prevent potential SSRF or injection from malformed URLs.
The safeParseURL function safely returns null on invalid URLs. Good approach! No issues found.
165-166: Confirm domain parsing for all TLDs.
The test ensures the function handles some unusual domain strings. This appears adequate.
src/lib/application.ts (3)
8-8: Check for side effects of additional import.
isGeneratorFunction is introduced here. Ensure there's no conflict with other is-type-of usage or an alternative method.
16-16: Verify CookieLimitExceedError import path.
Ensure that ./error/index.js re-exports CookieLimitExceedError and that this is the correct location.
287-290: New cookie limit exceed event.
Using CookieLimitExceedError is more explicit than a generic Error, which is great. Confirm that ctx.coreLogger.error is appropriate. For high severity, consider ctx.coreLogger.warn or ctx.coreLogger.error depending on your logging level.
test/lib/core/singleton.test.ts (16)
1-3: ES module import transition is well done.
The move from CommonJS to ES modules is clean and follows modern JavaScript recommendations.
39-39: Test structure and assertions are straightforward.
Using assert.equal ensures clarity in the test output when comparing values.
Also applies to: 47-48
60-60: Multiple clients initialization looks accurate.
The logic effectively demonstrates handling of multiple clients with correct assertions.
Also applies to: 68-70
74-74: Use of default config merges well.
The test covers the merging of default and custom configs. Looks good.
Also applies to: 91-93
97-97: Valid approach to test multiple client definitions with fallback defaults.
This ensures coverage for scenarios where clients have partially defined configurations.
127-127: Comprehensive test for default creation without predefined client configurations.
This confirms the fallback logic is working as intended.
151-151: Preventing object extensions is a thoughtful edge case.
Good job verifying that the code can handle unextensible objects safely.
Also applies to: 156-156
179-179: Testing frozen objects ensures immutability compatibility.
Checking that a frozen DataService can still function is beneficial.
Also applies to: 184-184
221-226: Well-implemented logger usage for warnings.
This captures unmodifiable prototypes, ensuring correct test coverage for potential runtime anomalies.
248-248: Client name is properly validated.
This test ensures that the correct client name is passed during instantiation.
Also applies to: 253-253
281-281: Using asyncCreate for single client initialization is consistent.
The test covers the asynchronous flow and ensures the instance is correctly assigned.
303-303: Asynchronous multiple-client creation logic looks good.
Verifies each client is separately initialized.
317-317: Default config fallback for async creation.
Ensures we can create new instances when no direct client/clients config is defined.
341-341: Appropriate test for throwing errors when using the wrong creation method.
This maintains consistent usage of asynchronous creation.
361-363: Thorough error message check.
Ensures the user is warned about synchronous creation attempts in async mode.
372-372: Client name in async creation is properly validated.
Verifies that the _create function receives the correct client name within the asynchronous flow.
Also applies to: 377-377
test/fixtures/apps/multiple-view-engine/nunjucks.js (2)
1-1: Confirm Node.js version compatibility.
The module node:timers/promises requires Node.js v16+; ensure your environment is running a compatible Node version.
5-5: Delay approach approved.
Using scheduler.wait(10) is a clean and native way to introduce a short wait period. If the delay is primarily for testing or demonstration, this looks fine.
test/fixtures/apps/multiple-view-engine/ejs.js (3)
1-1: Confirm Node.js version compatibility.
Similar to nunjucks.js, you use node:timers/promises. Ensure Node.js v16+ is used in all relevant environments.
5-5: Fine usage of wait.
The 10 ms wait might help simulate I/O latency. Using scheduler.wait is a valid approach if you want a short deterministic delay.
15-15: Consistent delay pattern.
Repeating the same scheduler.wait(10) usage in renderString ensures consistent behavior across your view methods.
| arrowFunction$: (a: any) => { console.log(a); }, | ||
| /* eslint object-shorthand: 0 */ | ||
| anonymousFunction$: function(a) { console.log(a); }, | ||
| generatorFunction$: function* a(a) { console.log(a); }, | ||
| asyncFunction$: async function a(a) { console.log(a); }, | ||
| anonymousFunction$: function(a: any) { console.log(a); }, | ||
| generatorFunction$: function* a(a: any) { console.log(a); }, | ||
| asyncFunction$: async function a(a: any) { console.log(a); }, |
There was a problem hiding this comment.
Address generator function with no yield and naming conflicts.
The static analysis indicates:
- The generator function
function* a(a: any)does not containyield. - Variable redeclaration or overshadowing of the name
a.
If you need a generator, add a yield statement; otherwise, convert it into a standard or async function. Also consider renaming the parameter or function name to avoid shadowing warnings.
Proposed minimal fix:
- generatorFunction$: function* a(a: any) { console.log(a); },
+ generatorFunction$: function aGen(param: any) {
+ console.log(param);
+ },📝 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.
| arrowFunction$: (a: any) => { console.log(a); }, | |
| /* eslint object-shorthand: 0 */ | |
| anonymousFunction$: function(a) { console.log(a); }, | |
| generatorFunction$: function* a(a) { console.log(a); }, | |
| asyncFunction$: async function a(a) { console.log(a); }, | |
| anonymousFunction$: function(a: any) { console.log(a); }, | |
| generatorFunction$: function* a(a: any) { console.log(a); }, | |
| asyncFunction$: async function a(a: any) { console.log(a); }, | |
| arrowFunction$: (a: any) => { console.log(a); }, | |
| /* eslint object-shorthand: 0 */ | |
| anonymousFunction$: function(a: any) { console.log(a); }, | |
| generatorFunction$: function aGen(param: any) { | |
| console.log(param); | |
| }, | |
| asyncFunction$: async function a(a: any) { console.log(a); }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 47-47: This generator function doesn't contain yield.
(lint/correctness/useYield)
[error] 47-47: Shouldn't redeclare 'a'. Consider to delete it or rename it.
'a' is defined here:
(lint/suspicious/noRedeclare)
[error] 48-48: Shouldn't redeclare 'a'. Consider to delete it or rename it.
'a' is defined here:
(lint/suspicious/noRedeclare)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (19)
src/lib/core/base_hook_class.ts (1)
13-13: Explicitly specify return type forlogger.Inferring the type is acceptable, but explicitly specifying the logger's type (e.g.,
EggLogger) can improve type clarity and help developers quickly identify the logger interface.test/fixtures/apps/subdir-services/app/service/foo/subdir2/sub2.js (1)
9-9: Optional: Add clarity on use cases.
While the method returns an object, consider documenting the expected use cases of this method and handling edge cases (if any) like a missing or invalidnameparameter.test/lib/core/loader/load_boot.test.ts (2)
6-38: Suggestion to consolidate repeated test logic or ensure app closure in a shared hook.Both the CommonJS and ESM tests follow a similar approach (initialize app, wait for readiness, then close). Consider moving the app closure logic to an
afterhook to ensure the application is always closed, even if a test fails. Alternatively, unify the repeated setup/teardown patterns for maintainability.
15-23: Consider adding negative or error-handling scenarios.Currently, the test focuses on verifying that
app.jsis successfully loaded and that certain log messages are present or absent. Adding tests that simulate failure or incomplete setups (e.g., missing files) could increase coverage of edge cases and better confirm that error paths are handled correctly.test/fixtures/apps/boot-app/app.js (2)
6-6: Refactor suggestion for constructor usage.
If this file’s future expansions remain straightforward, this pattern is fine. However, if you plan to extend "CustomBoot" further, destructuring theappparameter can keep the constructor cleaner.constructor(app) { - this.app = app; + const { config, logger, messenger, ...rest } = app; + Object.assign(this, { config, logger, messenger }); app.bootLog = []; ... }
19-40: Assess usage of short waits (1 ms).
All calls toscheduler.wait(1)pause execution for a minimal time. If these waits are placeholders for asynchronous operations or to ensure ordering, consider a more descriptive constant or a condition-based delay. This clarifies intent and ensures maintainability for future changes.- await scheduler.wait(1); + const WAIT_DURATION_MS = 1; + await scheduler.wait(WAIT_DURATION_MS);test/lib/core/loader/config_loader.test.ts (2)
41-41: RepeatedcreateAppcalls for environment checks.
Each test re-creates the app for a different environment. This is fine, though be mindful of test runtime if many tests do the same.
48-48: Potential unification of setup logic.
You could centralizecreateApp('apps/demo')in abeforeEachblock if multiple tests share it. This is optional but might reduce redundancy.src/lib/egg.ts (1)
445-445: Consider optional chaining for better readability.
You can replaceerr && err.message || errwitherr?.message || errto make the code more concise:- this.coreLogger.error('[egg:unhandledRejection] %s', err && err.message || err); + this.coreLogger.error('[egg:unhandledRejection] %s', err?.message || err);🧰 Tools
🪛 Biome (1.9.4)
[error] 445-445: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/lib/core/loader/load_plugin.test.ts (3)
1-6: Consider specifying stricter types forlogger.
You're importingEggConsoleLoggerand assigning it to a variable declared asany; consider updating the type to reflect the actual class for better type safety.
10-12: Strongly typedappis great, butloggeris typed asany.
While usingMockApplicationforappis beneficial, consider changinglogger: anyto the more specificEggConsoleLoggertype for improved maintainability.
128-145: Warning message test is well-structured.
This approach withmm(logger, 'warn', ...)successfully captures the log. If needed, consider verifying that no other unexpected warnings occur.test/fixtures/apps/boot-app/agent.js (2)
4-4: Clear class naming
Renaming toCustomBootclarifies the role of this class. Consider aligning the file name with the exported class if needed.
40-40: Consistent serverDidReady
Again, the minimal wait ensures a consistent approach in all lifecycle handlers. If the actual wait duration is important, consider a well-documented constant.test/fixtures/apps/boot-app-esm/app.js (2)
5-5: Exported class
Naming the classCustomBoothelps maintain clarity. If your convention is to match filenames with class names, you might rename the file consistently.
11-13: Event-driven design
Listening foragent2apponapp.messengeris straightforward. The short handler effectively updatesmessengerLog. Logs may need further expansion for debugging, if required.test/fixtures/apps/boot-app-esm/agent.js (3)
10-12: Integration events
Subscribing toagent.messengerevents helps keep the agent in sync with the application. Consider logging or error handling around messenger events if desired.
24-27: Updated willReady
Again, the 1ms delay might be a placeholder; if not, consider documenting the reason.
40-43: Final handshake
serverDidReadyis also implemented with the same minimal wait. For clarity, consider referencing a single shared constant for the wait duration.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (23)
src/lib/core/base_hook_class.ts(1 hunks)src/lib/egg.ts(3 hunks)test/fixtures/apps/boot-app-esm/agent.js(1 hunks)test/fixtures/apps/boot-app-esm/app.js(1 hunks)test/fixtures/apps/boot-app-esm/package.json(1 hunks)test/fixtures/apps/boot-app/agent.js(2 hunks)test/fixtures/apps/boot-app/app.js(2 hunks)test/fixtures/apps/demo/config/config.default.js(1 hunks)test/fixtures/apps/demo/config/config.unittest.js(1 hunks)test/fixtures/apps/subdir-services/app/service/foo/subdir2/sub2.js(1 hunks)test/fixtures/apps/subdir-services/app/service/ok.js(1 hunks)test/fixtures/apps/subdir-services/app/service/old_style.js(1 hunks)test/fixtures/apps/subdir-services/config/config.default.js(0 hunks)test/index.test.ts(1 hunks)test/lib/core/config/config.cookies.test.ts(1 hunks)test/lib/core/config/config.test.js(0 hunks)test/lib/core/config/config.test.ts(1 hunks)test/lib/core/loader/config_loader.test.ts(1 hunks)test/lib/core/loader/load_app.test.ts(1 hunks)test/lib/core/loader/load_boot.test.js(0 hunks)test/lib/core/loader/load_boot.test.ts(1 hunks)test/lib/core/loader/load_plugin.test.ts(11 hunks)test/lib/core/loader/load_service.test.ts(3 hunks)
💤 Files with no reviewable changes (3)
- test/fixtures/apps/subdir-services/config/config.default.js
- test/lib/core/config/config.test.js
- test/lib/core/loader/load_boot.test.js
✅ Files skipped from review due to trivial changes (1)
- test/fixtures/apps/boot-app-esm/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- test/fixtures/apps/demo/config/config.default.js
- test/fixtures/apps/demo/config/config.unittest.js
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/egg.ts
[error] 445-445: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/lib/core/loader/config_loader.test.ts
[error] 10-10: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
test/lib/core/loader/load_service.test.ts
[error] 8-8: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
[error] 53-53: Disallow duplicate setup and teardown hooks.
Disallow afterEach duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (72)
test/index.test.ts (2)
17-17: Newly added property is consistent and correct.
CookieLimitExceedError is spelled correctly, maintains the established naming scheme for custom errors, and appears to be tested correctly. Good job adding it to the array of expected keys in the egg object.
22-22: Newly added property is consistent and correct.
MessageUnhandledRejectionError follows the same naming pattern as other custom errors in the egg object. The test validations align well with the new property and ensure coverage.
src/lib/core/base_hook_class.ts (1)
6-6: Consider initializing or verifying usage of declare fullPath.
Although using declare is valid for ambient declarations in TypeScript, ensure that external usage or initialization is performed prior to reading fullPath. Otherwise, there is a risk of accessing an undefined property.
test/lib/core/loader/load_app.test.ts (3)
1-2: Switched to ES module syntax for imports.
This transition from CommonJS require to ES modules import improves code clarity, type support, and aligns with modern JavaScript/TypeScript best practices.
5-5: Introduced TypeScript type for app.
Defining app as MockApplication provides better type safety and improves developer experience by enabling proper IntelliSense and compile-time checks.
7-7: Refactored application creation logic.
Using createApp('apps/loader-plugin') clarifies which test fixture is being loaded and ensures consistency across test suites. Confirm that any additional test fixtures align with the new function signature if more are converted.
test/fixtures/apps/subdir-services/app/service/old_style.js (1)
1-1: Consider adding defensive checks for ctx.
While converting to an async arrow function is a positive step, ensure that ctx is always defined before accessing ctx.url, in case this method is ever used in a context where ctx is missing.
test/fixtures/apps/subdir-services/app/service/ok.js (1)
7-9: Looks good.
The transition from a generator function to an async function seems clean and aligns with modern asynchronous patterns in JavaScript. No further issues found at this time.
test/lib/core/loader/load_boot.test.ts (3)
1-3: Imports look appropriate and consistent.
The assert, fs, and path imports are standard, and the MockApplication and createApp imports appear legitimate for this test file. No issues found here.
24-38: Validation of the boot sequence looks good.
Verifying the bootLog contents for both the app and the agent ensures correct lifecycle ordering (configDidLoad → didLoad → willReady → didReady → serverDidReady → beforeClose). This is thorough and helps protect against regressions in lifecycle hooks.
41-73: ESM tests parallel the CommonJS flow effectively.
Mirroring the CommonJS tests for ESM coverage is a good approach. This ensures that both module systems exhibit consistent application behavior and boot sequences. No issues found with the ESM test logic.
test/fixtures/apps/boot-app/app.js (3)
8-8: Good practice to assert essential properties.
Using assert(this.app.config) ensures config is present. Ensure you have tests or a robust fallback if config is missing.
31-31: Leverage logging best practices.
this.app.logger.info('app is ready'); is concise and clear. If more context is needed (e.g., environment information or config details), log them here to aid debugging.
2-2: Confirm Node.js version compatibility.
The node:timers/promises module generally requires Node.js v16+ to guarantee stable behavior. Confirm the project's Node.js version support or consider a polyfill if backward compatibility is necessary.
✅ Verification successful
Let me analyze the review comment and the results from the shell scripts.
The review comment raises a concern about Node.js version compatibility for node:timers/promises module. From the package.json output, we can see that the project requires:
"engines": {
"node": ">= 18.19.0"
}Since node:timers/promises module is stable in Node.js 16+ and the project already requires Node.js 18.19.0+, there is no compatibility issue with using this module.
Node.js version requirement is compatible with timers/promises module
The project's Node.js version requirement (>=18.19.0) is high enough to safely use the node:timers/promises module which requires Node.js 16+. No polyfill or backward compatibility handling is needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Node.js engine version from package.json if available
# Retrieve relevant Node.js engine info from the file or any references
rg --multiline --json '\"engines\"[^{]*{[^}]*}'
Length of output: 139
Script:
#!/bin/bash
# Search for package.json files and check their content for Node.js engine requirements
fd package.json --exec cat {} | grep -A5 '"engines"'
# Also check for .node-version or .nvmrc files that might indicate Node.js version
fd -e nvmrc -e node-version --exec cat {}
# Look for any direct mentions of Node.js version requirements in README or similar files
fd -e md | xargs grep -l -i "node" | xargs cat
Length of output: 130596
test/lib/core/config/config.test.ts (3)
1-2: Imports appear correct and consistent with project structure.
The usage of node:assert in strict mode, along with the local utilities, aligns with best practices for TypeScript tests.
12-15: Check that the test logic matches the intended configuration.
The assertions on app.config.name and app.config.logger.disableConsoleAfterReady appear correct for verifying the basic configuration.
5-10: Validate the application path 'apps/demo'.
Ensure that the 'apps/demo' directory exists and includes the necessary configuration files.
Below is a script to confirm the directory and config usage:
✅ Verification successful
The 'apps/demo' path is valid and properly configured
The test application path is correctly set up under test/fixtures/apps/demo/ with all necessary configuration files:
- Contains proper config files (
config.default.jsandconfig.unittest.js) - Has a valid application structure with
app.js,config/directory, and required configurations - The app is properly structured for testing purposes with mock functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for 'apps/demo' in the project's directories to confirm existence
fd demo | grep 'apps/demo'
Length of output: 51
Script:
#!/bin/bash
# Let's check the contents of the test/fixtures/apps/demo directory
# to ensure it has the necessary configuration files
ls -la test/fixtures/apps/demo/
# Also check if there are any config files
fd -t f "config" test/fixtures/apps/demo/
Length of output: 584
Script:
#!/bin/bash
# Let's check the content of the config files to ensure they are properly structured
cat test/fixtures/apps/demo/config/config.default.js
cat test/fixtures/apps/demo/config/config.unittest.js
# Also let's check the app.js to understand how the app is structured
cat test/fixtures/apps/demo/app.js
Length of output: 1609
test/lib/core/config/config.cookies.test.ts (3)
1-2: Imports follow the established project patterns.
Using node:assert in strict mode, alongside the MockApplication type from utils.js, adheres to the codebase’s evolving TypeScript conventions.
4-5: Descriptive test suite for cookies configuration.
Good job naming the test suite to reflect its purpose and transitioning it to TypeScript with explicit typing for app.
7-7: Confirm 'apps/app-config-cookies' directory and config setup.
Verify that 'apps/app-config-cookies' contains the relevant configuration for cookie behavior.
Use this script to confirm presence of the config directory:
✅ Verification successful
✓ Cookie configuration is properly set up in the test fixture directory
The directory test/fixtures/apps/app-config-cookies contains the necessary cookie configuration:
config/config.default.jsdefines cookie settings withsameSite: 'lax'- The app has a controller that demonstrates cookie functionality by setting a cookie named 'foo'
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
fd app-config-cookies | grep 'apps/app-config-cookies'
Length of output: 93
Script:
#!/bin/bash
# Let's check the contents of the app-config-cookies directory to verify cookie configuration
ls -R test/fixtures/apps/app-config-cookies/
# Also check for any config files that might contain cookie settings
find test/fixtures/apps/app-config-cookies/ -type f -name "config*" -o -name "*.json" -o -name "*.js" | xargs cat
Length of output: 690
test/lib/core/loader/config_loader.test.ts (5)
1-4: Great ES module conversion and TypeScript imports.
These updated imports improve type-safety and maintain a modern syntax for Node.js modules.
6-7: Good use of typed test fixtures.
Declaring let app: MockApplication enhances clarity and helps the compiler catch type errors earlier.
13-13: Standardized test setup with createApp.
Switching to createApp('apps/demo') for app instantiation aligns with the broader refactor and promotes consistency across tests.
22-25: Verified additions to coreMiddleware array.
Make sure these middleware entries exist and are tested. Otherwise, this array reference might lead to unexpected behavior if the middleware is not loaded.
32-34: Logger directory assertion looks correct.
It’s good to confirm that logs go to apps/demo/logs/demo under test conditions.
src/lib/egg.ts (2)
98-99: Backward-compatibility retained with HttpClientNext.
Using the same class reference ensures existing Egg.js 3.x plugins continue functioning.
137-137: Informational doc comment.
Renaming or aliasing Boot as BaseHookClass is consistent with the refactoring, ensuring documentation clarity.
test/lib/core/loader/load_service.test.ts (8)
1-3: Switch to ES module syntax is a good improvement
Using modern ES module imports here instead of CommonJS (require) helps bring consistency and future-proofing to the code. Great job!
5-6: Nicely typed MockApplication variable
Declaring app with the explicit type MockApplication improves type-checking and clarity around the application's behavior in the tests.
11-11: Ensure downstream references to createApp are updated
The move from utils.app() to createApp() is beneficial for clarity and type safety. Verify that all references and imports have been updated and correctly point to the new createApp function.
29-29: Confidence check on ES6 service loading
By calling createApp('apps/services_loader_verify'), you further test ES6 service loading. This is a systematic way to confirm the loader’s coverage. Great approach.
33-33: Property checks using every
Good job verifying that the required properties (bar) exist on app.serviceClasses.foo. This ensures a robust guard against incomplete or missing service properties.
38-38: createApp('apps/service-app') enhances clarity
Explicitly naming the test application directory improves readability and organization in your test cases. Well done.
51-51: Separate app declaration in nested describe block
Redeclaring app for the nested context is valid if you need a fresh instance. Ensure there's no unintentional duplication of hooks or code. Otherwise, this context-scoped approach is fine.
56-56: Consistent createApp usage
Maintaining a uniform pattern for app instantiation across tests fosters readability. This uniform method also simplifies future refactors.
test/lib/core/loader/load_plugin.test.ts (14)
8-8: EGG_BASE definition looks good.
No issues found in this line; it's clear and serves the purpose well.
14-14: Application creation is straightforward and correct.
This line cleanly instantiates the application with createApp; no issues found.
Line range hint 20-60: Plugin loading test is clear and comprehensive.
The assertions thoroughly verify the plugin configuration, including dependencies, paths, and version checks. Good usage of assert.deepEqual for objects.
64-72: Same-name plugin level sequence is validated properly.
Ensuring that app-level plugins override framework-level ones is well-tested. All looks good here.
86-94: Alias name support is confirmed effectively.
This test cleanly demonstrates alias usage by validating both d1 and !d. Nicely done.
108-116: Package.json config logic is well-tested.
No issues found. The environment-based approach to verify plugin properties is thorough.
Line range hint 149-167: Custom plugins config is tested thoroughly.
Defining plugins inline and ensuring the loader respects these definitions effectively covers this scenario.
191-193: Error handling for non-existent plugin is correct.
Using assert.rejects ensures proper validation of missing plugins.
204-206: Base directory existence check is properly enforced.
Again, assert.rejects captures the expected error seamlessly.
217-226: Plugin sorting test is solid.
Verifying the order of loaded plugins is essential, and this approach is well-implemented.
251-260: Recursive dependency detection is well-covered.
This ensures the loader correctly identifies cyclic references.
264-273: Missing plugin dependency check is thorough.
Verifying an error for missing a1 is accurate, and the error message is explicit.
Line range hint 277-296: Auto-fill plugin info test is complete.
Switching environments verifies the different plugin states. No issues observed.
317-350: Custom loader overrides are well demonstrated.
Subclassing AppWorkerLoader and AgentWorkerLoader to override loadPlugin is a neat demonstration of flexibility.
test/fixtures/apps/boot-app/agent.js (8)
2-2: Use of Node's built-in scheduler
Importing scheduler from node:timers/promises is a clean replacement for custom sleep/wait logic, reducing dependencies.
6-6: Constructor assignment
Assigning agent to this.agent is straightforward. Ensure consistency by referencing this.agent throughout.
8-8: Assert presence of config
Good practice in verifying that the config is defined before usage.
19-19: Wait logic in didLoad
Using await scheduler.wait(1) is a concise approach for delaying processing during the didLoad lifecycle.
24-24: Asynchronous lifecycle
The willReady step also leverages the wait logic, keeping the lifecycle events consistent.
29-29: Consistent asynchronous flow
Continuing the one-millisecond wait maintains consistent asynchronous event ordering across all lifecycle methods.
31-31: Logging improvements
Using this.agent.logger distinguishes agent logs from app logs and is consistent with the agent context.
35-35: Ensuring graceful close
The wait call in beforeClose similarly ensures any pending operations complete before final shutdown steps.
test/fixtures/apps/boot-app-esm/app.js (8)
1-3: ES module imports
Using assert and scheduler from Node built-ins, plus Boot from the local framework, is a clean approach and consistent with modern Node.js ESM usage.
6-9: Constructor best practices
Constructing app.bootLog for lifecycle trace is a neat approach. Also, verifying this.config ensures readiness for subsequent usage.
16-18: Lifecycle logging
configDidLoad usage is consistent with the Egg.js boot sequence. Tracking each phase in bootLog greatly aids debugging.
20-23: Asynchronous wait pattern
Leveraging scheduler.wait(1) is succinct. Confirm if 1 ms is simply symbolic or if it's needed for a genuine wait scenario.
25-28: Further lifecycle instrumentation
willReady uses the same wait pattern. It's consistent, though you might revisit time intervals if these are placeholders.
30-34: Application readiness
didReady logs to bootLog and references this.logger. Good practice to differentiate from agent.logger.
36-39: Clean shutdown
Capturing beforeClose to record lifecycle events is crucial. The wait ensures graceful handling of any last-minute operations.
41-44: Consistent final check
serverDidReady matches the rest of the lifecycle approach. Using an async wait technique ensures uniform code across the lifecycle.
test/fixtures/apps/boot-app-esm/agent.js (7)
1-3: ESM imports
Import statements are properly referencing Node's built-ins and your local Boot class. This layout is consistent and straightforward.
5-5: Extending Boot
Exporting CustomBoot as a class extends the existing Boot lifecycle structure, making it easier to trace the agent's lifecycle.
6-9: Constructor correctness
Calling super(agent) and using assert(this.config) ensures the agent’s configuration and messenger are set. Storing bootLog is consistent with other references in the codebase.
15-17: Configuration load phase
configDidLoad is consistent with Egg.js lifecycle steps. Logging into bootLog is a neat way to confirm the step was executed.
19-22: didLoad lifecycle
Utilizing scheduler.wait(1) is consistent with other steps. Ensure that 1ms wait is an intentional approach.
29-33: didReady log
this.agent.bootLog.push('didReady') pairs well with this.logger.info('agent is ready'). This approach ensures quick debugging and clarity in logs.
35-38: Graceful closure
beforeClose is crucial for disposing resources. It's consistent to mirror the same wait pattern as other lifecycle methods.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5379 +/- ##
==========================================
+ Coverage 88.36% 95.25% +6.88%
==========================================
Files 39 45 +6
Lines 3772 3979 +207
Branches 342 434 +92
==========================================
+ Hits 3333 3790 +457
+ Misses 439 189 -250 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
test/utils.ts (1)
93-93: Consider reducing the timeout durationWhile the change to use
scheduler.waitis good, a 10-second timeout might be too long for unit tests. Consider reducing this to improve test execution time.- await scheduler.wait(10000); + await scheduler.wait(5000);test/lib/core/messenger/ipc.test.ts (3)
7-9: **Unify variable declaration forapp**Here,
const app: any = {}is used, but later in the file, a variable namedappof typeMockApplicationis also declared (lines 56, 89, 115, 142, 169). Consider adjusting this block to use a consistent typed approach or rename one of the variables to avoid confusion.
39-39: **Avoid redeclaringmessengerin nested scopes **Here, a new
messengervariable is declared, shadowing the outermessengervariable. While it may not cause immediate problems, consider giving it a distinct name (e.g.,localMessenger) to improve clarity.Also applies to: 51-51
169-169: **Duplicate utility function definitions **
function countreappears in multiple blocks (lines 160, 187). Consider extracting it to a shared utility or referencing the same definition to avoid repetition.
Would you like me to propose a short patch for consolidating them into a single utility?Also applies to: 172-172, 187-187
test/lib/application.test.ts (1)
Line range hint
115-136: Shadowedappvariable declarations might be confusing.Multiple blocks declare
let app: MockApplication;. Consider using different identifiers or scoping to avoid overshadowing the same variable name in nested describes.test/lib/core/messenger/local.test.ts (1)
2-2: Remove unusedschedulerimport.As flagged by static analysis,
scheduleris imported but never used here. You can safely remove it:-import { scheduler } from 'node:timers/promises';🧰 Tools
🪛 eslint
[error] 2-2: 'scheduler' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
[failure] 2-2:
'scheduler' is defined but never used🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
[failure] 2-2:
'scheduler' is defined but never used🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
[failure] 2-2:
'scheduler' is defined but never used🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
[failure] 2-2:
'scheduler' is defined but never used🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
[failure] 2-2:
'scheduler' is defined but never used🪛 GitHub Check: Node.js / Test (macos-latest, 20)
[failure] 2-2:
'scheduler' is defined but never used
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
test/lib/application.test.ts(11 hunks)test/lib/core/messenger/ipc.test.ts(10 hunks)test/lib/core/messenger/local.test.ts(4 hunks)test/lib/egg.test.ts(1 hunks)test/utils.ts(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- test/lib/egg.test.ts
🧰 Additional context used
🪛 eslint
test/lib/core/messenger/local.test.ts
[error] 2-2: 'scheduler' is defined but never used.
(@typescript-eslint/no-unused-vars)
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 22)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 20)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18.19.0)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 GitHub Check: Node.js / Test (ubuntu-latest, 18)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 GitHub Check: Node.js / Test (macos-latest, 18.19.0)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 GitHub Check: Node.js / Test (macos-latest, 20)
test/lib/core/messenger/local.test.ts
[failure] 2-2:
'scheduler' is defined but never used
🪛 Biome (1.9.4)
test/lib/core/messenger/ipc.test.ts
[error] 96-96: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (18)
test/utils.ts (4)
8-10: LGTM: Import and type updates look good!
The changes improve type safety by:
- Using the modern
node:timers/promisesAPI - Adding proper typing with
MockClusterOptions
Also applies to: 26-26
45-48: LGTM: Function signature update improves type safety
The cluster function now correctly uses MockClusterOptions type, making it more type-safe while maintaining backward compatibility with string parameters.
Line range hint 69-124: LGTM: Well-structured error handling and cleanup
The implementation includes:
- Proper server cleanup on process exit
- Comprehensive error scenario handling
- Clear separation of concerns in endpoint handlers
8-8: Verify the impact of removing the old sleep implementation
Let's ensure no other test files are still using the old sleep implementation.
✅ Verification successful
Let me check the actual usage of sleep in these files to determine if they're using the old implementation or not.
No impact from sleep implementation change
The search results show that none of the files are importing or using the sleep implementation from test/utils.ts. Instead, they all have their own local implementations of sleep:
test/lib/plugins/logrotator.test.js: Uses a localsleepfunction defined with arrow function syntaxtest/fixtures/apps/async-app/app/service/api.js: Has its own localsleepfunction implementationtest/fixtures/apps/agent-app/app/router.js: Defines a localsleepfunction with a different signature for callback-style usage
Since there are no direct dependencies on the sleep implementation being changed in test/utils.ts, the changes are safe.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining usage of the old sleep implementation
rg -l "exports\.sleep|await sleep" --type ts --type js
Length of output: 183
Script:
#!/bin/bash
# Check the actual sleep usage in the identified files
echo "=== test/lib/plugins/logrotator.test.js ==="
rg "sleep" -C 2 test/lib/plugins/logrotator.test.js
echo -e "\n=== test/fixtures/apps/async-app/app/service/api.js ==="
rg "sleep" -C 2 test/fixtures/apps/async-app/app/service/api.js
echo -e "\n=== test/fixtures/apps/agent-app/app/router.js ==="
rg "sleep" -C 2 test/fixtures/apps/agent-app/app/router.js
# Also check if there's any direct import of sleep from utils.ts
rg "import.*sleep.*from.*utils" --type ts --type js
Length of output: 1448
test/lib/core/messenger/ipc.test.ts (9)
1-5: **Imports successfully transitioned to ES modules **
These import statements efficiently replace the old CommonJS-style requires. The partial usage of TypeScript features (e.g., MockApplication import) is consistent and appears correct.
12-12: **Instantiating Messenger with app **
This new usage reflects the updated constructor requirement. Ensure that any references to the old constructor usage (new Messenger()) have been removed or updated elsewhere in the codebase.
26-27: **Validate process.emit arguments in tests **
Emitting 'message' with arguments {}, null, etc. is a valid approach for testing edge cases. However, do confirm that these calls properly simulate real message structures within your environment.
Would you like a script to verify all process.emit('message', ...) calls to confirm they reflect expected usage?
Also applies to: 33-33
56-56: **Typed app variable usage **
Explicitly typing app as MockApplication is a positive step toward stronger TypeScript hygiene. No concerns here.
Also applies to: 61-61
71-71: **Enhanced test coverage for assigning PIDs **
This test ensures the assignment of PIDs is validated. Looks good!
110-110: **Assertion clarity **
assert.equal(m.length, 9); is straightforward and clear. Good job verifying your broadcast logic.
115-115: **Consistent creation of MockApplication **
Here, the cluster usage for tests with multiple workers is consistent with the previous blocks. No issues to flag.
Also applies to: 118-118
142-142: **Worker pool creation for apps/messenger-app-agent **
Looks good. No concerns. The typed app: MockApplication usage is consistent.
Also applies to: 145-145
160-160: **Enhanced type annotations on count **
Changing function count(data, key) to function count(data: string, key: string) is a welcome safety improvement in TypeScript.
test/lib/application.test.ts (3)
1-9: Use of new ES Modules imports looks good.
No issues found in this updated import block. The scheduler import is indeed used later in this file.
226-236: Thorough test for CookieLimitExceedError.
The test coverage here is clear and verifies the error’s properties well.
242-242: Migration to pending() function is consistent.
The switch to pending from pedding for concurrency appears correct. Good job keeping this consistent for multi-callback tests.
Also applies to: 258-258
test/lib/core/messenger/local.test.ts (2)
24-24: Asynchronous test callback usage with pending(...).
These changes correctly switch to the pending function for concurrency in tests.
Also applies to: 38-38, 138-138
8-8: Add type annotations for clarity.
Declaring let app: MockApplication; helps ensure type safety within the tests.
| @@ -84,14 +86,14 @@ describe('test/lib/core/messenger/ipc.test.js', () => { | |||
| }); | |||
|
|
|||
| describe('broadcast()', () => { | |||
| let app; | |||
| let app: MockApplication; | |||
There was a problem hiding this comment.
**Duplicate before() hooks in the same describe block **
Static analysis flags the presence of two separate before hooks (lines 89 and 96). Merging them into a single hook or splitting them into beforeAll and beforeEach more clearly states which setup steps will run once vs. before every test.
describe('broadcast()', () => {
let app: MockApplication;
before(() => {
mm.env('default');
app = cluster('apps/messenger-broadcast', { workers: 2 });
app.coverage(false);
return app.ready();
});
- before(() => scheduler.wait(1000));
+
+ // Option A: Merge into the existing `before` if it should run exactly once
+ // Option B: Convert to `beforeEach` if it should run before each test
});Also applies to: 92-92, 96-96
| }); | ||
| }); | ||
|
|
||
| describe('app start timeout', function() { | ||
| afterEach(() => app.close()); | ||
| it('should emit `startTimeout` event', function(done) { | ||
| app = utils.app('apps/app-start-timeout'); | ||
| app = createApp('apps/app-start-timeout'); |
There was a problem hiding this comment.
💡 Codebase verification
Several test files still use utils.app and need to be migrated to createApp
Based on the search results, there are multiple test files that still use the older utils.app pattern instead of createApp:
- test/ts/index.test.js
- test/lib/plugins/onerror.test.js
- test/lib/plugins/multipart.test.js
- test/lib/plugins/static.test.js
- test/lib/plugins/security.test.js
- test/lib/plugins/session.test.js
- test/lib/cluster/cluster-client.test.js
- test/lib/cluster/cluster-client-error.test.js
- test/lib/plugins/logrotator.test.js
- test/lib/plugins/development.test.js
- test/lib/plugins/depd.test.js
- test/lib/plugins/i18n.test.js
These files should be updated to use createApp for consistency with the rest of the TypeScript codebase.
🔗 Analysis chain
Confirm all createApp call sites are updated to TypeScript.
If there were any older call sites using utils.app, please ensure they have been replaced by createApp to maintain consistency. Consider running:
Also applies to: 52-52, 69-69, 86-86, 103-103
Let me verify if these utils.app call sites have corresponding TypeScript files by checking their extensions and looking for TypeScript alternatives.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for older usage of utils.app
rg -A 3 'utils\.app'
Length of output: 3232
Script:
#!/bin/bash
# Check for TypeScript files with similar names
fd -e ts "onerror.test.ts|multipart.test.ts|static.test.ts|security.test.ts|session.test.ts|cluster-client.test.ts|cluster-client-error.test.ts|logrotator.test.ts|development.test.ts|depd.test.ts|i18n.test.ts" test/
# Also check if there's a createApp usage in TypeScript files
rg -A 3 'createApp\(' -g '*.ts'
Length of output: 28286
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (19)
test/fixtures/apps/schedule/app/schedule/sub/cron.js (1)
6-8: Double-check if async is truly needed here
Since the task currently only executes a logger statement, verify whether you need any asynchronous operations or error handling. If future enhancements involve asynchronous calls (e.g., DB queries, network requests), retaining async is beneficial. Otherwise, removing the async keyword can simplify the code.test/lib/plugins/schedule.test.ts (3)
15-15: Consider extracting the 7000ms timeout into a configurable constant.
Using a hardcoded 7-second delay can slow down tests unnecessarily or cause flakiness on slow machines. Making this value configurable or using a more deterministic strategy can improve test reliability.- await scheduler.wait(7000); + const WAIT_TIME_MS = 7000; + await scheduler.wait(WAIT_TIME_MS);
27-30: Use of synchronous I/O for reading logs in tests.
fs.readFileSyncis acceptable in a test environment due to its simplicity. However, if these tests grow more complex, consider using asynchronous file operations to avoid blocking.
32-34: String matching approach is flexible but can be fragile.
Reliance on textual matching for assertions is often brittle. If feasible, consider using structured log formats (e.g., JSON) or a more robust parsing approach for better maintainability.src/app/extend/context.types.ts (1)
20-22: Consider adding JSDoc comments for better documentation.These properties would benefit from JSDoc comments explaining their purpose and usage patterns.
Add documentation like this:
+ /** + * Default logger instance for application logging + */ get logger(): EggLogger; + /** + * Logger instance for framework-level logging + */ get coreLogger(): EggLogger; + /** + * Request-scoped storage for passing data between middlewares + */ get locals(): Record<string, any>;test/lib/plugins/session.test.ts (1)
28-28: Recommend defining a clearer type for theset-cookieheader.Casting the
set-cookieheader toanyand then usingjoin(';')is acceptable as a quick fix, but it’s safer to define a more precise type or interface. This helps avoid potential future type-related issues.test/fixtures/apps/multipart/app/controller/upload.js (3)
5-8: Consider additional error handling for multipart iteration.Using
for await (const part of parts)is a clean and modern approach. However, if an error occurs during the multipart parsing process (e.g., due to a network interruption or invalid request), the current implementation may not provide adequate feedback or handle the error gracefully. Consider augmenting this code to capture potential exceptions thrown bythis.multipart()or the iteration, and respond with a more detailed error message to the client.
18-18: Optional validation against an empty or invalid file.The condition
if (!filePart || !filePart.filename)appropriately checks for the existence of a file. Depending on your needs, consider adding content-type or size checks if you want to ensure the user has provided a valid or non-empty file.
26-26: Handle streaming errors for file saving.While
filePart.pipe(ws)will stream the file to disk, the current approach does not listen forerrororfinishevents onws. If there's an error writing to disk, the user might not get an appropriate error notification.filePart.pipe(ws) +ws.on('error', err => { + this.logger.error('Write stream error: ', err); + this.status = 500; + this.body = { message: 'File upload failed.' }; +}); +ws.on('finish', () => { + this.logger.info('File write finished successfully.'); +});test/lib/plugins/watcher.test.ts (3)
5-5: Consider TypeScript consistency for utility modules.
Theutils.jsfile is imported in a TypeScript test. For complete type-safety, consider convertingutils.jstoutils.tsor using type declarations to eliminate potential mismatches between TypeScript and JavaScript modules.
41-41: Avoid potential flakiness with time-based waits.
The repeatedscheduler.wait(5000)combined with file modifications can cause slow or flaky tests if the file watch events take longer or shorter to trigger. Consider a more event-driven or condition-based approach (e.g., polling a variable, hooking into watchers) instead of waiting a fixed amount of time.Also applies to: 53-53
90-90: Ensure robust error handling for file reads.
Reading logs withfs.readFileSync(logPath, 'utf8')is fine for tests, but consider gracefully handling non-existent or locked files to reduce test fragility in CI environments.test/lib/plugins/depd.test.ts (1)
10-10: Consider using an async function for the lifecycle hook.While
return app.ready()is acceptable, using an async function withawait app.ready()can enhance clarity and consistency in asynchronous test setup.- before(() => { - app = createApp('apps/demo'); - return app.ready(); - }); + before(async () => { + app = createApp('apps/demo'); + await app.ready(); + });test/lib/plugins/static.test.ts (1)
1-1: Consider renaming.jsto.tsfor consistency
Although it is valid to import from a.jsfile, consider renaming../../utils.jsto a TypeScript file to maintain consistency in a TypeScript codebase if the file is actually TypeScript.test/lib/plugins/logrotator.test.ts (1)
1-27: Ensure log rotation test timing is reliable
This test waits for 1 second after running the schedule to allow log rotation. While it may generally work, consider using a more robust synchronization mechanism (such as event-based signaling) to ensure that the rotation completes regardless of timing variations.test/lib/plugins/multipart.test.ts (2)
17-27: Consider unifying thesebeforehooks.
Having twobeforehooks is valid in Mocha, but static analysis flagged it as a potential duplication. If desired, you can merge them into a singlebeforehook for clarity.before(() => { app = createApp('apps/multipart'); return app.ready(); -}); - -before(done => { +}).before(done => { server = app.listen(); request(server) .get('/')🧰 Tools
🪛 Biome (1.9.4)
[error] 17-27: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
34-60: Multipart upload test logic looks good overall.
Verifies correct file handling and form fields, including CSRF token usage. For completeness, consider adding negative test cases (e.g., missing file or invalid_csrf) if they are relevant to the uploaded endpoint’s logic.test/fixtures/apps/watcher-development-app/agent.js (1)
3-4: Consider using camelCase for variable namingWhile
file_path1anddir_pathare functional, JavaScript codebases often use camelCase, e.g.,filePath1ordirPath. Unless your team guidelines mandate snake_case, consider aligning with common naming conventions for consistency.test/fixtures/apps/watcher-development-app/app/router.js (1)
3-4: Minor naming style suggestionLike in
agent.js, consider using camelCase forfile_path1anddir_pathto match typical JavaScript conventions and improve consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
package.json(3 hunks)src/app/extend/context.types.ts(1 hunks)test/fixtures/apps/multipart/app/controller/upload.js(2 hunks)test/fixtures/apps/multipart/config/config.default.js(0 hunks)test/fixtures/apps/schedule/app/schedule/sub/cron.js(1 hunks)test/fixtures/apps/schedule/config/config.default.js(0 hunks)test/fixtures/apps/watcher-development-app/agent.js(1 hunks)test/fixtures/apps/watcher-development-app/app/router.js(1 hunks)test/fixtures/apps/watcher-development-app/config/config.unittest.js(0 hunks)test/lib/core/messenger/local.test.ts(14 hunks)test/lib/plugins/depd.test.ts(1 hunks)test/lib/plugins/development.test.ts(2 hunks)test/lib/plugins/i18n.test.ts(1 hunks)test/lib/plugins/logrotator.test.js(0 hunks)test/lib/plugins/logrotator.test.ts(1 hunks)test/lib/plugins/multipart.test.js(0 hunks)test/lib/plugins/multipart.test.ts(1 hunks)test/lib/plugins/onerror.test.ts(1 hunks)test/lib/plugins/schedule.test.js(0 hunks)test/lib/plugins/schedule.test.ts(1 hunks)test/lib/plugins/security.test.ts(3 hunks)test/lib/plugins/session.test.ts(2 hunks)test/lib/plugins/static.test.ts(1 hunks)test/lib/plugins/watcher.test.ts(5 hunks)
💤 Files with no reviewable changes (6)
- test/fixtures/apps/schedule/config/config.default.js
- test/fixtures/apps/multipart/config/config.default.js
- test/fixtures/apps/watcher-development-app/config/config.unittest.js
- test/lib/plugins/logrotator.test.js
- test/lib/plugins/multipart.test.js
- test/lib/plugins/schedule.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- package.json
- test/lib/core/messenger/local.test.ts
🧰 Additional context used
🪛 Biome (1.9.4)
test/lib/plugins/multipart.test.ts
[error] 17-27: Disallow duplicate setup and teardown hooks.
Disallow before duplicacy inside the describe function.
(lint/suspicious/noDuplicateTestHooks)
🔇 Additional comments (35)
test/lib/plugins/schedule.test.ts (3)
1-5: Imports look clean and organized.
These imports are straightforward and align well with modern Node.js best practices. Using node:-prefixed modules clarifies that these are built-in.
7-8: Appropriate test block naming convention.
Descriptive test suite and test block names help maintain clarity. This naming pattern serves as a good example for consistent test organization.
9-10: Validate concurrency in multi-worker testing.
Spawning multiple workers for testing is a good practice, but ensure that the tested logic is thread-safe and that concurrency issues (e.g., race conditions) are properly addressed.
Would you like a script to search for potential concurrency or synchronization issues across the codebase?
src/app/extend/context.types.ts (1)
20-22: LGTM! Verify implementation alignment.
The type definitions for the new getters are well-structured and follow TypeScript best practices. They complement the existing logger functionality and add request-scoped storage capability.
Let's verify the implementation of these getters:
✅ Verification successful
Let me try a different search strategy to find the implementations.
Let's check the actual implementations in the main context files.
Implementation verified and properly aligned with type definitions
The type definitions in context.types.ts are correctly implemented in the codebase:
loggergetter is implemented insrc/app/extend/context.tsand returns the application logger viagetLogger('logger')coreLoggergetter is implemented insrc/app/extend/context.tsand returns the core logger viagetLogger('coreLogger')localsgetter/setter is implemented insrc/app/extend/context.tswith proper merging of app.locals and request-scoped locals
All implementations match their type definitions and include proper documentation. The code follows best practices with symbol-based private properties and efficient caching mechanisms.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the implementation of the new getters in the context class
# Search for logger getter implementation
echo "Searching for logger getter implementation..."
ast-grep --pattern 'get logger() {
$$$
}'
# Search for coreLogger getter implementation
echo "Searching for coreLogger getter implementation..."
ast-grep --pattern 'get coreLogger() {
$$$
}'
# Search for locals getter implementation
echo "Searching for locals getter implementation..."
ast-grep --pattern 'get locals() {
$$$
}'
Length of output: 452
Script:
#!/bin/bash
# Search for files that might contain the implementations
echo "Searching for potential implementation files..."
fd -e js -e ts context
# Search for logger-related implementations
echo -e "\nSearching for logger implementations..."
rg -l "logger" $(fd -e js -e ts context)
# Search for locals-related implementations
echo -e "\nSearching for locals implementations..."
rg -l "locals" $(fd -e js -e ts context)
Length of output: 2392
Script:
#!/bin/bash
# Check the implementation in src/app/extend/context.ts
echo "Content of src/app/extend/context.ts:"
cat src/app/extend/context.ts
# Check base context implementation
echo -e "\nContent of src/lib/core/base_context_class.ts:"
cat src/lib/core/base_context_class.ts
# Check logger implementation
echo -e "\nContent of src/lib/core/base_context_logger.ts:"
cat src/lib/core/base_context_logger.ts
Length of output: 10507
test/lib/plugins/session.test.ts (3)
1-3: Nice transition to ES modules and updated imports.
Switching from require to import syntax improves maintainability and aligns with modern TypeScript best practices. This looks good!
5-6: Good addition of explicit type for the app variable.
Declaring let app: MockApplication; clarifies the expected type and can help avoid potential runtime errors.
8-8: Ensure test environment alignment with 'apps/koa-session'.
Verify that 'apps/koa-session' is the correct fixture path or configuration name. This step ensures the new typed test still references the correct environment.
test/fixtures/apps/multipart/app/controller/upload.js (1)
28-28: Minor note on final response payload.
Setting this.body with { filename: filePart.filename, fields } is straightforward and clear. Ensure that any additional file metadata (like size or MIME type) is validated or sanitized if exposed in production scenarios.
test/lib/plugins/watcher.test.ts (5)
1-4: Great transition to ESM imports!
These new ES module imports (import { strict as assert } from 'node:assert', etc.) are consistent with modern JavaScript/TypeScript practices.
7-9: Validate filepaths in tests.
We are creating multiple file paths (file_path1, file_path2, file_path1_agent) for watch triggers. Ensure these directories exist before writing files to prevent test failures if a path is missing or incorrectly spelled.
13-13: Explicitly typed variable is excellent for clarity.
Declaring let app: MockApplication; improves readability and type safety. Keep ensuring that all important variables in tests are typed explicitly.
15-15: Potential for parallel tests.
The cluster('apps/watcher-development-app') approach is great but keep in mind that starting multiple clusters in parallel test runs could cause port collisions or concurrency issues if run concurrently. Consider verifying concurrency constraints.
79-79: Consistent approach to test setup is good.
Reusing the same cluster approach ensures consistency. Just confirm that starting up multiple watchers does not conflict with other test files if they run in parallel.
Also applies to: 81-81
test/lib/plugins/depd.test.ts (3)
1-3: Imports are clear and consistent with TypeScript evolution.
The shift to ESM imports and TypeScript aligns with modern best practices. Good job transitioning from the CommonJS style.
5-5: Title and naming in describe block look sound.
Everything is consistent, and the test structure remains unchanged while migrating to TypeScript.
8-8: Using a typed variable for the application is a great improvement.
Declaring app as MockApplication strengthens safety and clarity in the tests.
test/lib/plugins/static.test.ts (1)
4-4: Improved type safety
Declaring app as MockApplication is a good practice, ensuring that type checks can catch potential issues early.
test/lib/plugins/development.test.ts (3)
1-4: ES module imports upgrade
Using ES module syntax with node: built-in modules and the createApp, cluster functions is clean. Ensure any tooling configurations (e.g., tsconfig, Babel) support these imports.
52-52: Cluster usage
Leveraging cluster('apps/reload-worker') is appropriate for hot-reload testing. Ensure that the cluster is properly closed and cleaned to prevent test flakiness or leftover processes.
14-14: Consistency in environment mocking
You're mocking environment variables and log configuration. Confirm that these remain restored or properly reset in other tests to avoid cross-test contamination.
✅ Verification successful
Environment mocks are properly restored across tests
The verification shows that environment mocks are consistently restored using mm.restore() in afterEach hooks across all test files, including the file in question (test/lib/plugins/development.test.ts). This practice prevents test contamination by cleaning up mocks after each test case. Specifically:
- The file under review properly implements
afterEach(mm.restore)at the test suite level - This pattern is consistently applied across the entire test suite, as evidenced by similar implementations in:
- Other plugin tests (security.test.ts, onerror.test.ts, watcher.test.ts)
- Core component tests (egg.test.ts, application.test.ts, logger.test.ts)
- Middleware tests (notfound.test.ts)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
#!/bin/bash
# Searching for lines that restore environment mocks across test files to verify they are applied consistently.
rg -A 3 "mm.restore"
Length of output: 7196
test/lib/plugins/i18n.test.ts (2)
1-1: Appropriate typed import
Importing MockApplication, createApp is consistent with other test files and provides clarity in the TypeScript environment.
6-6: Validate locales
Ensure that 'apps/i18n' includes the required locale files and default locale configurations for these tests to pass consistently in all environments.
✅ Verification successful
Locale files and configurations are properly set up
The test fixture apps/i18n includes all necessary locale files and configurations:
- Locale files are present in
config/locales/with both JS (zh-CN.js) and JSON (de.json) formats - Translation strings are properly defined in each locale file
- Basic configuration is set up in config.default.js
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Searching for files in the i18n directory to ensure locales exist
fd i18n --type d
Length of output: 43
Script:
#!/bin/bash
# Let's check the content structure of the i18n test app
fd . test/fixtures/apps/i18n/ -t f
Length of output: 482
Script:
#!/bin/bash
# Let's check the content of config.default.js to verify locale configuration
cat test/fixtures/apps/i18n/config/config.default.js
# Also check the content of locale files
echo "=== zh-CN.js ==="
cat test/fixtures/apps/i18n/config/locales/zh-CN.js
echo -e "\n=== de.json ==="
cat test/fixtures/apps/i18n/config/locales/de.json
Length of output: 714
test/lib/plugins/multipart.test.ts (4)
1-6: Imports look consistent and TypeScript-friendly.
All required modules are properly imported. The use of node:assert is aligned with modern Node.js best practices.
7-12: Good use of typed variables.
Declaring app: MockApplication; helps ensure type safety across tests.
13-16: Separate hook for app creation.
Calling createApp() inside the before hook for initialization is a standard approach, ensuring everything is set up before the server starts listening.
29-32: Consistent teardown strategy.
Closing both server and app in the after hook ensures all resources are correctly released, preventing test bleed over.
test/lib/plugins/security.test.ts (5)
2-3: Good import updates and type usage.
Switching to ES module syntax and using MockApplication fosters better type safety and code clarity.
5-5: Test suite structure is clear.
The top-level describe clearly indicates the file under test and its purpose.
9-11: CSRF test coverage is well-organized.
Instances are set to different modes (csrf-disable vs. csrf-enable) to validate behavior under various configurations.
29-31: Consistent approach for multiple test scenarios.
You’re ensuring each scenario has its own app instance, which makes debugging simpler if tests fail.
46-48: Clear tests for partial CSRF disabling.
Verifies that paths under /api/ are skipped from checks while maintaining coverage on other routes.
test/lib/plugins/onerror.test.ts (2)
1-2: Imports modernized for ES modules.
Using @eggjs/mock and local createApp aligns with the broader transition to TypeScript.
4-9: Environment variable usage and typed application.
Ensuring EGG_LOG = 'NONE' may help suppress logs during tests. The typed MockApplication fosters better validation and standardization.
test/fixtures/apps/watcher-development-app/agent.js (1)
1-1: Good use of the native Node.js path module
Using node:path is a clean, straightforward approach that avoids relying on external utilities for path handling.
test/fixtures/apps/watcher-development-app/app/router.js (1)
1-1: Aligned path usage with agent.js changes
Switching to node:path keeps the approach consistent across your codebase.
| before(() => { | ||
| app = utils.app('apps/static-server'); | ||
| app = createApp('apps/static-server'); |
There was a problem hiding this comment.
💡 Codebase verification
The app directory exists but with a different path structure
The test is using 'apps/static-server' but the actual directory is located at test/fixtures/apps/static-server/. The directory exists and has a valid structure with app, config, and necessary files, but:
- The path in the test should be updated to use the correct relative path:
'test/fixtures/apps/static-server' - Note that while this is a test for TypeScript, the fixture itself uses JavaScript files which is common in testing scenarios
🔗 Analysis chain
Verify the app directory structure
Confirm that the 'apps/static-server' folder exists and is correctly structured for a TypeScript test environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verifying the existence of 'apps/static-server'
fd static-server --type d
Length of output: 61
Script:
#!/bin/bash
# Let's check the structure of the static-server directory
ls -R test/fixtures/apps/static-server/
# Also verify if it contains TypeScript files
fd -e ts -p test/fixtures/apps/static-server/
Length of output: 317
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (23)
src/lib/type.ts (1)
334-335: New request body type introduced.Defining a flexible
RequestObjectBodytype (Record<string, any>) improves clarity for request body structures. Consider restricting theanyusage if your use cases are well-defined to increase type safety in the long term.test/fixtures/apps/app-ts/app/extend/context.ts (1)
1-7: Use caution with deep relative imports for type declarations.
You're importingContextfrom a relatively deepsrc/index.jspath, which might complicate refactoring if the file structure changes. Consider referencing a type definition (.d.ts) or a top-level TypeScript export if available, for better maintainability. Otherwise, this updated import path looks fine, and usingthis: Contextwithin thetestfunction is consistent with Egg’s approach to context-aware functions.src/app/extend/request.types.ts (1)
4-4: Use a more specific type instead ofanyforbody.
If possible, define a more constrained type forbodyto leverage compile-time checks and reduce runtime errors. Even a generic<T = unknown>approach might be safer than plainany.test/fixtures/apps/app-ts/app/service/foo.ts (1)
1-2: Consider removing the commented-out import if it's no longer needed.The replaced import suggests that you're now referencing a local
Serviceinstead of the one fromegg. If this is your intended approach, removing the commented line clarifies the code. Otherwise, ensure you still rely on the officialeggdefinitions as appropriate.test/fixtures/apps/app-ts/app/router.ts (1)
1-2: Remove or update the commented-out import for clarity.Similar to other refactors, if this is the desired import path, consider removing the old import reference from
eggto avoid confusion.test/fixtures/apps/app-ts/app/middleware/generic_ctx.ts (2)
1-2: Remove or replace commented-out import if not needed.If
EggContextis now your canonical import, remove the commented line to maintain cleanliness and clarity in the file.
13-13: Avoid casting toanywhen possible.Casting
ctx.bodytoanyundermines type safety and can mask potential errors at compile time. Consider defining a more precise type forbaror ensuring the existing typed interface is retained.src/lib/egg.types.ts (1)
9-13: Validate newly exposed logging properties.The new
logger,coreLogger, andgetLogger(name: string)methods give extended logging abilities. Confirm that your logs handle concurrency correctly and that the chosen naming conventions don't collide with existing loggers.test/fixtures/apps/app-ts/lib/export-class.ts (1)
1-2: Maintain consistency in import paths.
Similar to the logger file, the import from../../../../../src/index.jscan make the project structure more fragile. Confirm that referencing the local path is intended and that no alias or simpler path resolution is preferable.test/lib/cluster/cluster-client-error.test.ts (1)
9-18: Logical error check for error presence.This block ensures an error is thrown and captured. You might consider specifying the expected message or error type if known, for more precise testing.
src/lib/core/httpclient.ts (2)
7-7: Consider stricter type thananyfor better type safety
ReplacingContextDelegationorunknownwithanygrants less type safety and may mask type errors. If possible, define an interface or use a more specific type to maintain clarity.
15-16: Re-evaluatectxandtracertype usage
Though usinganycan be a pragmatic approach during refactor, you may want a known interface or type definition forctxandtracerto ensure consistency across the codebase.test/index.test-d.ts (1)
17-17: No issues withctx.request.body
Confirm that theanytype used forctx.request.bodyis intended. If you are certain about the shape of the request body, consider using a more specific type.test/fixtures/apps/app-ts/app/controller/foo.ts (1)
106-118: Avoid accumulating commented-out methods.
IftestViewRender()andtestViewRenderString()are no longer required, consider removing them to keep the code clean. If you intend to restore them, consider marking them with aTODO:comment or adding a brief explanation.test/ts/index.test.ts (2)
1-5: Consider removing or re-enabling commented imports.
The commented-out imports (e.g.,coffee,importResolve) might cause confusion. Delete them if they’re not needed or restore them if the tests for TypeScript compilation should be active.
9-36: Use async/await in tests for cleaner flow.
You’re usingdonecallbacks, which can be replaced with async/await for improved readability and reduced callback usage. For example:-it('controller run ok', done => { - app.httpRequest() - .get('/foo') - .expect(200) - .expect({ env: 'unittest' }) - .end(done); -}); +it('controller run ok', async () => { + await app.httpRequest() + .get('/foo') + .expect(200) + .expect({ env: 'unittest' }); +});src/app/extend/request.ts (1)
19-23: Provide stricter typing forbody.
Declaringbodyasanycan hide potential type issues. If feasible, define a more specific interface to improve maintainability and type-checking benefits.src/app/extend/context.ts (1)
34-36: Avoid usinganyforproxy.
If theproxyproperty has a known shape or interface, consider defining a more precise type to enhance type safety.test/lib/cluster/master.test.ts (2)
172-178: Multiple apps in one server
Initializingapp1andapp2is a good approach to verifying concurrency and port allocation.
Consider adding negative tests (e.g., handling port collisions, if any) for completeness.
278-280: Process spawn test
Utilizing thecoffee.spawn(...)approach is effective, but the comment clarifies thatendwon’t emit.
Consider converting to a pair of events or early termination to handle potential concurrency issues in CI.src/lib/core/base_context_class.ts (1)
Line range hint
2-15: Context and application references
Transition fromContextDelegationtoContext, plus the additions ofappandservice, is consistent with the new typings.
The index signature[key: string | symbol]: any;is flexible but could potentially hide type mismatches — consider limiting usage if feasible.src/index.ts (1)
21-22: Consider using explicit exports for error types.Using
export *could potentially expose internal error types that weren't meant to be part of the public API. Consider explicitly listing the error types that should be exposed.-export * from './lib/error/index.js'; +export { + CookieLimitExceedError, + MessageUnhandledRejectionError, + // ... other public error types +} from './lib/error/index.js';src/lib/egg.ts (1)
453-453: Use optional chaining for cleaner error message access.The current error message access can be simplified using optional chaining.
-this.coreLogger.error('[egg:unhandledRejection] %s', err && err.message || err); +this.coreLogger.error('[egg:unhandledRejection] %s', err?.message ?? err);🧰 Tools
🪛 Biome (1.9.4)
[error] 453-453: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
test/fixtures/apps/app-ts/node_modules/egg/index.jsis excluded by!**/node_modules/**test/fixtures/apps/app-ts/node_modules/egg/package.jsonis excluded by!**/node_modules/**
📒 Files selected for processing (40)
package.json(5 hunks)src/app/extend/context.ts(5 hunks)src/app/extend/context.types.ts(2 hunks)src/app/extend/request.ts(2 hunks)src/app/extend/request.types.ts(1 hunks)src/app/extend/response.types.ts(1 hunks)src/app/middleware/meta.ts(1 hunks)src/app/middleware/notfound.ts(1 hunks)src/app/middleware/site_file.ts(2 hunks)src/index.ts(3 hunks)src/lib/application.ts(4 hunks)src/lib/core/base_context_class.ts(2 hunks)src/lib/core/context_httpclient.ts(1 hunks)src/lib/core/httpclient.ts(2 hunks)src/lib/egg.ts(7 hunks)src/lib/egg.types.ts(1 hunks)src/lib/type.ts(5 hunks)test/fixtures/apps/app-ts/app.ts(1 hunks)test/fixtures/apps/app-ts/app/controller/foo.ts(3 hunks)test/fixtures/apps/app-ts/app/extend/context.ts(1 hunks)test/fixtures/apps/app-ts/app/extend/helper.ts(1 hunks)test/fixtures/apps/app-ts/app/middleware/default_ctx.ts(1 hunks)test/fixtures/apps/app-ts/app/middleware/generic_ctx.ts(1 hunks)test/fixtures/apps/app-ts/app/middleware/test.ts(1 hunks)test/fixtures/apps/app-ts/app/router.ts(1 hunks)test/fixtures/apps/app-ts/app/service/foo.ts(1 hunks)test/fixtures/apps/app-ts/config/config.ts(1 hunks)test/fixtures/apps/app-ts/lib/export-class.ts(2 hunks)test/fixtures/apps/app-ts/lib/logger.ts(1 hunks)test/fixtures/apps/app-ts/package.json(1 hunks)test/fixtures/apps/app-ts/tsconfig.json(1 hunks)test/fixtures/apps/cluster_mod_app/agent.js(0 hunks)test/fixtures/apps/cluster_mod_app/app.js(1 hunks)test/index.test-d.ts(2 hunks)test/lib/cluster/cluster-client-error.test.js(0 hunks)test/lib/cluster/cluster-client-error.test.ts(1 hunks)test/lib/cluster/cluster-client.test.ts(2 hunks)test/lib/cluster/master.test.ts(9 hunks)test/ts/index.test.js(0 hunks)test/ts/index.test.ts(1 hunks)
💤 Files with no reviewable changes (3)
- test/fixtures/apps/cluster_mod_app/agent.js
- test/lib/cluster/cluster-client-error.test.js
- test/ts/index.test.js
✅ Files skipped from review due to trivial changes (3)
- test/fixtures/apps/app-ts/app/middleware/test.ts
- test/fixtures/apps/app-ts/config/config.ts
- test/fixtures/apps/app-ts/app/middleware/default_ctx.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/app/extend/context.types.ts
- src/lib/application.ts
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/egg.ts
[error] 453-453: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (74)
src/lib/type.ts (4)
21-21: Immediate clarity improvement forIgnoreItem.Using
Contextin the callback parameter name aligns it with the broader Egg.js convention and enhances code clarity.
12-12: Switch from ContextDelegation to Context.This change is consistent with the rest of the code where
Contextis used. Verify that all references or imports ofContextDelegationin your codebase have been removed or replaced to avoid confusion.✅ Verification successful
No remaining references to ContextDelegation found
The search results confirm that there are no remaining references to
ContextDelegationin the codebase. The change to useContextis consistent with the rest of the codebase, as evidenced by multiple imports and uses ofContextthroughout the project. The typeEggContextis properly aliased toContextinsrc/lib/egg.ts, showing this is the standard type being used across the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Attempt to locate any remaining references to ContextDelegation. # If there are any, consider removing or replacing them. rg "ContextDelegation" --context 3Length of output: 34
Script:
#!/bin/bash # Let's try a case-insensitive search to be thorough rg -i "contextdelegation" --context 3 # Also search for any potential import statements related to Context rg "import.*Context" --context 3 # Use ast-grep to find any type declarations or interfaces using Context ast-grep --pattern 'interface $_ extends Context' ast-grep --pattern 'type $_ = Context'Length of output: 11619
106-106: Ensure theonerrorsignature matches middleware usage.The method signature now uses
Context; confirm that upstream callers and middleware references are usingContextcorrectly.✅ Verification successful
Let me gather more information about the usage of
onerrorin the codebase.
Let me check one more thing to understand the implementation details.
Based on the gathered information, I can now provide the final response:
The
onerrorsignature change appears to be consistent with the codebaseThe
onerrormethod with signatureonerror(err: any, ctx: Context)is properly defined in the type interface and is used consistently across the codebase. Key findings:
- The Context type is properly imported and used throughout the codebase
- The implementation in
src/config/config.default.tsmatches the signature- The method is correctly bound to the context in
src/lib/egg.ts- The error handling is integrated with the egg-onerror plugin
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify usage of onerror method is consistent with the new signature. rg -A 3 "onerror\(err: any, ctx: Context\)"Length of output: 164
Script:
#!/bin/bash # Search for onerror implementations and usages rg -A 5 "onerror" --type ts # Find Context type imports and usage rg "import.*Context" --type tsLength of output: 7748
Script:
#!/bin/bash # Check for actual onerror implementations and its usage in the context ast-grep --pattern 'onerror($$$) { $$$ }'Length of output: 43
139-142: Monitor deprecation usage.The
useHttpClientNextproperty is marked as deprecated. Ensure that applications still using this property in Egg 3.x can gracefully migrate without breakage.If you need to maintain backward compatibility, consider documenting the recommended migration approach or providing a fallback mechanism to avoid confusion for end-users.
✅ Verification successful
Let me gather more information about the migration and deprecation context.
Let me check the actual implementation and usage in the codebase to understand the migration path better.
Migration path for
useHttpClientNextis well documented and testedBased on the evidence found in the codebase:
- The property was introduced to support urllib@3 migration as documented in issue [RFC] Better http lib: undici #4847
- There are comprehensive test cases showing both the old and new behavior
- The migration path is clear: users can opt-in to urllib@3 by setting
config.httpclient.useHttpClientNext = true- The default configuration in app-ts shows
falseas the default value, ensuring backward compatibility- Test fixtures demonstrate successful usage in both scenarios
The deprecation is well managed with:
- Clear documentation in the type definition
- Proper test coverage for both old and new behavior
- Opt-in mechanism through configuration
- Backward compatibility maintained
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to 'useHttpClientNext' in the codebase to confirm usage or migration notes. rg "useHttpClientNext"Length of output: 1151
Script:
#!/bin/bash # Check for any migration guides or documentation fd README -e md -X cat {} # Look for the actual implementation and usage of useHttpClientNext ast-grep --pattern 'useHttpClientNext' # Check the specific commit that introduced this change rg -A 5 "Support urllib@3" CHANGELOG.mdLength of output: 6610
Script:
#!/bin/bash # Check the implementation details around useHttpClientNext rg -B 5 -A 10 "useHttpClientNext" src/ rg -B 5 -A 10 "urllib@3" src/ # Check test cases to understand the migration path rg -B 5 -A 10 "useHttpClientNext" test/Length of output: 6197
src/app/extend/response.types.ts (1)
1-7: ValidaterealStatusbehavior and synchronization with underlying HTTP status.
The newrealStatusgetter and setter can improve expressiveness, but ensure they remain in sync with the actual HTTP status code returned to the client. If the underlying implementation diverges (e.g., transforming or mapping statuses), consider adding relevant type checks or validations. Also confirm that any references toresponse.realStatusreflect the correct usage.test/fixtures/apps/app-ts/app/extend/helper.ts (3)
1-2: Confirm that the newIHelperimport path provides correct type definitions.
Switching from'egg'to'../../../../../../src/index.js'could imply relying on compiled JavaScript rather than.d.tsdeclarations. It might be preferable to import from a TypeScript definition to ensure robust type checking.
10-10: Logging statement changed to a static string.
Previously, it might have been logging the result ofthis.ctx.test(). Now it only logs'foo'. Ensure that this is intentional and does not remove valuable debugging context.
12-12: Maintain consistent newlines and brackets.
The file ends cleanly with a newline after the closing brace, adhering to standard coding style. No further action needed here.src/app/extend/request.types.ts (5)
1-3: Module augmentation for@eggjs/corelooks good.
Declaring additional request properties within the declared module helps keep Egg’s typed definitions neatly consolidated.
5-5:acceptJSONgetter is straightforward.
This property clarifies whether the request accepts JSON, aligning well with typical content negotiation checks.
6-7: Consider the implications of overridingqueryas both a getter and setter.
Reading and manipulating query parameters via a typedRecord<string, string>can be concise. However, ensure that downstream code or libraries do not expect arrays for all parameters. If they do, rely onqueriesor unify both approaches.
8-8:queriesintegrates well with multi-value query parameters.
ExposingqueriesasRecord<string, string[]>helps parse repeated parameters. Confirm that any associated validation or sanitization logic is updated to handle arrays of values.
9-10: Module augmentation scope ends properly.
The newly added definitions are cleanly wrapped within thedeclare moduleblock, which follows best practices for extending external modules.test/fixtures/apps/app-ts/app/router.ts (1)
6-6: Verify middleware signature change.By switching from
app.middleware.test()toapp.middlewares.test({}, app), ensure the method matches the new signature in your codebase. Any mismatch could break the routing functionality.test/fixtures/apps/app-ts/app/middleware/generic_ctx.ts (1)
9-10: Check for lost type specificity.Switching from
Context<CustomBody>toEggContextmight reduce the type information available forctx.body. Verify that this matches your intended design and that any typed fields (e.g.,bar) are still validated if needed.src/lib/egg.types.ts (1)
1-3: Confirm Node.js version compatibility.Using
AsyncLocalStoragefrom'node:async_hooks'requires a Node.js version that supports it (v13.10.0+). Ensure your runtime environment meets this requirement.test/fixtures/apps/app-ts/lib/logger.ts (1)
1-2: Confirm the new import path and log level.
Switching from the'egg'package import to a deeply nested local path might lead to maintainability issues if additional refactors occur. Also, ensure that'DEBUG'is indeed a validLoggerLevelfor your logging system.Run the following script to confirm
LoggerLevelusage and validity:✅ Verification successful
Let me generate additional verification scripts to check the
LoggerLeveltype definition and its source.
Let me try a different approach to verify the LoggerLevel import and its valid values.
Let me try one final verification to check the egg-logger package's LoggerLevel definition.
The import path change and LoggerLevel usage are valid
The verification shows that:
- The import path change from
'egg'to'../../../../../src/index.js'is correct as the code is in a test fixture and the LoggerLevel type is properly re-exported from 'egg-logger' in src/index.ts.- 'DEBUG' is a valid LoggerLevel value, as confirmed by the documentation and type definitions which show that LoggerLevel can be: 'DEBUG', 'INFO', 'WARN', 'ERROR', or 'NONE'.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Looking for LoggerLevel definitions and references rg -A 4 "export type LoggerLevel" rg -A 4 "LoggerLevel"Length of output: 1678
Script:
#!/bin/bash # Check the LoggerLevel type definition from egg-logger rg -A 4 "type LoggerLevel" src/ # Check if DEBUG is a valid LoggerLevel value ast-grep --pattern 'type LoggerLevel = $_'Length of output: 78
Script:
#!/bin/bash # Check the egg-logger import and its usage rg -A 4 "from 'egg-logger'" src/ # Check the index.js exports cat src/index.tsLength of output: 4243
Script:
#!/bin/bash # Check if egg-logger is a local dependency and view its type definitions fd -e json package.json --exec cat {} # Check for any logger level constants or enums rg -A 4 "DEBUG.*INFO.*WARN"Length of output: 12923
test/fixtures/apps/cluster_mod_app/app.js (1)
8-8: Validate the usage ofapp.cluster.
These lines consistently inject theclusterattribute for registry and API clients, which is fine. However, ensure that the underlyingclustermechanism correctly initializes each client with the intended configuration, and confirm thatcreate()is necessary forapp.registryClientwhereas direct instantiation suffices forapp.apiClientandapp.apiClient2.Also applies to: 16-17
test/fixtures/apps/app-ts/lib/export-class.ts (1)
26-26: Verify cookie initialization.
Passing['foo']as an additional argument toContextCookiesmight have implications for how cookies are handled. Ensure that this second parameter aligns with the constructor’s expectations and that any code depending on single-argument usage is updated accordingly.src/app/middleware/meta.ts (1)
6-6: Confirm theContextchange.
ReplacingContextDelegationwithContextappears consistent with the broader refactor. Validate that all properties accessed onctxare available in the newContexttype and that no implicit or missing type definitions cause runtime issues.Also applies to: 14-14
src/lib/core/context_httpclient.ts (2)
7-7: Good improvement on type usage.Changing the type to
Contextis more consistent with the rest of the codebase and helps ensure better type safety.
10-10: Confirm references to the new constructor signature.Please make sure all calls to the
ContextHttpClientconstructor are updated to pass aContextobject rather thanContextDelegation.✅ Verification successful
All constructor calls to
ContextHttpClientare using the correct signatureI've verified that all instances where
ContextHttpClientis instantiated are using the correct signature:
- In
test/fixtures/apps/app-ts/lib/export-class.ts: Usingctxparameter- In
test/lib/core/httpclient.test.ts: Using an object with the expected Context interface propertiesThere are no instances using the old
ContextDelegationtype, so no updates are needed.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for any usage of the old constructor signature referencing `ContextDelegation` rg 'new\s+ContextHttpClient\s*\(\s*\w+\s*:\s*ContextDelegation'Length of output: 65
Script:
#!/bin/bash # Let's search for all constructor calls to ContextHttpClient rg "new ContextHttpClient\(" -A 2Length of output: 438
test/fixtures/apps/app-ts/app.ts (1)
1-3: Imports updated correctly.Imports now match the project's new structure. Great job on maintaining consistency with
.jsextensions.test/lib/cluster/cluster-client-error.test.ts (3)
1-4: Nicely structured test setup with ESM features.These initial imports use Node’s built-in modules effectively. Good choice of
importstyle for modern JavaScript.
24-29: Good approach for verifying error logs.Waiting, then validating the logs do not include
ECONNRESETis a clear demonstration that the error was handled.
6-8: Ensure application teardown in parallel tests.If multiple tests create separate apps, confirm that each
MockApplicationis closed to avoid resource leaks.✅ Verification successful
Let me analyze the results from the shell script to verify the concern about application teardown in parallel tests.
The search results show that across the test files, there is a consistent pattern of properly closing the applications after tests. Here are the key observations:
- Most test suites follow the pattern:
before(() => { app = createApp(...); return app.ready(); }); after(() => app.close());
Even in cases with multiple apps in the same test file (like in test/app/middleware/body_parser.test.ts), each app is properly closed.
The specific file under review (test/lib/cluster/cluster-client-error.test.ts) follows the same pattern as other test files in the codebase.
Let me verify if there are any unclosed apps in this specific test file:
Application teardown is properly handled
The test file correctly manages application teardown:
- The app is created in the
beforehook- There's an explicit test case
'should close even if app throw error'that ensures app.close() is called- The test follows the same pattern as other test files in the codebase for proper app cleanup
- Even in the error case scenario being tested, the app is properly closed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Checking for open apps in test code that aren't properly closed rg 'createApp\(' -A 10Length of output: 71138
Script:
#!/bin/bash # Check the specific test file for app creation and closing cat test/lib/cluster/cluster-client-error.test.tsLength of output: 901
src/app/middleware/notfound.ts (1)
9-9: Aligned to the standardizedContexttype.Updating the middleware to use
Contextis consistent with the broader changes.src/lib/core/httpclient.ts (2)
20-20: Use consistent type definition in#app
Same as with thectxandtracerfields, consider eventually replacinganywith a well-defined interface or type to avoid confusion.
46-50: Export aliases look good
ExportingHttpClientwith multiple aliases can help with backward compatibility, and the approach appears consistent.test/index.test-d.ts (6)
2-4: Import statements and usage look correct
The shift to ES modules is consistent, and referencing the new type definitions from../src/index.jsis aligned with your TypeScript conversion goals.
15-15: Validation ofctxtype
Confirm or test thatctxindeed adheres to theContextinterface in all relevant usage sites.
19-61:AppBootclass lifecycle stage tracking
The lifecycle methods look properly declared, and thestagesarray usage appears consistent with theILifecycleBootinterface.
63-65: Verifying correct type usage
InstantiatingAppBootand checking types forIBootandILifecycleBootis a good practice for ensuring type correctness.
67-67: Stages returns a string array
Verifying thatappBoot.stagesis a string array is a concise and straightforward test.
69-69:LoggerLeveltest
Asserting'DEBUG'againstLoggerLevelensures correct type definitions for logging levels.src/app/middleware/site_file.ts (3)
4-4: ImportingContext
Switching toContext(instead of the olderContextDelegation) follows the new standardized approach.
6-6: Signature update forSiteFileContentFun
Replacing the old context type with aContext-typed parameter is consistent with the updated codebase.
17-17: Middleware context parameter
Updating thesiteFilemiddleware function’sctxfromContextDelegationtoContextmaintains consistent usage across the application.test/lib/cluster/cluster-client.test.ts (5)
1-3: Transition to ES modules
Using ESM syntax for imports and aligning with the@eggjs/mockpackage is a step closer to a type-safe, modern codebase.
5-5: UsingSymbol.for
DeclaringinnerClientwithSymbol.for('ClusterClient#innerClient')helps maintain a single unique symbol across the application.
7-8: Type definitions forapp
DeclaringappasMockApplicationclarifies usage in the test suite. This is a solid practice to ensure type correctness.
12-12: Replacedutils.appwithcreateApp
Ensures consistency with new utility naming. This also facilitates an easier transition to TypeScript.
63-63: Single process mode test
UsingsingleProcessApphelps isolate and validate the single-process scenario independently. Nicely done.test/fixtures/apps/app-ts/app/controller/foo.ts (3)
7-10: Ensure imports match usage.
It looks likeHttpClientwas commented out, andEggHttpClientandEggContextHttpClientare now imported. Please confirm that no other parts of the file require the commented import, and remove it if no longer needed.
12-12: Validate the switch from 'urllib-next' to 'urllib'.
Since'urllib-next'was replaced by'urllib', ensure that all required features are still supported by'urllib'and that the new import library exists in the project dependencies.
24-25: Confirm reversed client assignments.
Swapping the types forctxHttpClientandappHttpClient(EggContextHttpClient ↔ EggHttpClient) can break existing references if callers expect the old behavior. Verify the changes in methods referencing these properties to avoid runtime errors.test/ts/index.test.ts (1)
68-110: Review commented-out type-check tests.
These tests can be useful to validate TS compilation. If they are outdated or no longer needed, remove them to keep the test suite focused. Otherwise, consider re-enabling to maintain coverage.src/app/extend/request.ts (2)
4-4: Check for missing'./context.js'availability.
Ensure'./context.js'is compiled and present in the final build, as it’s imported here for type definitions.
16-16: Migration from ContextDelegation to Context.
Declaringctx: Contextis aligned with the new type usage across the codebase. Confirm that any existing places referencing the oldContextDelegationtype are updated or removed.src/app/extend/context.ts (3)
Line range hint
233-246: Handle async background tasks carefully.
The updatedrunInBackgroundsignature using(ctx: Context)is consistent with your overall type changes. Confirm that all call sites are updated to pass the expected argument type.
266-268: Use of request getters for context.
These easy-to-read getters (query,queries,acceptJSON,ip) simplify property access in theContext. Well done!Also applies to: 270-272, 279-281, 288-294
301-307:realStatuspass-through.
Directly mappingrealStatusto the response can be handy, but ensure no unexpected side effects ifrealStatusis mutated. This looks fine if you keep the response logic consistent.test/lib/cluster/master.test.ts (14)
1-4: Well-structured ES module imports.
These imports effectively leverage ESM syntax, without any evident issues.
6-9: Test suite initialization
Switching to TypeScript naming in the describe block is aligned with the file’s .ts extension and fosters clarity.
10-13: Ensure coverage on cluster setup.
Defininglet app: MockApplication;and assigning it usingcluster('apps/app-die')is consistent with the test objectives.
Consider verifying error handling scenarios to ensure all new code paths are tested.
54-57: Coherent naming forapp-die-ignore-code.
Definingapp: MockApplication;and reusingcluster('apps/app-die-ignore-code')is consistent.
No immediate concerns with the test logic; keep ensuring test coverage for error ignore codes.
96-102: Separating master from app usage
Using a separate variablemasterclarifies the distinction between master process tests and app tests.
No issues in terms of logic.
109-114: Leverage environment-based logging checks
Assigninglet app: MockApplication;and verifying the'Egg started'log is helpful to confirm startup success.
Ensure all environment variations remain tested.
122-123: Production environment-specific checks
Applyingmm.env('prod')and adjusting the home path is relevant to test production environment behaviors.
130-133: Porting cluster tests
Definingapp: MockApplication;withcluster('apps/cluster_mod_app')ensures coverage for cluster mode.
No immediate refactor suggestions.
155-157: Ensuring dev environment coverage
Using the same cluster setup for dev environment tests is consistent with the existing approach.
223-227: Custom environment checks
The cluster usage with a custom config path is valid.
Verifying environment-based expectation is well-handled.
244-251: Skipped framework start test
This test block is skipped. Consider enabling it if coverage is needed for custom frameworks.
271-274: Graceful test teardown
Killing the spawned process inafterEachensures clean test environment.
No further concerns.
288-289: Skipped test withcoffee.fork
Likewise, the test is skipped. Ensure coverage if these flows become critical to the final release.
298-300: Worker threads scenario
Again, the test is skipped. Confirm if worker threads usage is stable enough to test in CI.test/fixtures/apps/app-ts/package.json (1)
3-5: ES module adoption
Adding"type": "module"confirms the ESM usage. No compatibility issues spotted.test/fixtures/apps/app-ts/tsconfig.json (1)
7-11: Improved TypeScript path references
Switching"egg": ["../../../../src/index.ts"]ensures closer alignment with the project’s source structure.
Adding"rootDir": "."clarifies the project root, which can simplify build tasks.src/index.ts (3)
10-14: LGTM! Well-structured backward compatibility export.The addition of the
IHelperalias maintains compatibility with egg v3 while using modern TypeScript export syntax.
24-27: LGTM! Clean type export.The explicit type export for
LoggerLevelfollows TypeScript best practices.
75-93: LGTM! Improved type-safe exports.The refactoring from const assignments to proper named exports with aliases improves type safety while maintaining the same functionality.
src/lib/egg.ts (3)
59-59: LGTM! Improved type safety with explicit Context type.The update to use the explicit
Contexttype improves type safety and clarity.Also applies to: 65-65
240-245: LGTM! Well-documented getter with proper this binding.The getter properly binds the
clusterWrappermethod and includes a helpful usage example in the comment.
Line range hint
454-467: LGTM! Robust error handling implementation.The error handling implementation thoroughly handles both Error and non-Error objects, properly preserving error properties during conversion.
🧰 Tools
🪛 Biome (1.9.4)
[error] 453-453: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
|
|
||
| export default { | ||
| test(this: IHelper) { | ||
| this.test2(); | ||
| (this as any).test2(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using (this as any) if type information is available.
Casting this to any obscures type relationships and can hide errors. If test2() must be called on the same IHelper instance, consider using a more precise type or adjusting your type definitions accordingly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
src/app/extend/context.ts (2)
34-36: Avoid usinganyforproxy
Declaringproxy: any;works but undermines type safety. Consider specifying a more precise type if possible.declare service: BaseContextClass; -declare proxy: any; +declare proxy: unknown; // or a suitable interface/type
288-294: Directly settingip
Providing bothgetandsetforipis flexible. However, be mindful that changingthis.request.ipmay cause confusion in real-world usage if overwritten mid-request.src/lib/egg.ts (1)
452-452: Use optional chaining for clarity
Replacingerr && err.messagewith optional chaining makes the expression more concise and explicit.-this.coreLogger.error('[egg:unhandledRejection] %s', err && err.message || err); +this.coreLogger.error('[egg:unhandledRejection] %s', err?.message ?? err);🧰 Tools
🪛 Biome (1.9.4)
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
test/lib/cluster/master.test.ts (2)
28-28: Long wait times in tests.
Usingawait scheduler.wait(10000)may help stabilize environment-dependent tests but will slow test feedback loops. If feasible, adapt a shorter or condition-based wait to reduce time overhead while ensuring reliability.Also applies to: 46-46
244-251:describe.skipusage.
Skipping these tests will keep them out of the CI, which might be intentional if the related functionality is incomplete or flaky. Consider adding TODO comments or re-enabling them once you’ve resolved underlying issues.Need assistance making these tests robust or investigating failures? Let me know if you'd like help opening an issue.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/app/extend/context.ts(5 hunks)src/index.ts(3 hunks)src/lib/agent.ts(1 hunks)src/lib/core/httpclient.ts(2 hunks)src/lib/egg.ts(8 hunks)test/app/middleware/meta.test.ts(1 hunks)test/index.test.ts(1 hunks)test/lib/cluster/master.test.ts(10 hunks)test/lib/egg.test.ts(1 hunks)test/lib/plugins/depd.test.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/lib/agent.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/index.test.ts
- test/lib/plugins/depd.test.ts
- src/lib/core/httpclient.ts
- test/lib/egg.test.ts
- src/index.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/egg.ts
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (25)
test/app/middleware/meta.test.ts (1)
21-21: Stricter validation of X-Readtime values
It's great that you're now enforcing a decimal format with up to three fractional digits. This ensures more precise performance measurements.src/app/extend/context.ts (7)
7-7: Good practice: Switching to type-only imports
Usingimport type { EggLogger }ensures type-only loading, preventing runtime overhead.
13-14: Consistent type-only imports
ImportingRequestandResponsepurely as types is a neat, TS-friendly approach to reduce bundling issues.
233-233: Improved parameter type for background tasks
ReplacingContextDelegationwithContextinrunInBackgroundenhances consistency.
246-246: Consistency for_runInBackgroundscope
This method now accepts(ctx: Context), matching the updated signature above.
266-272: Retaining type safety on request-based getters
Introducing these getters foracceptJSONandqueryis a clean approach to ensure typed access.
279-281:queriesgetter
ExposingqueriesasRecord<string, string[]>clarifies multi-value query params.
301-303: Readable and writablerealStatus
Allowing bothgetandsetonrealStatuskeeps status manipulation straightforward. No issues found.Also applies to: 305-308
src/lib/egg.ts (7)
29-29: Adopting the unifiedContext
Importing the newContextunifies the codebase around a single context type.
59-59: Switch fromContextDelegationtoContext
BothRequestandResponseclasses now referenceContext, ensuring consistent usage.Also applies to: 65-65
71-72: Maintaining backward-compatibility
KeepingILifecycleBoot as IBootensures a smooth transition for Egg 3.x consumers.
76-77: ExportingContext-based types
DefiningEggContext = ContextandMiddlewareFunc<T extends Context>standardizes usage across the framework.
92-92: AsyncLocalStorage with newContext
UpdatingctxStorage: AsyncLocalStorage<Context>aligns perfectly with the rest of the refactoring.
98-99:HttpClientNextfor legacy compatibility
ExposingHttpClientNexthelps older code adapt without lengthy refactors.
239-244:clustergetter referencingclusterWrapper
Neatly exposingclusterWrapperthrough a getter improves clarity and avoids duplication.test/lib/cluster/master.test.ts (10)
1-4: Good adoption of TypeScript-compatible ES module imports.
It’s great to see the updated import statements fornode:timers/promises,@eggjs/mock,coffee, and local utilities. Just ensure that your TypeScript configuration supports.jsimports where needed (e.g.,"allowJs": trueintsconfig.json).
6-6: Consistent test suite naming.
Thedescribeblock explicitly referencesmaster.test.ts, helping maintain clarity in the test output. This is a solid practice for bridging file names and test titles.
10-13: Strong type usage forappvariable.
Defininglet app: MockApplication;and assigning it viacluster('apps/app-die')improves test reliability and IDE autocompletion for the cluster-based setup.
54-57: Readable test flow for ignoring uncaught exceptions.
This block ensures the application doesn’t exit under certain ignored errors. This is very clear and readable—good balancing of necessary environment setup versus the test scenario.
96-102: Clear coverage expectations for cluster failures.
Usingmaster.expect('code', 1)is a valuable technique to verify that the process exits with the correct status when the worker dies. Great approach for cluster-level error handling tests.
109-114: Environment-based test usage.
Switching to dev and production environments and verifying messages like "Egg started" is a strong approach to ensuring environment-specific behaviors are tested. Just be mindful that themm.env('prod')call can carry over if not reset, but you already reset inafterEach.Also applies to: 122-123
130-133: Consistent pattern for cluster tests.
Defining alet app: MockApplication;then callingapp.ready()is a uniform flow across your test suites, enhancing clarity for all cluster mode tests.
172-178: Multi-application scenario tested thoroughly.
Spinning up two separate cluster instances is an effective way to ensure concurrency reliability. Immediate readiness checks and parallel closures reduce test runtime and catch potential conflicts.
271-280: Proper cleanup of spawned child processes.
Invoking(app as any).proc.kill('SIGTERM')helps avoid lingering processes. Good practice to ensure each test cleans up properly, preventing test pollution.
288-289: Skipping worker-thread-related tests.
Marking these tests with.skipis a clean way to track incomplete or environment-specific checks without blocking the entire suite. Just remember to revisit them periodically or designate them for future sprints if they become essential.Also applies to: 298-300
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/egg.ts (1)
452-452: Optional chaining suggested by static analysis.
You might consider:- this.coreLogger.error('[egg:unhandledRejection] %s', err && err.message || err); + this.coreLogger.error('[egg:unhandledRejection] %s', err?.message || err);However, this is purely a stylistic improvement and does not affect runtime behavior significantly.
🧰 Tools
🪛 Biome (1.9.4)
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
index-old.d.ts(0 hunks)package.json(5 hunks)src/config/config.default.ts(1 hunks)src/config/config.local.ts(1 hunks)src/config/config.unittest.ts(1 hunks)src/index.ts(3 hunks)src/lib/egg.ts(8 hunks)src/lib/types.ts(5 hunks)test/index.test-d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- index-old.d.ts
✅ Files skipped from review due to trivial changes (2)
- src/config/config.default.ts
- src/config/config.local.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome (1.9.4)
src/lib/egg.ts
[error] 452-452: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (32)
src/config/config.unittest.ts (1)
1-1: No issues with the updated import path
The import path change to../lib/types.jsaligns with the reorganization of type definitions and appears consistent with the rest of the codebase.src/lib/types.ts (8)
12-12: No concerns with these imports.
These imported entities appear to be used consistently across the codebase.
21-21: Context type update is consistent.
Previously, this likely referred toContextDelegation. Switching toContextaligns with other changes in this PR.
106-106: Revised method signature.
Updatingonerrorto receiveContexthere is consistent with the broader move away fromContextDelegation.
139-142: Deprecation flag is well documented.
MarkinguseHttpClientNextas deprecated provides clarity for future code removal or migration.
334-336: New type definition:RequestObjectBody.
This addition cleanly captures object-body semantics for request handling.
337-346: InterfaceIEggPluginItemlooks well-defined.
This structure keeps plugin configs modular and aligned withEggEnvType, promoting clarity in plugin usage.
347-347: Type aliasEggPluginItem.
Allowing boolean or anIEggPluginItemis a flexible approach, especially for toggling plugins on/off.
349-366:EggPlugininterface broadens plugin configurations.
This structure is thorough and leaves room for custom plugin definitions while still enumerating known plugins.src/index.ts (10)
10-12: ExportingHelper.
Exporting the Helper class streamlines usage in consumer code.
13-16: IntroducingIHelpertype alias.
Providing backward compatibility for older Egg versions is a good practice.
20-20: Re-exporting types from./lib/types.js.
Promotes a single entry point for external consumers and consistent type usage.
23-25: Re-exporting errors.
Exposing newly added or refactored error classes fosters centralized error handling.
26-31: Exporting logger-related types.
Granular re-exports of logging types simplify integration for external code.
32-35: HTTP client exports.
Making HTTP clients available at the top level keeps the API consistent and discoverable.
46-48: AliasingstartEggasstart.
A user-friendly rename for straightforward referencing.
78-80: Re-exportingControllerasBaseContextClass.
This aligns with the internal move fromControllerto a base context class but preserves the expected external name.
86-88: Re-exportingService.
Consistent naming for service classes, preserving legacy usage while tying into the new base context logic.
94-96: Re-exportingSubscription.
Maintains the subscription pattern in a type-safe manner while centralizing the underlying base class.test/index.test-d.ts (5)
3-5: Updated imports for typed tests.
UsingContext,Application,IBoot,ILifecycleBoot,LoggerLevel, andEggPluginaligns tests with the new type definitions introduced in the main code.
16-16: Confirmingctxis now aContext.
This matches the removal ofContextDelegation.
20-62: ImplementingAppBootclass for lifecycle.
Demonstrates the extended lifecycle approach with typed methods. The stages array ensures robust introspection of each lifecycle stage.
64-70: Type checks forAppBootinstance and its fields.
Ensures compliance withIBootandILifecycleBoot, covering robust testing of typed lifecycle classes.
72-100: DetailedEggPluginusage.
Declaring multiple plugin entries withenableandpackagefields effectively tests type correctness for plugin configurations.src/lib/egg.ts (8)
29-29: Switching to theContextimport.
This eliminates the deprecatedContextDelegationreference.
59-65: Context property usage.
Declaringdeclare ctx: ContextonRequestandResponseclasses clarifies the type and ensures consistency.
71-77: Preserving backward compatibility with Egg v3.
Re-exportingILifecycleBootasIBootandEggContextasContexthelps smoothly upgrade existing projects.
92-92: AsyncLocalStorage now storesContext.
This solidifies the new approach, ensuringctxStorageworks with the updatedContexttype.
98-99: PreservingHttpClientNextfor Egg 3.x.
Continues supporting older usage patterns while encouraging the new approach.
239-245: Introducing theclustergetter.
ExposingclusterWrapperthis way is a neat pattern for reference in client creation.
264-264:clusterWrapperrefactor.
Renaming the method is clear and avoids overloading theclustername.
666-667: Updated return type forcreateContext.
Returning theContexttype upholds the new standard across the framework.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/index.test-d.ts (1)
19-19: Consider using a more specific type for request.bodyInstead of using
any, consider using the newly definedRequestObjectBodytype from types.ts for better type safety.-expectType<any>(ctx.request.body); +expectType<RequestObjectBody>(ctx.request.body);src/lib/types.ts (2)
144-147: Add deprecation version informationConsider adding the version number when this was deprecated and when it will be removed.
- * @deprecated keep compatible with egg 3.x, no more used + * @deprecated since version X.Y.Z. Will be removed in version A.B.C. + * Kept for compatibility with egg 3.x
340-340: Consider a more specific RequestObjectBody typeThe current
Record<string, any>type is very permissive. Consider defining a more specific structure or using a generic type parameter for better type safety.-export type RequestObjectBody = Record<string, any>; +export type RequestObjectBody<T = unknown> = Record<string, T>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
index-old.d.ts(0 hunks)src/app/extend/request.ts(2 hunks)src/app/extend/response.ts(1 hunks)src/lib/types.ts(5 hunks)test/index.test-d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- index-old.d.ts
✅ Files skipped from review due to trivial changes (1)
- src/app/extend/response.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/app/extend/request.ts
🔇 Additional comments (6)
test/index.test-d.ts (3)
65-69: LGTM! Good type testing coverageThe type assertions correctly verify that AppBoot implements both IBoot and ILifecycleBoot interfaces, and that stages returns string[].
73-101: LGTM! Comprehensive plugin type testingThe test case thoroughly verifies the EggPlugin type with various plugin configurations, including both detailed configurations and boolean shortcuts.
103-111: LGTM! Complete EggAppInfo type testingThe test case verifies all required properties of EggAppInfo type.
src/lib/types.ts (3)
Line range hint
12-26: LGTM! Good type migrationThe changes correctly update the types to use Context instead of ContextDelegation, maintaining type safety while modernizing the codebase.
111-111: LGTM! Consistent type updateThe onerror callback signature is correctly updated to use the Context type.
357-371: LGTM! Well-structured plugin type systemThe EggPlugin interface provides a comprehensive and type-safe way to configure built-in plugins. The use of optional properties and union types with EggPluginItem allows for flexible configuration.
| } | ||
|
|
||
| get stages(): string[] { | ||
| const app: any = this.app; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using any type
Using any type defeats TypeScript's type checking. Consider creating an interface for the app's stages property.
- const app: any = this.app;
+ interface AppWithStages extends Application {
+ stages?: string[];
+ }
+ const app = this.app as AppWithStages;Committable suggestion skipped: line range outside the PR's diff.
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/@types/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected], npm/[email protected] |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/lib/start.ts (1)
Line range hint
6-58: Consider documenting the single mode type hierarchy.The new type hierarchy for single mode operation is well-structured. Consider adding TSDoc comments to explain:
- The relationship between
SingleModeApplicationandSingleModeAgent- When to use these types vs the base
ApplicationandAgenttypes- The purpose and expected shape of the
pluginsconfigurationThis documentation would help other developers understand the type system and prevent misuse.
test/index.test-d.ts (1)
20-20: Consider using a more specific type for request body.Using
anytype defeats TypeScript's type safety. Consider using a more specific type orunknownif the exact type cannot be determined at compile time.-expectType<any>(ctx.request.body); +expectType<unknown>(ctx.request.body);src/lib/types.ts (1)
151-180: Remove commented out development configurationInstead of keeping commented out code, consider either:
- Removing it completely if it's no longer needed
- Moving it to the actual implementation if it's still relevant
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
package.json(5 hunks)src/index.ts(3 hunks)src/lib/start.ts(3 hunks)src/lib/types.ts(5 hunks)src/lib/utils.ts(0 hunks)test/index.test-d.ts(2 hunks)
💤 Files with no reviewable changes (1)
- src/lib/utils.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.ts
- package.json
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: Node.js / Test (windows-latest, 22)
- GitHub Check: Node.js / Test (windows-latest, 20)
- GitHub Check: Node.js / Test (windows-latest, 18.19.0)
- GitHub Check: Node.js / Test (windows-latest, 18)
- GitHub Check: Node.js / Test (ubuntu-latest, 22)
- GitHub Check: Node.js / Test (ubuntu-latest, 20)
- GitHub Check: Node.js / Test (ubuntu-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 22)
- GitHub Check: Node.js / Test (macos-latest, 18)
- GitHub Check: Node.js / Test (macos-latest, 18.19.0)
- GitHub Check: Node.js / Test (macos-latest, 20)
🔇 Additional comments (12)
src/lib/start.ts (3)
6-6: LGTM! Enhanced type safety for plugin system.The addition of
EggPlugintype improves type safety when configuring plugins.Also applies to: 17-17
20-25: LGTM! Well-structured type hierarchy for single mode.The new interfaces
SingleModeApplicationandSingleModeAgentproperly type the circular reference between application and agent in single mode operation.
54-54: LGTM! Type assertions ensure proper typing.The type assertions to
SingleModeAgentandSingleModeApplicationare necessary and correctly implemented to handle the circular reference between agent and application.Also applies to: 58-58
test/index.test-d.ts (5)
2-8: LGTM! Well-organized imports and type checks.The import statements are properly structured, and the type checks effectively validate core functionality.
Also applies to: 18-20
22-24: LGTM! Proper type checking for watcher configuration.The type checks for watcher configuration properties are well-defined and specific.
34-34: Avoid usinganytype.Using
anytype defeats TypeScript's type checking. Consider creating an interface for the app's stages property.
26-68: LGTM! Well-structured lifecycle implementation.The AppBoot class correctly implements the ILifecycleBoot interface with proper stage tracking. The type checks validate both IBoot and ILifecycleBoot implementations.
Also applies to: 70-74
78-126: LGTM! Comprehensive plugin and application configuration.The code demonstrates:
- Well-structured plugin configuration with proper typing
- Complete EggAppInfo validation
- Thorough type checking for application startup
src/lib/types.ts (4)
10-14: LGTM! Import changes improve type safetyThe changes to imports and type exports enhance type safety and maintain better organization of type definitions.
Also applies to: 22-24
112-112: Well-documented configuration with proper type safetyThe onerror method signature has been updated consistently with the Context type change, maintaining good documentation standards.
339-370: Well-structured plugin type definitionsThe new plugin type definitions provide:
- Clear type hierarchy with
IEggPluginItemandEggPluginItem- Comprehensive interface for built-in plugins
- Good type safety for plugin configuration
This is a solid improvement to the type system.
27-27: Verify all usages of ContextDelegation are updatedThe change from
ContextDelegationtoContexttype is a breaking change. Let's verify all usages are updated consistently.✅ Verification successful
Type change from ContextDelegation to Context is consistently applied
All occurrences in the codebase are using the new Context type, and no references to ContextDelegation remain.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining references to ContextDelegation rg "ContextDelegation" --type ts # Search for the new Context type usage to ensure proper adoption rg "ctx: Context" --type tsLength of output: 2255
Summary by CodeRabbit
Based on the comprehensive summary, here are the high-level release notes for end-users:
New Features
Bug Fixes
Performance
Breaking Changes
These release notes provide a concise overview of the key changes in the framework, focusing on user-facing improvements and potential impacts.