fix: export EggAppConfig type let plugin can override it#286
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 8 minutes and 19 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 (5)
WalkthroughThis pull request introduces a comprehensive type system enhancement for the Egg framework. The changes centralize type definitions in a new Changes
Sequence DiagramsequenceDiagram
participant Types as types.ts
participant Index as index.ts
participant Loader as egg_loader.ts
participant FileLoader as file_loader.ts
Types->>Index: Re-export all types
Index-->>Loader: Import types
Loader->>FileLoader: Use CaseStyle enum
FileLoader-->>Loader: Provide type-safe case styles
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/loader/egg_loader.ts (2)
825-828: Clear initialization of essential middleware arrays.
DefiningappMiddlewareandcoreMiddlewarehere aligns withEggAppConfig. Consider also documenting default usage to avoid confusion about empty arrays.
858-859: Potential naming mismatch.
coreMiddlewares = coreMiddlewareandappMiddlewares = middlewarecan be confusing. Consider consistent naming across the codebase for clarity (e.g.,coreMiddleware→coreMiddlewaresin config or vice versa).src/egg.ts (1)
251-252: Fallback Return for ConfigurationWhile casting an empty object to
EggAppConfigavoids TypeScript errors, it might mask missing properties at runtime. Consider returning a properly shaped partial object or throwing an error to prevent hidden misconfigurations.- return this.loader ? this.loader.config : {} as EggAppConfig; + if (this.loader) { + return this.loader.config; + } + // Return a safe fallback or throw error instead of silent cast + return { + // Provide an actual minimal fallback or handle the scenario appropriately + } as EggAppConfig;test/loader/mixin/load_controller.test.ts (1)
382-382: Safeguard Non-Null AssertionUsing the
!operator assertscontrolleris non-null. Ensure this is always valid here. Alternatively, you could safely guard this test logic to prevent potential runtime errors ifcontrollerbecomes undefined.- assert.equal(app.config.controller!.supportParams, true); + assert.ok(app.config.controller && app.config.controller.supportParams === true, + 'controller supports params should be true' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/egg.ts(2 hunks)src/index.ts(1 hunks)src/loader/egg_loader.ts(14 hunks)src/loader/file_loader.ts(3 hunks)src/types.ts(1 hunks)test/index.test.ts(3 hunks)test/loader/mixin/load_controller.test.ts(1 hunks)test/loader/mixin/load_custom_loader.test.ts(1 hunks)
🔇 Additional comments (23)
src/loader/egg_loader.ts (10)
13-13: No issues importing fromfile_loader.
Usage ofCaseStyleshould facilitate robust filename transformations.
22-22: Good centralization of type definitions.
Switching toEggAppConfig,EggAppInfo, andEggPluginInfofrom a dedicated file helps maintain cleaner type references.
811-811: Improved type safety for config property.
UsingEggAppConfigstrengthens type coherence and enforces consistent config structures.
1177-1177: UsingCaseStyle.lowerfor service loading.
Ensures consistent case transformation, avoiding collisions when naming service classes.
1217-1217: ApplyingCaseStyle.lowerfor middleware loading.
Consistent usage improves naming conventions, minimizing letter-case drift in middleware files.
1292-1292: Controller loader now enforcesCaseStyle.lower.
This aligns controllers with a standardized naming pattern across the application.
1372-1372: ExtendingcaseStyle: CaseStyle.lowerfor custom loader injections.
Maintaining uniform naming ensures less confusion when referencing dynamically loaded classes.
1383-1383: Initializer for custom loader also usesCaseStyle.lower.
This continues the uniform approach, simplifying code support.
1502-1503: Enhanced type forloadToAppsignature.
By omitting'inject'and'target', you prevent accidental overriding and maintain clearer, safer defaults. The usage ofnew FileLoader(loadOptions).load()is coherent with the updated signature.Also applies to: 1506-1506, 1515-1515
1526-1528: Refined context loader signature.
Similar toloadToApp, this ensures consistent injection mechanics for context objects. Keep verifying that other parts of the framework properly adopt these new type constraints.Also applies to: 1537-1537
✅ Verification successful
Let me analyze the usage of
loadToContextacross the codebase to verify the type constraints.
Let me check one more aspect to verify the type constraints implementation.
Type constraints for loadToContext are properly implemented and used
Based on the verification results:
- The
loadToContextmethod's signature correctly enforces type constraints throughContextLoaderOptionsinterface- The interface extends
FileLoaderOptionswith proper omission of 'target' property- All usages in the codebase follow the type constraints:
- Service loading uses proper type casting:
options as ContextLoaderOptions- Custom loader implementation properly constructs the options object
- Test cases validate the type constraints through various scenarios
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching references to loadToContext calls, verifying parameter usage rg -A 5 "loadToContext"Length of output: 4846
Script:
#!/bin/bash # Check the type definitions and implementations ast-grep --pattern 'interface ContextLoaderOptions { $$$ }' # Check test cases for type validation rg -B 3 -A 3 "loadToContext.*options"Length of output: 867
Script:
#!/bin/bash # Check the interface definitions and type constraints rg -B 5 -A 5 "interface.*LoaderOptions" # Check test cases with type validation ast-grep --pattern 'loadToContext($$$, $_, { $$$ })'Length of output: 2587
src/index.ts (1)
13-13: Re-exporting types from a centralized file.
This is beneficial: it exposes all type definitions at a single import entry point for consumers.test/index.test.ts (3)
3-3: Explicit type import fosters clarity.
ImportingEggAppConfigimproves tests’ maintainability and ensures alignment with the actual app config shape.
14-14: Added 'CaseStyle' to the assertion list.
Verifying thatCaseStyleis exposed from the index, ensuring the library’s public API remains stable.
37-44: Test ensures new config properties are exposed.
Demonstrates thatcoreMiddlewareandappMiddlewarematchEggAppConfigexpectations. Good coverage of the newly introduced fields.src/types.ts (4)
1-16:EggAppInfocentralizes all essential app metadata.
The defined fields (e.g.,baseDir,env,root) are well-structured, reflecting typical Egg usage patterns.
18-38:EggPluginInfoclarifies plugin metadata.
Properties likedependencies,optionalDependencies, andenvare crucial for a robust plugin ecosystem.
40-47:CustomLoaderConfigItemstructure is clear.
Capturingdirectory,inject, andloadunitoffers direct guidance for customizing loader behavior in Egg.
49-56:EggAppConfigconsolidates config fields.
DefiningcoreMiddleware,appMiddleware,customLoader, andcontrollersupport ensures that applications benefit from type-safe configuration.test/loader/mixin/load_custom_loader.test.ts (1)
105-106: Explicitly addingcoreMiddlewareandappMiddleware.
Enhances clarity in your test config, proving that the application can handle these new fields without collisions.src/loader/file_loader.ts (3)
14-18: Enums Provide Stronger Type SafetyReplacing the string literal union type with an enum is a solid move. It increases readability, ensures robust type-safety, and eases debugging and refactoring operations.
87-87: Good Default Value UsageSetting
CaseStyle.camelas a default aligns with typical naming conventions. This makes the loader behavior more intuitive out of the box.
96-96: Backward Compatibility ApproachThanks for preserving backward compatibility by converting
lowercaseFirstusage toCaseStyle.lower. Ensure the deprecation notice is adequately documented to guide users in migrating away from the legacy option.src/egg.ts (1)
21-21: New Explicit Type ImportImporting
EggAppConfigexplicitly clarifies the contract for configuration objects, enhancing maintainability.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #286 +/- ##
==========================================
- Coverage 97.74% 97.73% -0.02%
==========================================
Files 10 10
Lines 3376 3351 -25
Branches 595 596 +1
==========================================
- Hits 3300 3275 -25
Misses 76 76 ☔ View full report in Codecov by Sentry. |
[skip ci] ## [6.2.12](v6.2.11...v6.2.12) (2025-01-03) ### Bug Fixes * export EggAppConfig type let plugin can override it ([#286](#286)) ([62b1f97](62b1f97))
1 participant
Summary by CodeRabbit
Type Safety Improvements
Code Structure
Testing