fix: load extend #279
fix: load extend #279
Conversation
|
Warning Rate limit exceeded@fengmk2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThis pull request introduces several significant updates to the Egg.js core package, focusing on modernizing asynchronous code, enhancing lifecycle management, and improving error handling and debugging capabilities. The changes span multiple files across the project, with a primary emphasis on transitioning from generator functions to async/await syntax, adding more detailed logging, and refining the plugin and boot hook management processes. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Lifecycle as Lifecycle Manager
participant BootHook as Boot Hook
participant Plugin as Plugin
App->>Lifecycle: registerBeforeClose
Lifecycle-->>App: Register Callback
App->>BootHook: Initialize
BootHook-->>Lifecycle: Track Hooks
Lifecycle->>Plugin: Load and Initialize
Plugin-->>Lifecycle: Provide Metadata
Lifecycle->>App: Trigger Lifecycle Events
App->>App: Close Application
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
|
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (18)
test/fixtures/extend-controller-service/app/service/api.js (1)
3-4: Consider removingawaitif no additional logic follows.
Currently, the code usesawait this.getData(), but there's no further processing of the result before returning. You could simply doreturn this.getData()to avoid introducing unnecessary async overhead.- return await this.getData(); + return this.getData();test/fixtures/extend-controller-service/app/controller/api.js (1)
8-8: Reassess the need forasynckeyword.
ThefailActionmethod does not use any asynchronous operations. If there's no plan to do so, you can consider removing theasynckeyword to keep the method signature consistent with its logic.- async failAction() { + failAction() { this.fail('something wrong'); }test/fixtures/extend-controller-service/app.js (1)
19-19: Evaluate the necessity ofasyncwhen returning a static value.
Currently, this method doesn’t perform any asynchronous tasks. If it’s designed to be asynchronous in the future, that’s fine; otherwise, consider removingasyncfor consistency and clarity.src/lifecycle.ts (1)
154-169: Enhanced boot hook API
Allowing an optionalfullPathparameter and storing it in a static property on theBootclass promotes better traceability.
- Consider verifying if multiple boot hooks share the same class reference; they might overwrite the static
fullPath. If that is intentional, then it’s fine. Otherwise, you may want each instance to storefullPathin instance fields only.test/fixtures/boot/app/plugin/boot-plugin/app.js (1)
9-9: Short fixed delay
Usingawait scheduler.wait(5);is appropriate for test or demonstration purposes. If you intend meaningful delays in production, consider clarifying that this is only a placeholder or use configuration-based timing that can be fine-tuned.test/fixtures/boot-willReady-error/app.js (2)
14-14: Minimal wait indidLoad
Awaitingscheduler.wait(1);is effectively a negligible delay. Make sure this is deliberate for the test scenario. If you intend a more realistic async initialization, consider a configurable or more substantial delay.
29-29: Consistent short wait inbeforeClose
Wrapping up with a final short delay is consistent with your approach. Just like the other handlers, if you ever need a more robust or configurable approach, consider passing the wait duration as a parameter from config.test/fixtures/boot-didReady-error/app.js (1)
14-14: Minor delay indidLoad
A 1ms wait is effectively instantaneous in practice. Confirm whether this is purely for test validation or consider making it explicit for clarity (e.g.,TEST_DELAY_MS).test/fixtures/boot-configDidLoad-error/app.js (2)
14-14: Short delay might not reflect real-world conditions
A 1 ms delay is extremely brief and might not catch real concurrency or timing issues. Consider increasing the delay or documenting why 1 ms suffices for your test scenario.
24-24: Maintaining consistent async pattern
Continuing to useawait scheduler.wait(1)keeps the lifecycle hooks uniform and more readable than generator-based delays.test/fixtures/boot-didLoad-error/app.js (1)
29-29: beforeClose short wait
As with other lifecycle methods, the 1 ms delay is minimal. Consider explaining the rationale in your test documentation to avoid confusion.test/fixtures/beforestart/app.js (2)
7-8: Async lifecycle usage
Switching from generator to async function makes the code more readable. The 1000 ms delay is more realistic for modeling a real “startup” wait if needed for your test scenario.
16-16: Arrow function-based beforeStart
Using an arrow function for one of these hooks demonstrates flexibility. All appear to perform the same wait logic, so consider whether you can unify them or keep them distinct for varying test coverage.test/fixtures/boot-serverDidLoad-error/app.js (1)
24-24: didReady minimal wait
A 1 ms delay might suffice for verifying the async nature of the lifecycle. If flakiness appears, consider a slightly longer timeout.test/fixtures/app-before-close/app.js (1)
11-11: Rename variable to match the new async implementation
Though the property is namedcloseGeneratorFn, it is now an async function. Consider renaming it for clarity.- app.closeGeneratorFn = true; - app.closeOrderArray.push('closeGeneratorFn'); + app.closeAsyncClosureFn = true; + app.closeOrderArray.push('closeAsyncClosureFn');test/loader/get_framework_paths.test.ts (1)
18-20: Improved clarity by extractingApplicationbefore creation
AssigningApplicationfromimportModulefirst, then injecting it intocreateApp, enhances readability without changing functionality.src/egg.ts (1)
323-333: Document the recommended new lifecycle approach more explicitly.While these lines rightly deprecate direct usage of
beforeClose()in favor of life cycle methods, consider adding references to the suggested new APIs or code examples in the doc comments to guide developers.test/egg.test.ts (1)
89-94: Streamline test setup.These lines repeat the loader steps: load plugin/config/custom app/etc. Consider extracting them into a shared test helper to adhere to DRY (Don’t Repeat Yourself) principles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
test/fixtures/extend/node_modules/b/app/extend/context/index.jsis excluded by!**/node_modules/**
📒 Files selected for processing (32)
package.json(4 hunks)src/egg.ts(1 hunks)src/lifecycle.ts(9 hunks)src/loader/egg_loader.ts(3 hunks)test/egg.test.ts(26 hunks)test/fixtures/app-before-close/app.js(1 hunks)test/fixtures/beforestart-error/app.js(1 hunks)test/fixtures/beforestart-params-error/app.js(0 hunks)test/fixtures/beforestart-timeout/app.js(1 hunks)test/fixtures/beforestart-with-timeout-env/app.js(1 hunks)test/fixtures/beforestart/app.js(1 hunks)test/fixtures/boot-before-close/app.js(0 hunks)test/fixtures/boot-before-close/app/plugin/boot-plugin-dep/app.js(0 hunks)test/fixtures/boot-before-close/app/plugin/boot-plugin/app.js(0 hunks)test/fixtures/boot-before-close/config/plugin.js(0 hunks)test/fixtures/boot-configDidLoad-error/app.js(2 hunks)test/fixtures/boot-didLoad-error/app.js(2 hunks)test/fixtures/boot-didReady-error/app.js(2 hunks)test/fixtures/boot-serverDidLoad-error/app.js(2 hunks)test/fixtures/boot-timeout/app.js(1 hunks)test/fixtures/boot-willReady-error/app.js(2 hunks)test/fixtures/boot/agent.js(2 hunks)test/fixtures/boot/app.js(2 hunks)test/fixtures/boot/app/plugin/boot-plugin/agent.js(1 hunks)test/fixtures/boot/app/plugin/boot-plugin/app.js(1 hunks)test/fixtures/extend-controller-service/app.js(1 hunks)test/fixtures/extend-controller-service/app/controller/api.js(1 hunks)test/fixtures/extend-controller-service/app/router.js(0 hunks)test/fixtures/extend-controller-service/app/service/api.js(1 hunks)test/fixtures/run-with-debug/index.js(0 hunks)test/loader/get_framework_paths.test.ts(1 hunks)test/loader/mixin/load_extend.test.ts(1 hunks)
💤 Files with no reviewable changes (7)
- test/fixtures/beforestart-params-error/app.js
- test/fixtures/boot-before-close/app.js
- test/fixtures/boot-before-close/config/plugin.js
- test/fixtures/extend-controller-service/app/router.js
- test/fixtures/boot-before-close/app/plugin/boot-plugin-dep/app.js
- test/fixtures/run-with-debug/index.js
- test/fixtures/boot-before-close/app/plugin/boot-plugin/app.js
🧰 Additional context used
🪛 Biome (1.9.4)
test/egg.test.ts
[error] 327-328: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (103)
test/fixtures/extend-controller-service/app/controller/api.js (1)
3-4: Ensure robust error handling in the async call.
While the transition to async/await is correct, consider wrapping the call to this.service.api.get() in error handling if there's a possibility of exceptions thrown by this.getData().
src/lifecycle.ts (10)
65-66: Introduce FunWithFullPath type
Defining FunWithFullPath is a clean way to extend existing functions with an optional fullPath. This approach elegantly supports better debugging and traceability.
73-73: Use of Set
Switching from Set<Fun> to Set<FunWithFullPath> ensures compatibility with the new fullPath property. No issues found.
101-104: Additional logging in ready event
These log messages provide improved diagnostics on readiness timing and potential timeouts. They appear consistent with your logging strategy.
176-188: Hook initialization and path assignment
Using isClass to instantiate the hook, then assigning the fullPath if missing covers both class-based and object-based hooks. The logic is concise and well-reasoned.
193-194: Debug statements for registerBeforeStart
Straightforward improvements to aid debugging. No issues.
203-211: registerBeforeClose with optional fullPath
Adding path-based metadata provides clarity on close callbacks. Logging the count is helpful for debugging.
217-220: Detailed close process logging
These debug messages make the close lifecycle traceable, especially with multiple registered close functions. Looks good.
Also applies to: 229-229
236-236: Config lifecycle debug evolution
Adding boot.fullPath to debug logs and registering beforeClose in configDidLoad offers improved visibility and maintainability.
Also applies to: 248-248, 254-254
300-300: Error handling during didReady
Capturing and logging the fullPath along with errors is beneficial for diagnosing issues. No concerns here.
Also applies to: 303-305
320-320: Error handling during serverDidReady
Similar to didReady, logging errors with fullPath significantly improves troubleshooting.
Also applies to: 324-324
test/fixtures/boot-timeout/app.js (1)
1-1: Switched to scheduler.wait
Replacing the custom or manual sleep with Node’s built-in scheduler.wait(10) is a modern approach. Ensure that the Node.js version in use supports node:timers/promises.
Also applies to: 5-5
test/fixtures/beforestart-error/app.js (1)
3-3: Async arrow function for beforeStart
Transitioning from a generator function to async/await simplifies the asynchronous flow. Confirm that the code properly handles any thrown errors.
test/fixtures/beforestart-timeout/app.js (1)
1-1: Async beforeStart with delay
Using scheduler.wait(11000) in an async arrow function is consistent with the broader codebase’s modernization efforts. No issues found.
Also applies to: 4-5
test/fixtures/beforestart-with-timeout-env/app.js (1)
1-2: Adoption of scheduler.wait and async
Replacing generator-based yields with await scheduler.wait(11000) simplifies code. Setting app.beforeStartFunction to true immediately after ensures clarity around the readiness state. Everything looks good.
Also applies to: 4-5
test/fixtures/boot/app/plugin/boot-plugin/agent.js (2)
6-6: Tiny wait duration
Using await scheduler.wait(5); introduces a 5ms wait, which might be too short to have any practical waiting effect in real-world scenarios. If 5ms is intentional for a quick test scenario, that’s fine. Otherwise, consider a slightly longer wait or a robust condition-based synchronization.
1-1: Use of node:timers/promises API
Importing scheduler from node:timers/promises is a modern and concise approach for delaying execution. However, ensure that your runtime environment is at least Node.js 16+ to guarantee full compatibility.
You can verify your Node.js version requirement by reviewing your project's documentation or package metadata (e.g., engines field in package.json). If needed, here’s a script to quickly look for engine specifications:
✅ Verification successful
Node.js version requirement is compatible with node:timers/promises
The project requires Node.js version >= 18.19.0 as specified in package.json, which fully supports the node:timers/promises API. This version requirement is well above the minimum version needed (Node.js 16+) for using the scheduler from node:timers/promises.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Looking for Node.js version requirements in package.json:
rg '"engines":\s*\{' -A 5
Length of output: 185
test/fixtures/boot/app/plugin/boot-plugin/app.js (1)
2-2: Updated import from sleep to scheduler
Consistently switching from a custom sleep utility to node:timers/promises is a good step toward unified asynchronous handling. Continue ensuring Node.js environment compatibility as noted previously.
test/fixtures/boot-willReady-error/app.js (3)
1-1: Consistent import of scheduler
The update to const { scheduler } = require('node:timers/promises'); aligns with the refactoring across the codebase. Good job maintaining consistency.
19-19: Ensuring error throw after wait
You correctly maintain the error throw in willReady even after switching wait mechanisms. This preserves the original test logic for simulating startup errors.
24-24: Confirm didReady logic
The 1ms wait plus a push to bootLog is consistent with the rest. Verify that any references expecting the old sleep method have been removed project-wide.
test/fixtures/boot-didReady-error/app.js (4)
1-1: Uniform usage of scheduler
Replacing the sleep function with node:timers/promises usage promotes a Promise-based approach throughout the code and avoids external utility dependencies.
19-19: Maintain stability in willReady
Your method succinctly logs “willReady” after the short wait. As with other lifecycle methods, ensure any potential side-effects are handled properly when a short delay is used.
24-24: Error handling in didReady
This is correct for preserving the original error scenario. The updated wait call does not interfere with the custom error logic.
29-29: Same pattern in beforeClose
Your short wait here mirrors the approach in other lifecycle hooks, ensuring consistency.
test/fixtures/boot-configDidLoad-error/app.js (3)
1-1: Use of native scheduler for async waiting
Great to see the usage of scheduler.wait(1)—it’s a concise, built-in mechanism for introducing delays. Ensure that your Node.js environment supports node:timers/promises.
19-19: Consistency in lifecycle logging
This line uniformly logs the willReady state after a wait. The approach is consistent with other lifecycle hooks.
29-29: Sufficient error handling for beforeClose
Awaiting scheduler.wait(1) here is fine. If you need to handle cleanup errors, consider wrapping this in a try-catch to ensure the application can respond appropriately.
test/fixtures/boot-didLoad-error/app.js (4)
1-1: Replaced local sleep with native scheduler
Switching to scheduler.wait removes external dependencies and ensures consistency across Node versions that support node:timers/promises.
14-14: Intentional error throw
Noting that you throw an error in didLoad after waiting. This is presumably part of the test fixture. Ensure your tests verify that the error is properly caught and reported.
19-19: Maintain consistency in logging
Similar to other lifecycle methods, the addition of a short wait is consistent. The logging approach remains clear.
24-24: didReady logging using native wait
Looks straightforward. Confirm you have test coverage ensuring the application logs “didReady” at the expected time.
test/fixtures/beforestart/app.js (2)
1-1: Importing scheduler from node:timers/promises
Good choice to switch from manual or third-party solutions to a native timer. Ensure CI environments run on a Node version that supports it.
11-13: Consistent approach across multiple beforeStart handlers
You’re consistently using await scheduler.wait(1000) in all your beforeStart hooks, which helps maintain uniformity in test nature.
test/fixtures/boot-serverDidLoad-error/app.js (5)
1-1: Refined approach for waiting
Using scheduler.wait(1) eliminates the need for a custom sleep utility. This also clarifies that the wait is minimal, suitable for quick coverage in tests.
14-14: Monitored didLoad state
Awaiting 1 ms then pushing ‘didLoad’ to bootLog. Confirm that your test framework accounts for async states accurately.
19-19: Logging willReady
All lifecycle hooks in this file use the same waiting pattern; the code remains consistent.
29-29: Cleaning up in beforeClose
Awaiting a minimal delay is presumably enough to ensure any pending tasks are handled before closure. Looks good.
34-34: serverDidReady throws error
The error is thrown intentionally. Double-check that your test captures this behavior and logs any relevant details.
test/fixtures/boot/agent.js (6)
14-14: Delay usage appears correct
Using the built-in scheduler.wait(1) is a straightforward replacement for custom sleep logic. This maintains readability and consistency.
19-19: Delay usage appears correct
Using await scheduler.wait(1) continues the uniform approach to introducing brief async delays in lifecycle hooks.
24-24: Delay usage appears correct
Your approach is consistent with other lifecycle methods, aiding in unified asynchronous logic.
29-29: Delay usage appears correct
Likewise, this usage is straightforward and consistent. No issues observed.
34-34: Delay usage appears correct
This final lifecycle hook follows the same delays for test or debugging convenience.
1-1: Ensure Node version compatibility for node:timers/promises
Modern Node.js (v16.0.0+) is required to use node:timers/promises. Verify that this project’s environment supports it.
✅ Verification successful
Node version requirement is satisfied for node:timers/promises
The project is running on Node.js v22.9.0, which fully supports the node:timers/promises module introduced in Node.js v16.0.0+. The usage of this module is safe in this environment.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify Node.js version is >=16
node --version
Length of output: 24
test/fixtures/boot/app.js (6)
1-1: Import statement is fine
Replacing a custom sleep utility with the native node:timers/promises is a good modernization step.
18-18: Consistent wait usage in didLoad
This updates the previously used custom sleep call to a native approach, aligning with the codebase’s new async standard.
23-23: Consistent wait usage in willReady
Maintains clarity and consistency across lifecycle hooks.
28-28: Consistent wait usage in didReady
No issues found; the code remains straightforward and aligns with desired async management.
33-33: Consistent wait usage in beforeClose
Again, good replacement that keeps the codebase consistent.
38-38: Consistent wait usage in serverDidReady
Uniform approach across all hooks. The code remains clean and test-friendly.
test/loader/get_framework_paths.test.ts (1)
12-12: Minor correction confirmation
The test description or usage referencing a “parameter” is correct to clarify the configuration source. No issues found.
test/loader/mixin/load_extend.test.ts (1)
24-35: Tests provide clear, descriptive assertion messages.
These changes enhance debugging by providing explicit messages upon test failures. This improvement helps pinpoint exactly which property check failed and speeds up troubleshooting for failing tests.
test/egg.test.ts (44)
8-8: Ensure pedding usage is still necessary.
The pedding library is typically used to handle parallel async tasks in older code. Verify if it's still relevant, given that the code now mostly uses async/await.
13-13: Describe block unskipped.
Activating this test suite is beneficial for coverage. Ensure it remains stable under all environments.
28-28: Confirm that loadCustomApp is needed here.
Loading custom app logic in the same test might duplicate coverage if tested elsewhere. Verify if bundling logic across multiple tests is intentional.
111-111: Plugin assertion.
Ensures that app.plugins has been properly loaded and references the loader’s plugins. Looks consistent and correct.
136-149: Graceful error handling for not-yet-ready plugin.
The added logic to log warnings if plugins remain pending after a timeout is valuable. Verifying final readiness states ensures robust app initialization.
160-162: Confirm log messages.
These lines verify that debug output matches expectations. If log format changes in the future, update these checks accordingly.
172-176: Strict type check for boot functions.
The assertion that “boot only supports function” provides strong dev feedback. This is a good approach to fail fast on invalid input.
179-186: Extended coverage of beforeStart variants.
This test checks synchronous, generator, async, and translated async usage. Excellent coverage on the various ways to define a beforeStart function.
189-197: Timeout environment variable.
Good practice to test a scenario using a custom environment variable for app readiness. This ensures that timeouts behave correctly in real usage.
200-211: Timeout with short EGG_READY_TIMEOUT_ENV.
This block verifies the app behavior under short timeouts. Double-check that the pending calls and event listener handle concurrency correctly.
216-222: Proper error handling in beforeStart.
The test ensures that any thrown errors get propagated and handled. This is essential for diagnosing startup issues.
227-235: Coverage of error retrieval from ready.
Confirms that an app fails gracefully if beforeStart throws. This direct retrieval of the error from app.ready() is crucial for robust error reporting.
240-242: Timeout handling for beforeStart.
Demonstrates a function that never signals readiness, ensuring that the test triggers ready_timeout. Good job covering edge cases.
251-261: Lifecycle of app.close event.
Ensures that the close event is emitted before the process exits. This is critical for resource cleanup routines.
268-268: Promise return from app.close.
Asserting that close() returns a promise helps verify the correct usage of async lifecycle. This is consistent with modern best practices.
272-279: Error handling on removal of listeners.
Using mm to mock out removeAllListeners tests how the app handles unexpected errors during shutdown. Good thorough coverage of edge cases.
300-301: Readiness established before close.
Ensures the app is fully ready, then triggers beforeClose. This test helps confirm correct ordering of lifecycle steps.
322-337: Validation of multiple beforeClose calls.
The test ensures that all added beforeClose functions execute once in a well-defined order. This is critical to avoid repeated cleanup or concurrency issues.
🧰 Tools
🪛 Biome (1.9.4)
[error] 327-328: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
349-349: Double-close check.
Line 349 tests that subsequent closes do not re-invoke the same logic. A single close routine is a well-known best practice.
355-355: Testing extended controllers & services.
This line verifies the creation app environment for advanced usage of Controller and Service. Implementation looks stable.
357-358: Final readiness check.
Ensures that the loader processes all files and awaits readiness. No issues spotted.
376-376: Skipped debug test.
Reconfirm if skipping is temporary. Re-enabling the debug test ensures coverage for debug-based diagnostics.
455-463: Detailed timeline checks.
Verifies timeline generation during loading phases. This approach helps ensure accurate performance and debug metrics.
519-522: Checks agent-based timeline.
Similarly verifies the lifecycle on agent-based loading. Keeping consistency between agent and app tests is crucial.
528-530: Agent timeline assertions.
Additional focus on time calculations and process IDs. Well done ensuring these are captured accurately.
533-533: Log statement coverage.
Validates logging for plugin loading steps. The line parameter ensures correct logging output.
536-538: Ensuring config file resolution.
Check that each config file is loaded in correct order. This fosters consistent environment-specific overrides.
541-542: Extend/application load logs.
Verifies the correct load event name. This naming consistency is helpful for debugging large codebases.
545-546: Agent-specific logging.
Ensures that agent-specific code also notes the correct file path, simplifying plugin or boot hook debugging.
551-551: Skipped script timing suite.
If possible, re-enable these tests. Script-level timing can be very informative for diagnosing performance issues.
574-574: Check various boot lifecycle phases.
Ensures that the final test steps confirm standard phases: configDidLoad, didLoad, willReady, didReady, serverDidReady, beforeClose, etc.
636-639: Agent loader calls.
Verifies partial loading approach for agent-based apps. Good consistency with application-based tests.
711-711: Handle configDidLoad error.
Test ensures a thrown error in configDidLoad halts the boot sequence. Good coverage of early-stage issues.
724-724: didLoad error handling.
Confirms that subsequent steps do not proceed after a failed didLoad, ensuring the application gracefully handles failure.
743-743: Timing log validation.
Asserts a meaningful timeline is logged on errors. This helps diagnosis of partial boot states.
752-752: willReady error test.
Fails quickly if willReady throws. This is a hallmark of robust error-handling in an asynchronous lifecycle.
778-778: didReady error test.
Ensures the error event triggers if didReady fails. This advanced coverage ensures no silent failures occur late in boot.
806-806: serverDidReady error test.
Similarly checks that post-server boot errors propagate. Thorough coverage for every stage of the lifecycle.
815-815: Error retrieval.
Captures the error from app.on('error') after serverDidReady. Good demonstration of how to handle final-phase errors.
830-830: Boot with debug statements.
Ensures no unexpected issues occur when running the final steps with debug logs. Thorough approach to QA.
832-832: Order of lifecycle checks.
Adds an extra readiness function call to confirm the final order is correct. Straightforward and valuable test approach.
862-862: Cleanup after verifying final state.
Closes the app neatly once done checking the final readiness function. Good practice for test isolation.
877-877: Timeout warnings.
Tests that a large boot step triggers a readiness timeout for debugging. This helps detect slow or stuck boot hooks promptly.
882-882: Close after verifying logs.
Ensures correct final state and no resource leaks remain. Good housekeeping in integration tests.
src/loader/egg_loader.ts (3)
784-784: Enhanced plugin resolution error logging.
Logging the error along with plugin info is excellent for debugging. This detail often helps pinpoint misconfigurations.
1169-1185: Refined boot hook loading.
The debug statements and function-based boot hooking clarify each loaded file. This change aids developers in tracking which files define boot hooks.
1602-1608: Ignore .ts file loading for non-TypeScript mode.
These lines skip .ts resolution unless EGG_TYPESCRIPT is true. This is a helpful safeguard preventing accidental usage of TS files in plain JS environments.
package.json (3)
15-15: LGTM! Build process improvements look good
The changes enhance the build process with:
- Clean builds via rimraf
- Type checking via attw
- Consistent pre-test and pre-ci flows
Also applies to: 17-17, 20-20, 22-22
41-41: Verify dependency compatibility and security
Please ensure:
- The @eggjs/utils ^4.1.5 update is compatible with the codebase
- The new dependencies (@eggjs/bin ^7.0.0, pedding ^2.0.0, rimraf 6) are stable and secure
Also applies to: 56-56, 69-70
✅ Verification successful
Let me gather more information about the dependency usage and compatibility.
Dependencies appear to be safe and compatible
The verification shows:
- No security vulnerabilities found in any of the new dependencies (pedding, @eggjs/bin, rimraf)
- @eggjs/utils is used minimally in the codebase for importing modules and getting plugins, primarily in test files
- The usage pattern suggests the update should be compatible as it's used for basic utility functions
- No peer dependency conflicts found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any known vulnerabilities in the new dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "pedding") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}
'
# Check changelog for breaking changes in @eggjs/utils
curl -s https://raw.githubusercontent.com/eggjs/utils/main/CHANGELOG.md || echo "Changelog not found"
Length of output: 355
Script:
#!/bin/bash
# Check how @eggjs/utils is used in the codebase
rg "@eggjs/utils" -A 2
# Check for any known vulnerabilities in other new dependencies
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "@eggjs/bin") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}
'
gh api graphql -f query='
{
securityVulnerabilities(first: 10, ecosystem: NPM, package: "rimraf") {
nodes {
advisory {
summary
severity
}
vulnerableVersionRange
}
}
}
'
# Check package.json for peer dependencies
cat package.json | jq '.peerDependencies'
Length of output: 1678
3-3: Verify the rationale for a beta release
The version bump to 6.3.0-beta.0 suggests significant changes. Please ensure:
- The changes warrant a minor version bump (6.3.0)
- The beta tag is appropriate given the stability of the changes
✅ Verification successful
Based on the CHANGELOG.md and recent pull requests, I can now provide a final response about the version bump.
Version bump to 6.3.0-beta.0 is appropriate
The version bump from 6.2.4 to 6.3.0-beta.0 is justified based on the following significant changes in development:
-
New features being added:
- Support for file returning async functions when loading configuration
- Support for
pkg.eggPlugin.exportsproperty for better module exports handling - Separation of agent and application start time in timing metrics
-
Breaking changes being tested:
- Changes to timing and lifecycle management
- Modifications to core loading behavior
The beta tag is appropriate as these changes need thorough testing, especially the new async file loading capabilities and timing modifications which could impact application startup and configuration loading.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for breaking changes or significant features
git log --oneline $(git describe --tags --abbrev=0)..HEAD | grep -iE "feat:|breaking change:"
# Check package.json version history
git log -p package.json | grep "version"
Length of output: 0
Script:
#!/bin/bash
# Let's try a different approach to check for changes
# Check recent commits
git log --oneline -n 20
# Look for CHANGELOG.md updates
fd "CHANGELOG.md" --exec cat {}
# Check for any major changes in pull requests
gh pr list --state merged --limit 10 --json title,body
Length of output: 39462
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #279 +/- ##
==========================================
+ Coverage 92.89% 97.86% +4.96%
==========================================
Files 10 10
Lines 3294 3366 +72
Branches 529 591 +62
==========================================
+ Hits 3060 3294 +234
+ Misses 234 72 -162 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [6.2.5](v6.2.4...v6.2.5) (2024-12-28) ### Bug Fixes * load extend ([#279](#279)) ([0f0a13c](0f0a13c))
Summary by CodeRabbit
New Features
6.3.0-beta.1.Bug Fixes
Tests
Chores