feat: support optional inject #254
Conversation
WalkthroughThe pull request introduces enhancements to the Changes
Possibly related PRs
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (12)
core/types/core-decorator/Inject.ts (1)
4-5: Consider enhancing documentation with examples.While the comment clearly explains the default behavior, it would be helpful to add JSDoc examples showing both optional and required injection usage patterns.
Consider adding examples like this:
export interface InjectParams { // obj instance name, default is property name name?: string; // optional inject, default is false which means it will throw error when there is no relative object + /** + * @example + * // Required injection (default) + * @Inject() + * service: UserService; + * + * // Optional injection + * @Inject({ optional: true }) + * cache?: CacheService; + */ optional?: boolean; }core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts (2)
3-4: Consider enhancing the test fixture with a more realistic interface.While the empty interface works for demonstration, consider adding some properties to
PersistenceServiceto better represent a real-world scenario and improve type safety.interface PersistenceService { + save(data: unknown): Promise<void>; + find(id: string): Promise<unknown>; }🧰 Tools
🪛 Biome
[error] 2-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
6-13: Add documentation to explain the different injection approaches.The class effectively demonstrates two ways to achieve optional injection, but it would be helpful to add JSDoc comments explaining:
- The purpose of this test fixture
- The difference between
@Inject({ optional: true })and@InjectOptional()- The expected behavior when dependencies are missing
+/** + * Test fixture demonstrating optional dependency injection. + * This service shows two equivalent approaches for optional injection: + * 1. Using @Inject with explicit optional configuration + * 2. Using the shorthand @InjectOptional decorator + * + * Both properties will be undefined when PersistenceService is not available, + * instead of throwing an error. + */ @SingletonProto() export default class OptionalInjectService { + // Explicit approach using optional configuration @Inject({ optional: true }) persistenceService: PersistenceService; + // Shorthand approach using dedicated decorator @InjectOptional() persistenceService2: PersistenceService; }core/core-decorator/test/fixtures/decators/ConstructorObject.ts (1)
15-16: Consider documenting the preferred injection approachThe implementation demonstrates two valid approaches for optional injection:
@Inject({ optional: true })- Configuration-based@InjectOptional()- Dedicated decoratorWhile both approaches work correctly, having two ways to achieve the same goal might lead to inconsistency in the codebase. Consider documenting the preferred approach in the framework's guidelines.
core/core-decorator/test/fixtures/decators/CacheService.ts (1)
42-43: Consider using a more specific type than 'any'While
anymight be sufficient for testing purposes, consider using a more specific type to better represent the expected service type and improve type safety.- optionalService1?: any; + optionalService1?: TestService4; // or another appropriate typecore/core-decorator/src/decorator/Inject.ts (1)
60-67: Add JSDoc documentation for the InjectOptional function.The implementation is clean and type-safe. However, it would benefit from JSDoc documentation explaining:
- The purpose of optional injection
- Example usage
- Parameter description
Add documentation like this:
+/** + * Creates a decorator for optional dependency injection. + * Use this when the dependency might not be available at runtime. + * + * @example + * class MyService { + * @InjectOptional('optionalDep') + * private dep?: SomeService; + * } + * + * @param param - Optional name or configuration for the injectable dependency + */ export function InjectOptional(param?: Omit<InjectParams, 'optional'> | string) {core/types/metadata/model/EggPrototype.ts (2)
28-31: Documentation could be more descriptive.The addition of the
optionalproperty is well-structured, but the documentation could be more descriptive.Consider enhancing the JSDoc comment:
/** - * optional inject + * Indicates whether this injection is optional. When true, the system will not throw + * an error if the injectable object is not found. */ optional?: boolean;
28-31: Consider documenting optional injection behavior.The addition of optional injection support is a significant architectural change. Consider:
- Documenting the runtime behavior when optional dependencies are not found
- Adding examples in the framework's documentation
- Updating error handling guidelines for optional dependencies
Would you like assistance in drafting documentation for these architectural changes?
Also applies to: 55-58, 79-82, 103-106
core/metadata/src/impl/EggPrototypeBuilder.ts (1)
143-145: Remove redundant optional flag assignment.The condition sets
optional = truewhen it's alreadytrue, which is redundant.Remove the redundant assignment:
- if (injectObject.optional) { - injectObject.optional = true; - }core/core-decorator/test/decorators.test.ts (1)
76-83: Consider adding more test cases for optional injection scenarios.While the current tests verify the metadata for optional services, consider adding test cases that:
- Verify behavior when optional services are not available
- Include negative test cases (e.g., required services marked as optional)
Would you like me to help generate additional test cases to improve coverage?
core/metadata/test/LoadUnit.test.ts (2)
78-88: Enhance test coverage for optional injection.While the test verifies that
injectObjectsis empty, consider the following improvements:
- Make the test case name more descriptive (e.g., "should handle optional injection when dependency is missing")
- Add test cases to verify:
- Optional injection when dependency exists
- Multiple optional dependencies
- Mix of optional and required dependencies
Here's a suggested structure:
describe('optional inject', () => { it('should handle missing optional dependency', async () => { // Current test implementation }); it('should inject optional dependency when available', async () => { // Test with available dependency }); it('should handle mix of optional and required dependencies', async () => { // Test combining both types }); });
86-86: Improve assertion message for better debugging.Consider adding a descriptive message to the assertion to make test failures more informative.
- assert.deepStrictEqual(optionalInjectServiceProto[0].injectObjects, []); + assert.deepStrictEqual( + optionalInjectServiceProto[0].injectObjects, + [], + 'Optional inject service should have no inject objects when dependencies are missing' + );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (11)
- core/core-decorator/src/decorator/Inject.ts (3 hunks)
- core/core-decorator/test/decorators.test.ts (2 hunks)
- core/core-decorator/test/fixtures/decators/CacheService.ts (2 hunks)
- core/core-decorator/test/fixtures/decators/ConstructorObject.ts (2 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (2 hunks)
- core/metadata/test/LoadUnit.test.ts (1 hunks)
- core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts (1 hunks)
- core/metadata/test/fixtures/modules/optional-inject-module/package.json (1 hunks)
- core/types/core-decorator/Inject.ts (1 hunks)
- core/types/core-decorator/model/InjectObjectInfo.ts (1 hunks)
- core/types/metadata/model/EggPrototype.ts (4 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/metadata/test/fixtures/modules/optional-inject-module/package.json
🧰 Additional context used
🪛 Biome
core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts
[error] 2-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
🔇 Additional comments (16)
core/types/core-decorator/Inject.ts (1)
4-5: LGTM! Well-documented interface extension.The addition of the
optionalproperty with clear documentation of its default behavior and implications is clean and follows TypeScript best practices.Let's verify the test coverage for this new property:
✅ Verification successful
Optional injection is well-tested across multiple test files
The feature has comprehensive test coverage:
- Test cases in
core/metadata/test/LoadUnit.test.tswith dedicated "optional inject" test suite- Implementation tested in both property and constructor injection scenarios in
core/core-decorator/test/fixtures/- Both
@Inject({ optional: true })and@InjectOptional()decorator variants are covered- Error handling for optional dependencies is implemented in
EggPrototypeBuilder🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for test cases covering the optional injection behavior # Expect: Test files containing assertions for both successful optional injection and failure cases # Search for test files related to injection rg -l "describe.*[Ii]nject" --type ts # Look for specific test cases handling optional injection rg "optional.*[Ii]nject|[Ii]nject.*optional" --type tsLength of output: 2774
core/types/core-decorator/model/InjectObjectInfo.ts (1)
12-15: LGTM! Well-documented and type-safe implementation.The addition of the optional
optionalproperty is clean, maintains backward compatibility, and follows TypeScript best practices. The JSDoc documentation clearly explains the property's purpose.core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts (1)
1-13: Ensure test coverage for optional injection scenarios.While this fixture demonstrates the optional injection feature, please ensure there are corresponding test cases that verify:
- Behavior when dependencies are available
- Behavior when dependencies are missing
- Both injection approaches work identically
Would you like help creating comprehensive test cases for this feature?
🧰 Tools
🪛 Biome
[error] 2-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
core/core-decorator/test/fixtures/decators/ConstructorObject.ts (2)
3-3: LGTM: Import statement is correctly structuredThe addition of
InjectOptionalto the import statement is appropriate and follows the pattern of importing related decorators together.
15-16: Verify test coverage for both injection approachesLet's ensure both optional injection approaches are properly tested.
✅ Verification successful
Both optional injection approaches are properly tested
The verification shows comprehensive test coverage for both optional injection approaches:
- Unit tests in
LoadUnit.test.tsspecifically verify optional injection behavior in the "optional inject" test suite- Both
@Inject({ optional: true })and@InjectOptional()decorators are tested in multiple test fixtures:
core/core-decorator/test/fixtures/decators/CacheService.tscore/core-decorator/test/fixtures/decators/ConstructorObject.tscore/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts- The decorator tests (
decorators.test.ts) verify the injection metadata for both approaches, ensuring they are properly marked as optional in the dependency injection systemBoth approaches are consistently implemented and tested across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases covering both optional injection approaches echo "Searching for test cases covering @Inject({ optional: true })..." rg -l "@Inject\(\s*{\s*optional:\s*true\s*}\s*\)" --type ts echo "Searching for test cases covering @InjectOptional..." rg -l "@InjectOptional\(\s*\)" --type ts echo "Searching for related test assertions..." rg "(?i)optional.*inject" --type ts -A 5 "*.test.ts"Length of output: 898
Script:
#!/bin/bash # Let's fix the regex and search more effectively # Search for files with optional injection patterns echo "=== Files with @Inject({ optional: true }) ===" rg -l '@Inject.*optional.*true' --type ts echo -e "\n=== Files with @InjectOptional ===" rg -l '@InjectOptional' --type ts # Look for test files and their contents echo -e "\n=== Test file contents for optional injection ===" fd -e ts -e test.ts | xargs rg -l "optional.*inject|@InjectOptional|@Inject.*optional" || true # Get content of specific test files to verify coverage echo -e "\n=== Content of CacheService test ===" rg -A 5 "@InjectOptional|@Inject.*optional" core/core-decorator/test/fixtures/decators/CacheService.ts || true echo -e "\n=== Content of ConstructorObject test ===" rg -A 5 "@InjectOptional|@Inject.*optional" core/core-decorator/test/fixtures/decators/ConstructorObject.ts || true echo -e "\n=== Content of OptionalInjectService test ===" rg -A 5 "@InjectOptional|@Inject.*optional" core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts || trueLength of output: 2271
Script:
#!/bin/bash # Let's check the actual test files to verify test coverage echo "=== Checking LoadUnit.test.ts for optional injection tests ===" cat core/metadata/test/LoadUnit.test.ts echo -e "\n=== Looking for test files in core/core-decorator/test ===" fd -e test.ts . core/core-decorator/test echo -e "\n=== Checking core-decorator test files for optional injection tests ===" fd -e test.ts . core/core-decorator/test -x cat {}Length of output: 21070
core/core-decorator/test/fixtures/decators/CacheService.ts (3)
1-2: LGTM: Import statements are properly updatedThe imports are correctly structured to support the new optional injection feature.
41-46: LGTM: Good test coverage for optional injectionThe test fixture properly demonstrates both approaches to optional injection, which will help ensure the feature works as intended. The implementation looks clean and straightforward.
42-46: Clarify the difference between @Inject({ optional: true }) and @InjectOptional()The code demonstrates two different approaches to optional injection. This might be confusing for developers. Consider:
- Documenting the difference between these approaches
- Providing guidance on when to use each approach
- Potentially standardizing on one approach if they serve the same purpose
Let's check if there's documentation for these decorators:
✅ Verification successful
@InjectOptional()is a convenience wrapper for@Inject({ optional: true })Looking at the implementation,
@InjectOptional()is simply a convenience decorator that internally calls@Inject()with theoptional: trueparameter. Both approaches achieve the same result, with@InjectOptional()offering a more concise syntax. Specific findings:
- Both decorators use the same underlying implementation
@InjectOptional()forwards all parameters to@Inject()while ensuringoptional: true- They are functionally equivalent and just differ in syntax
Recommendation:
- Prefer
@InjectOptional()for better readability when only optional injection is needed- Use
@Inject({ optional: true })when additional injection parameters need to be specified along with optional behavior🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for documentation about optional injection echo "Searching for documentation about optional injection..." rg -l "optional.*inject|inject.*optional" --type md # Search for other usages of these patterns echo "Searching for other usage patterns..." rg "@Inject\(\s*{\s*optional:\s*true\s*}\s*\)" --type typescript rg "@InjectOptional\(\)" --type typescriptLength of output: 543
Script:
#!/bin/bash # Search for documentation about optional injection with simpler patterns echo "Searching for documentation..." rg -l "optional.*inject|inject.*optional" . # Search for decorator implementations echo "Searching for decorator implementations..." ast-grep --pattern 'export function InjectOptional' ast-grep --pattern 'export function Inject' # Search for usage patterns with simpler regex echo "Searching for usage patterns..." rg "@Inject.*optional.*true" rg "@InjectOptional"Length of output: 1408
Script:
#!/bin/bash # Let's check the implementation details of both decorators echo "Checking Inject decorator implementation..." cat ./core/core-decorator/src/decorator/Inject.ts # Let's also check the test files to understand the intended behavior echo "Checking test files..." cat ./core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.tsLength of output: 3064
core/core-decorator/src/decorator/Inject.ts (4)
6-7: LGTM: Clean parameter normalization.The parameter handling has been improved by consolidating the string/object transformation into a single variable, making the code more maintainable.
10-10: LGTM: Consistent parameter handling.The changes maintain the existing logic while leveraging the normalized
injectParam, ensuring consistency throughout the code.Also applies to: 20-20
59-67: Verify test coverage for optional injection scenarios.Please ensure comprehensive test coverage for optional injection, including:
- Runtime behavior when optional dependency is missing
- Type checking with TypeScript's strict mode
- Edge cases with constructor injection
Let's check the test coverage:
#!/bin/bash # Look for test cases covering optional injection rg -A 5 "InjectOptional|optional.*inject" "test/"
28-31: Consider adding type validation for the optional property.While the implementation is clean, consider ensuring that the
optionalproperty is properly typed in theInjectObjectInfointerface to maintain type safety.Let's verify the type definition:
core/metadata/src/impl/EggPrototypeBuilder.ts (2)
124-151: LGTM: Well-implemented error handling for optional injections.The error handling implementation correctly:
- Attempts to find the prototype
- Gracefully handles missing prototypes for optional injections
- Preserves error throwing for non-optional injections
- Maintains type safety with proper error instance checks
Line range hint
101-151: Verify optional injection usage across the codebase.Let's verify the implementation of optional injection feature and its error handling.
✅ Verification successful
Let me gather more specific information about the optional injection implementation and error handling.
Let me check the test files specifically to verify the optional injection behavior testing.
Based on the gathered information, I can now provide a final response:
Optional injection implementation is correctly handled with proper error handling
- The implementation in
EggPrototypeBuilder.tscorrectly handles optional injection by:
- Setting the optional flag appropriately in the injection configuration
- Properly catching and handling
EggPrototypeNotFounderrors for optional dependencies- The feature is well-tested in
LoadUnit.test.tswith specific test cases for optional injection- The implementation is consistently used across the codebase, with proper integration in
GlobalGraph.tsandAppGraph.tsfor dependency resolution🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of optional injection and error handling patterns # Test 1: Find usage of optional injection echo "=== Optional Injection Usage ===" rg -l "optional\s*[:=]\s*true" --type ts # Test 2: Check error handling patterns for injection echo -e "\n=== Injection Error Handling Patterns ===" ast-grep --pattern 'try { $$$ inject($$$) $$$ } catch (e) { $$$ }'Length of output: 705
Script:
#!/bin/bash # Check the implementation details of optional injection # Look for optional injection property definitions echo "=== Optional Injection Property Definitions ===" rg "optional.*=.*true" -B 2 -A 2 --type ts # Look for EggPrototypeNotFound error handling echo -e "\n=== EggPrototypeNotFound Error Handling ===" rg "EggPrototypeNotFound.*optional" -B 2 -A 2 --type ts # Check test cases for optional injection echo -e "\n=== Optional Injection Test Cases ===" rg "optional.*inject" -B 2 -A 2 --type ts test/Length of output: 2351
Script:
#!/bin/bash # Search for optional injection tests in the correct test directories # Find test files related to optional injection echo "=== Optional Injection Test Files ===" fd test -e ts -x rg -l "optional.*inject" # Check test assertions for optional injection echo -e "\n=== Optional Injection Test Assertions ===" fd test -e ts -x rg "optional.*inject" -B 3 -A 3 # Look for EggPrototypeNotFound test cases echo -e "\n=== EggPrototypeNotFound Test Cases ===" fd test -e ts -x rg "EggPrototypeNotFound" -B 3 -A 3Length of output: 1177
core/core-decorator/test/decorators.test.ts (1)
93-94: Verify optional flag in constructor injection tests.The test verifies the presence of optional parameters but doesn't explicitly check if they're marked as optional in the metadata.
Let's verify the implementation of the constructor injection:
core/metadata/test/LoadUnit.test.ts (1)
80-80: Verify the fixture module structure.Let's ensure the fixture module is properly set up with the necessary test cases.
✅ Verification successful
Fixture module is properly structured and implements optional injection patterns ✅
The fixture module correctly implements the test cases for optional injection with:
- A service class
OptionalInjectServicedemonstrating both injection patterns:
@Inject({ optional: true })syntax@InjectOptional()shorthand decorator- Proper imports from
@eggjs/core-decorator- An empty interface
PersistenceServicesuitable for testing optional dependency scenarios🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the optional inject module structure and implementation # Check if the fixture module exists if [ ! -d "./core/metadata/test/fixtures/modules/optional-inject-module" ]; then echo "Error: Fixture module directory not found" exit 1 fi # Look for the service implementation echo "Searching for OptionalInjectService implementation:" rg -t typescript "class OptionalInjectService" ./core/metadata/test/fixtures/modules/optional-inject-module # Look for optional inject decorators echo "Checking optional inject usage:" rg -t typescript "@Inject.*optional.*true|@InjectOptional" ./core/metadata/test/fixtures/modules/optional-inject-moduleLength of output: 565
Script:
#!/bin/bash # Let's try a different approach using fd to find files and cat to view their contents # Find all files in the optional-inject-module directory echo "Files in optional-inject-module directory:" fd . "core/metadata/test/fixtures/modules/optional-inject-module" # Display contents of any .ts files found echo -e "\nContents of TypeScript files:" fd -e ts . "core/metadata/test/fixtures/modules/optional-inject-module" --exec cat {}Length of output: 795
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (16)
plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts (2)
8-10: Consider adding error handling for demonstration purposes.While this is a test fixture, it might be helpful to add error handling to demonstrate the difference between optional and non-optional injection behavior.
bar() { - console.log(this.doesNotExist); + try { + console.log(this.doesNotExist); + } catch (error) { + console.error('Expected error: Property injection failed'); + throw error; + } }
5-6: Document the purpose of this negative test case.Since this is a test fixture demonstrating invalid injection, consider adding a comment to clarify its purpose.
+ // This property intentionally uses non-optional injection with a non-existent dependency + // to verify that the framework correctly handles missing required dependencies @Inject() doesNotExist: object;plugin/tegg/test/fixtures/apps/invalid-inject/config/plugin.js (1)
13-13: Consider documenting the purpose of disabled watcher.While disabling the watcher is valid, it would be helpful to add a comment explaining why it's disabled in this test fixture, especially if it relates to testing invalid injection scenarios.
-exports.watcher = false; +// Disable watcher to ensure consistent behavior during injection validation tests +exports.watcher = false;plugin/tegg/test/fixtures/apps/optional-inject/config/plugin.js (1)
13-13: Consider documenting the watcher configuration.While disabling the watcher is valid for tests, adding a comment explaining why it's disabled would improve maintainability.
+// Disable watcher for injection tests as it's not needed exports.watcher = false;standalone/standalone/test/fixtures/optional-inject/bar.ts (1)
1-2: Document the difference between injection approaches.The code demonstrates two different ways to achieve optional injection:
@InjectOptional()and@Inject({ optional: true }). Consider adding documentation to clarify when to use each approach and if there's a preferred method.core/types/core-decorator/model/InjectConstructorInfo.ts (1)
16-19: Consider enhancing the documentation.The JSDoc comment could be more descriptive to help developers understand the behavior. Consider expanding it to explain what happens when the injection is optional vs required.
/** - * optional inject + * Indicates whether this dependency is optional. + * When true, the framework will not throw an error if the dependency cannot be resolved. + * When false or undefined, the framework will throw an error if the dependency is missing. */ optional?: boolean;standalone/standalone/test/fixtures/invalid-inject/foo.ts (1)
7-8: Add documentation to clarify the test case purpose.Since this is a test fixture for invalid injection scenarios, it would be helpful to add a comment explaining that this property intentionally uses non-optional injection to verify the behavior when the dependency is missing.
+ // Intentionally using non-optional injection to verify behavior with missing dependency @Inject() doesNotExist?: object;plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts (1)
5-6: Consider documenting the expected type ofdoesNotExist1.While the injection is correctly implemented, the
objecttype is very broad. Consider being more specific about what type of object is expected to be injected here, or document why a genericobjecttype is appropriate for this test case.+ // Represents a non-existent dependency for testing optional injection @Inject({ optional: true }) - doesNotExist1: object; + doesNotExist1: Record<string, unknown>;plugin/tegg/test/fixtures/apps/invalid-inject/config/config.default.js (2)
1-1: Remove redundant 'use strict' directive.Since this is an ES module, the 'use strict' directive is unnecessary as modules are automatically in strict mode.
-'use strict';🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
14-16: Consider documenting security implications.The CSRF protection is explicitly enabled for JSON requests (
ignoreJSON: false). While this is generally good for security, it might affect how dependency injection behaves in test scenarios.Consider adding a comment explaining why CSRF protection is specifically configured for these tests:
security: { csrf: { + // Ensure CSRF protection is active for testing injection security ignoreJSON: false, }, },plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts (1)
5-8: Consider adding documentation and more specific types.While both injection approaches achieve similar results, it would be helpful to:
- Document why/when to use
@Inject({ optional: true })vs@InjectOptional()- Consider using a more specific type than
objectif possibleconstructor( + // Use @Inject({ optional: true }) when additional configuration might be needed @Inject({ optional: true }) readonly doesNotExist1: object, + // Use @InjectOptional() as a shorthand for simple optional dependencies @InjectOptional() readonly doesNotExist2: object, ) {}standalone/standalone/test/fixtures/optional-inject/foo.ts (1)
8-12: Consider standardizing optional injection approach.The code demonstrates two different ways to achieve optional injection:
@Inject({ optional: true })@InjectOptional()Consider standardizing on one approach throughout the codebase for better maintainability.
plugin/tegg/test/Inject.test.ts (2)
8-20: Add type annotation for the app variableConsider adding explicit type annotation for better type safety and code clarity.
- let app; + let app: any; // Or more specific type like `EggApplication`
50-59: LGTM: Error handling is well-implementedThe error case is properly tested with a specific error message. Consider extracting the error message to a constant for reusability and easier maintenance.
+ const PROTOTYPE_NOT_FOUND_ERROR = /EggPrototypeNotFound: Object doesNotExist not found in LOAD_UNIT:a/; it('should throw error if no proto found', async () => { // ... await assert.rejects( app.ready(), - /EggPrototypeNotFound: Object doesNotExist not found in LOAD_UNIT:a/, + PROTOTYPE_NOT_FOUND_ERROR, ); });standalone/standalone/test/index.test.ts (2)
188-202: Test suite needs more comprehensive coverage.While the test cases cover basic functionality and error handling, consider adding the following scenarios for better coverage:
- Optional injection with multiple dependencies
- Optional injection with mixed required and optional dependencies
- Edge cases like circular dependencies with optional injections
I can help generate additional test cases if needed.
189-193: Improve test case naming and assertions.The test case "should optional work" could be more descriptive. Consider renaming to "should handle optional injection successfully" for clarity. Also, the assertion could be more explicit about what
truerepresents in this context.- it('should optional work', async () => { + it('should handle optional injection successfully', async () => { const fixturePath = path.join(__dirname, './fixtures/optional-inject'); const nil = await main<boolean>(fixturePath); - assert.equal(nil, true); + assert.equal(nil, true, 'Optional injection should resolve successfully'); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (21)
- core/core-decorator/src/decorator/Inject.ts (3 hunks)
- core/core-decorator/test/decorators.test.ts (2 hunks)
- core/types/core-decorator/model/InjectConstructorInfo.ts (1 hunks)
- plugin/tegg/test/Inject.test.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/invalid-inject/config/config.default.js (1 hunks)
- plugin/tegg/test/fixtures/apps/invalid-inject/config/plugin.js (1 hunks)
- plugin/tegg/test/fixtures/apps/invalid-inject/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/config/config.default.js (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/config/plugin.js (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/package.json (1 hunks)
- standalone/standalone/test/fixtures/invalid-inject/foo.ts (1 hunks)
- standalone/standalone/test/fixtures/invalid-inject/package.json (1 hunks)
- standalone/standalone/test/fixtures/optional-inject/bar.ts (1 hunks)
- standalone/standalone/test/fixtures/optional-inject/foo.ts (1 hunks)
- standalone/standalone/test/fixtures/optional-inject/package.json (1 hunks)
- standalone/standalone/test/index.test.ts (1 hunks)
✅ Files skipped from review due to trivial changes (6)
- plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/package.json
- plugin/tegg/test/fixtures/apps/invalid-inject/package.json
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/package.json
- plugin/tegg/test/fixtures/apps/optional-inject/package.json
- standalone/standalone/test/fixtures/invalid-inject/package.json
- standalone/standalone/test/fixtures/optional-inject/package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- core/core-decorator/src/decorator/Inject.ts
- core/core-decorator/test/decorators.test.ts
🧰 Additional context used
🪛 Biome
plugin/tegg/test/fixtures/apps/invalid-inject/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/tegg/test/fixtures/apps/invalid-inject/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/tegg/test/fixtures/apps/optional-inject/config/config.default.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/tegg/test/fixtures/apps/optional-inject/config/plugin.js
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
🔇 Additional comments (21)
plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts (1)
3-6: Verify test coverage for non-optional injection failure case.This appears to be a negative test case for non-optional injection. Since the PR adds optional injection support, we should verify that appropriate test cases exist to validate both the failure scenario (this case) and the success scenario with
@InjectOptional().✅ Verification successful
Let me gather more information about the test coverage by examining the test files and fixtures.
Let me check the actual test implementation to verify the coverage of both optional and non-optional injection cases.
Test coverage for non-optional injection is properly implemented
The test coverage is comprehensive and correctly validates both scenarios:
- The
invalid-injectfixture withBarServiceis used in a test that verifies the failure case, throwingEggPrototypeNotFoundwhen a non-optional dependency is missing- The
optional-injectfixtures are used to verify successful initialization when optional dependencies are missing🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for corresponding test cases that verify this fixture # Test 1: Look for test files that use this fixture echo "Searching for test files using BarService..." rg -l "invalid-inject.*BarService" --type ts # Test 2: Check for test cases using @InjectOptional echo "Checking for @InjectOptional test coverage..." rg "@InjectOptional" --type tsLength of output: 1000
Script:
#!/bin/bash # Check test files that verify both optional and non-optional injection # Look for test files related to injection echo "Searching for test files related to injection..." rg -l "describe.*inject" --type ts # Check implementation of invalid-inject test cases echo "Checking invalid-inject test implementation..." rg -A 10 "invalid-inject.*BarService" --type ts # Check implementation of optional-inject test cases echo "Checking optional-inject test implementation..." rg -A 10 "optional-inject.*FooService" --type tsLength of output: 1281
Script:
#!/bin/bash # Check the actual test implementations # Check Inject.test.ts content echo "Checking Inject.test.ts content..." rg -A 20 "describe.*Inject" plugin/tegg/test/Inject.test.ts # Check LoadUnit.test.ts content for injection tests echo "Checking LoadUnit.test.ts content..." rg -A 20 "describe.*inject" core/metadata/test/LoadUnit.test.ts # Look for error cases in test files echo "Checking for error test cases..." rg -A 10 "should.*throw|expect.*throw" plugin/tegg/test/Inject.test.tsLength of output: 3855
plugin/tegg/test/fixtures/apps/invalid-inject/config/plugin.js (2)
3-11: LGTM: Plugin configuration follows standard format.The configuration for
egg-tracerandtegg-configplugins follows the standard Egg.js plugin format and both are appropriately enabled for testing dependency injection scenarios.
1-13: Verify test coverage for invalid injection scenarios.This test fixture appears to be set up for testing invalid injection scenarios. Since this PR adds optional injection support, please ensure that the test cases cover:
- Invalid optional injection configurations
- Edge cases where optional dependencies are unavailable
- Error handling for misconfigured optional injections
✅ Verification successful
Test coverage for invalid injection is appropriate and distinct from optional injection tests
The
invalid-injectfixture correctly tests the error case for non-optional injection by using@Inject()without the optional flag, which appropriately throws an error when the injected dependency is not found. This is complemented by separate test cases for optional injection in theoptional-injectfixture, making the test coverage comprehensive for both scenarios.Key test validations:
- Invalid injection: Verifies error thrown when required dependency is missing
- Optional injection: Separate test cases for both property and constructor injection patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for related test cases in the invalid-inject test directory rg -l "InjectOptional|@Inject.*optional" "plugin/tegg/test/fixtures/apps/invalid-inject/"Length of output: 89
Script:
#!/bin/bash # Let's first check what test files exist in the invalid-inject directory fd . "plugin/tegg/test/fixtures/apps/invalid-inject/" -t f # Then let's look for any test files that might contain injection-related tests rg -l "inject|Inject" "plugin/tegg/test/fixtures/apps/invalid-inject/" # Also check the actual test files that might use this fixture rg -l "invalid-inject" "plugin/tegg/test/"Length of output: 637
Script:
#!/bin/bash # Let's examine the content of BarService.ts and Inject.test.ts to understand the test coverage cat "plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts" echo "---" cat "plugin/tegg/test/Inject.test.ts" | grep -A 10 "invalid-inject"Length of output: 1363
Script:
#!/bin/bash # Let's check the actual Inject.test.ts content since previous command didn't show it cat "plugin/tegg/test/Inject.test.ts" # Also let's verify the correct invalid-inject directory content since we got optional-inject content cat "plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts"Length of output: 1966
🧰 Tools
🪛 Biome
[error] 1-1: Redundant use strict directive.
The entire contents of JavaScript modules are automatically in strict mode, with no statement needed to initiate it.
Safe fix: Remove the redundant use strict directive.(lint/suspicious/noRedundantUseStrict)
plugin/tegg/test/fixtures/apps/optional-inject/config/plugin.js (1)
8-11: LGTM! Required configuration for testing.The teggConfig plugin is appropriately enabled as it's essential for dependency injection functionality.
standalone/standalone/test/fixtures/optional-inject/bar.ts (2)
4-5: LGTM!The class declaration follows TypeScript naming conventions and the singleton pattern is appropriately used.
1-10: Verify test coverage and documentation.Since this implements a new feature for optional injection, let's verify the test coverage and documentation.
✅ Verification successful
Based on the analysis of the test cases and documentation, I can now provide a final response:
Optional injection feature is properly implemented and tested
The feature for optional dependency injection is well covered:
Test cases exist in multiple locations:
core/core-decorator/test/decorators.test.ts- Tests both constructor and property injection with optional dependenciescore/metadata/test/AppGraph.test.ts- Tests optional module dependenciesstandalone/standalone/test/fixtures/optional-inject/- Contains integration test casesImplementation is properly documented in the codebase through test cases demonstrating:
@InjectOptional()decorator usage@Inject({ optional: true })configuration usage- Optional module dependencies handling
No additional test coverage or documentation is needed as the feature is sufficiently covered through existing test cases and examples.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other test cases covering optional injection echo "Checking for other test cases..." rg -l "InjectOptional|optional:\s*true" --type ts # Check for documentation updates echo "Checking for documentation updates..." fd -e md -x grep -l "optional.*inject" {}Length of output: 905
Script:
#!/bin/bash # Let's examine the test cases and implementation more closely # Check the implementation of InjectOptional decorator echo "=== InjectOptional Implementation ===" ast-grep --pattern 'export class InjectOptional' -A 10 # Check test cases content echo -e "\n=== Test Cases Content ===" echo "1. Core decorator test:" cat core/core-decorator/test/decorators.test.ts echo -e "\n2. Metadata test:" cat core/metadata/test/AppGraph.test.ts # Check if there's any documentation in README files echo -e "\n=== Documentation Check ===" fd README.md -x cat {}Length of output: 62281
core/types/core-decorator/model/InjectConstructorInfo.ts (1)
16-19: LGTM! The interface change is well-structured.The addition of the optional
optionalproperty is clean and maintains backward compatibility.standalone/standalone/test/fixtures/invalid-inject/foo.ts (1)
4-13: Verify if this is intended as a negative test case for invalid injection.This appears to be a test fixture for verifying the behavior of non-optional injection when the dependency doesn't exist. However, given that PR #254 adds support for optional inject, we should verify if this test case needs to be updated to explicitly test the non-optional injection behavior.
✅ Verification successful
This is a valid test case for non-optional injection behavior
The code is correctly testing the error scenario for non-optional injection. The test suite verifies that attempting to inject a non-existent dependency without the optional flag throws an
EggPrototypeNotFounderror. This complements the new optional injection tests found in theoptional-injectfixtures which verify both@Inject({ optional: true })and@InjectOptional()behaviors.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if there are corresponding test cases for both optional and non-optional injection # Test: Look for related test files and their assertions rg -l "invalid-inject|optional-inject" --type ts rg -A 5 -B 5 "doesNotExist" --type tsLength of output: 6892
plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts (3)
1-4: LGTM! Clean imports and class declaration.The imports are properly structured, and the
@SingletonProto()decorator is appropriately used to ensure single instance creation.
1-17: Verify error handling for invalid injection scenarios.While the class implements optional injection, we should verify how the framework handles edge cases like:
- Circular dependencies with optional injections
- Runtime type mismatches when dependencies are provided
#!/bin/bash # Search for error handling tests related to optional injection rg -A 5 "describe.*optional.*inject.*error" --type ts # Look for circular dependency tests rg -A 5 "circular.*dependency.*optional" --type ts
8-9: Verify consistency between both optional injection approaches.The code demonstrates two different approaches to optional injection. We should verify that both approaches produce identical runtime behavior.
plugin/tegg/test/fixtures/apps/invalid-inject/config/config.default.js (1)
5-20: Verify the test fixture's purpose.This configuration appears to be a standard Egg.js configuration, but it's placed in an
invalid-injecttest fixture directory. However, the configuration doesn't contain any specific settings related to testing invalid injection scenarios. This might make the test cases unclear or incomplete.Let's verify if there are related test cases that use this configuration:
Consider:
- Adding comments explaining how this configuration contributes to testing invalid injection scenarios
- Including specific configuration properties that would trigger invalid injection cases
- Renaming the fixture directory if it's not specifically testing invalid injections
Would you like assistance in enhancing this test fixture?
✅ Verification successful
Let me gather more specific information about the test cases.
Let me check the content of the BarService.ts file and the test case more thoroughly.
Test fixture's purpose is correctly implemented and verified
The test fixture is correctly designed to test invalid injection scenarios. The
BarService.tscontains an@Inject()decorator trying to inject a non-existent dependencydoesNotExist, and the test case verifies that the application throws an appropriate error (EggPrototypeNotFound) when such invalid injection is attempted. The configuration file is a standard setup needed to bootstrap the test application.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test files that reference this invalid-inject fixture rg -l "invalid-inject" --type ts --type js | grep "test" # Search for the actual injection test cases ast-grep --pattern 'describe($_, function() { $$$ it($_, function() { $$$ inject($$$) $$$ }) $$$ })'Length of output: 250
Script:
#!/bin/bash # Check the content of the test files that mention invalid-inject rg -A 10 "invalid-inject" plugin/tegg/test/Inject.test.ts # Look for any other files in the invalid-inject fixture directory fd . plugin/tegg/test/fixtures/apps/invalid-inject/ # Check if there are any specific injection-related files in the fixture rg -l "inject" plugin/tegg/test/fixtures/apps/invalid-inject/Length of output: 1015
Script:
#!/bin/bash # Check the content of BarService.ts cat plugin/tegg/test/fixtures/apps/invalid-inject/app/modules/module-a/BarService.ts # Get more context around the test case rg -B 5 -A 15 "invalid-inject" plugin/tegg/test/Inject.test.tsLength of output: 686
plugin/tegg/test/fixtures/apps/optional-inject/config/config.default.js (1)
5-20: Verify the configuration's relationship to optional inject testing.While the configuration appears valid, it's not clear how this setup demonstrates or tests the optional inject feature. Consider adding configuration properties that explicitly test optional dependency injection scenarios.
Would you like help adding configuration properties that demonstrate optional inject scenarios?
plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts (3)
1-1: LGTM! Imports are correctly specified.
3-4: LGTM! Class declaration follows best practices.
1-16: Verify test coverage for both injection approaches.Since this is a test fixture, we should ensure there are corresponding test cases that verify both optional injection approaches.
✅ Verification successful
Let me check the content of the test files to verify the coverage.
Let me try a different approach to check the test coverage.
Let me check the BarService implementation to see how it tests property injection.
Test coverage for both optional injection approaches is adequate
The test fixture and its corresponding test cases adequately verify both optional injection approaches:
Constructor injection (FooService):
@Inject({ optional: true })@InjectOptional()Property injection (BarService):
@Inject({ optional: true })@InjectOptional()Both cases are tested in
Inject.test.tswith proper assertions verifying that the injected dependencies are undefined (resulting in 'Y' output) when the optional dependencies are not available.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for test cases using this fixture echo "Searching for test cases using FooService..." rg -l "FooService" --type ts --glob "**/*test*" # Search for other optional injection test cases echo "Searching for other optional injection tests..." rg -l "optional.*inject" --type ts --glob "**/*test*"Length of output: 526
Script:
#!/bin/bash # Check content of Inject.test.ts which seems most relevant echo "=== Content of plugin/tegg/test/Inject.test.ts ===" rg -A 10 "FooService" plugin/tegg/test/Inject.test.ts # Also check for specific optional injection test cases echo "=== Optional injection test cases ===" rg "describe.*optional.*inject" plugin/tegg/test/Inject.test.ts rg "it.*optional.*inject" plugin/tegg/test/Inject.test.tsLength of output: 1244
Script:
#!/bin/bash # Get more context around the test case echo "=== Test context for optional injection ===" rg -B 5 -A 15 "optional-inject" plugin/tegg/test/Inject.test.ts # Check the actual test implementation echo "=== Full test implementation ===" cat plugin/tegg/test/Inject.test.tsLength of output: 3039
Script:
#!/bin/bash # Check BarService implementation echo "=== Content of BarService.ts ===" cat plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.tsLength of output: 503
standalone/standalone/test/fixtures/optional-inject/foo.ts (1)
1-20: Verify test coverage for optional injection scenarios.While this test fixture covers the basic cases of optional injection, consider adding more test cases to verify:
- Runtime behavior when optional dependencies are provided
- Error cases and edge conditions
- Interaction with other decorators
✅ Verification successful
Based on the search results, I can now generate a final response as we have found comprehensive test coverage for optional injection across the codebase.
Optional injection test coverage is sufficient ✓
The codebase has thorough test coverage for optional injection scenarios across multiple test fixtures:
- Core decorator tests (
core/core-decorator/test/) cover both@Inject({ optional: true })and@InjectOptional()syntax- Integration tests in plugin (
plugin/tegg/test/fixtures/apps/optional-inject/) verify runtime behavior with missing dependencies- Standalone tests demonstrate usage in both constructor and property injection
- Tests exist across different contexts: standalone, plugin, and core modules
The test cases cover:
- Both syntax variants for optional injection
- Constructor and property injection scenarios
- Integration with other decorators (e.g.,
@SingletonProto)- Runtime behavior when dependencies are missing
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for other test cases covering optional injection echo "Searching for other test files related to optional injection..." fd -e ts -e js --full-path "optional.*inject.*test" echo "Checking for usage patterns of optional injection in tests..." rg -t ts -t js "optional:\s*true|InjectOptional" --glob "test/**"Length of output: 540
Script:
#!/bin/bash # Let's try a different search strategy # Search for test files containing 'inject' in their name echo "=== Test files with 'inject' in name ===" fd -t f "inject" "test/" # Search for InjectOptional usage in the entire codebase echo -e "\n=== InjectOptional usage patterns ===" rg "InjectOptional" -A 3 # Search for optional injection patterns echo -e "\n=== Optional injection patterns ===" rg "@Inject.*optional.*true" -A 3 # Look for test files that might test injection functionality echo -e "\n=== Test files containing Inject ===" rg "describe.*Inject" "test/"Length of output: 8848
plugin/tegg/test/Inject.test.ts (3)
1-6: LGTM: Imports are well-organizedThe imports include all necessary testing utilities and test fixtures.
22-48: Enhance test coverage and descriptionsWhile the basic functionality is well-tested, consider adding the following test cases for more comprehensive coverage:
- Test partial optional injections (some dependencies present, others missing)
- Test runtime behavior when optional dependencies become available/unavailable
- Test error cases for invalid optional dependencies
Also, consider making test descriptions more descriptive:
- it('should work with property', async () => { + it('should handle optional property injection when dependencies are missing', async () => {Let's check if there are any other test files for optional injection:
1-60: Verify test inclusion in the test suiteThe test file looks well-structured overall. Let's verify it's properly included in the test suite configuration.
✅ Verification successful
Test file is properly included in the test suite
The verification shows that:
- The test file is in the correct location at
plugin/tegg/test/Inject.test.ts- The package.json in plugin/tegg directory has proper test configuration using mocha
- Tests are executed through npm workspaces from the root package.json
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if this test file is included in test configuration rg -l "Inject\.test\.ts" --type jsonLength of output: 38
Script:
#!/bin/bash # Let's try different approaches to verify test inclusion # 1. Check common test configuration files fd -e json "test|jest|mocha|package" # 2. Look for test patterns in package.json rg "test.*pattern|\"test\":" package.json # 3. Check if the test file is discovered by the test runner fd -e ts "Inject.test.ts" # 4. Look for test configuration in tsconfig fd "tsconfig" -e jsonLength of output: 17286
Script:
#!/bin/bash # Let's check the root package.json and project's test configuration cat package.json # Also check the plugin/tegg/package.json since our test is in that directory cat plugin/tegg/package.jsonLength of output: 3479
standalone/standalone/test/index.test.ts (1)
195-201: Verify error message consistency.The error message pattern being tested should be documented and consistent across the codebase. Let's verify this.
✅ Verification successful
Let me search for the error message definition to verify its consistency.
Error message pattern is consistent and well-defined
The error message pattern is correctly implemented and consistently used across the codebase:
- Error class is defined in
core/metadata/src/errors.ts- Error is thrown with consistent format using
FrameworkErrorFormaterin core modules- Test cases in both
standalone/testandplugin/tegg/testverify the same error pattern🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for similar error message patterns across the codebase rg -g '*.ts' -g '*.js' "EggPrototypeNotFound.*not found in LOAD_UNIT" -A 2 -B 2Length of output: 754
Script:
#!/bin/bash # Search for the error definition ast-grep --pattern 'EggPrototypeNotFound' # Also search for any string containing this error rg "EggPrototypeNotFound" -A 2 -B 2Length of output: 5663
plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts
Show resolved
Hide resolved
plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
README.md (2)
484-495: Consider enhancing the InjectOptional documentation.While the documentation is functionally correct, it could be more helpful by:
- Adding a brief explanation of why one might choose
@InjectOptionalover@Inject({ optional: true }).- Including a note about TypeScript's handling of optional properties (the
?modifier).Consider adding:
// 等价于 @Inject({ ...params, optional: true }) +// 使用 @InjectOptional 可以让代码更简洁，特别是在处理可选依赖时 +// 注意：使用此装饰器的属性应该使用 TypeScript 的可选属性修饰符 (?) @InjectOptional(params: {
536-546: Constructor injection example needs type safety improvement.While the example is functionally correct, it could be enhanced to better demonstrate TypeScript best practices.
Consider adding:
constructor( - @Inject() readonly logger: EggLogger, - @InjectOptional() readonly maybeUndefinedLogger?: EggLogger, + @Inject() private readonly logger: EggLogger, + @InjectOptional() private readonly maybeUndefinedLogger?: EggLogger | undefined, ) {}This change:
- Uses
privatefor better encapsulation- Explicitly declares the undefined type for better type safety
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- README.md (3 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/metadata/src/impl/EggPrototypeBuilder.ts
🔇 Additional comments (2)
README.md (2)
477-481: Documentation for optional parameter looks good!The explanation of the
optionalparameter is clear and concise, effectively communicating both the default behavior and the implications of setting it to true/false.
511-521: Example demonstrates good practices!The example effectively shows:
- Both required and optional injection usage
- Proper null checking for optional dependencies
- Clear comparison between the two approaches
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts (2)
3-4: Consider using a type alias instead of an empty interface.While this empty interface might be intentional for testing purposes, using a type alias would be more idiomatic TypeScript.
-interface PersistenceService { -} +type PersistenceService = Record<string, unknown>;🧰 Tools
🪛 Biome
[error] 2-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
6-13: Add documentation to clarify the different injection approaches.The implementation correctly demonstrates two ways to achieve optional injection. However, consider adding JSDoc comments to explain the purpose of using different approaches and their implications.
@SingletonProto() +/** + * Test fixture demonstrating different approaches for optional dependency injection: + * 1. Using @Inject({ optional: true }) - explicit configuration approach + * 2. Using @InjectOptional() - shorthand decorator approach + */ export default class OptionalInjectService {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/FooService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- plugin/tegg/test/fixtures/apps/optional-inject/app/modules/module-a/BarService.ts
🧰 Additional context used
🪛 Biome
core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts
[error] 2-4: An empty interface is equivalent to {}.
Safe fix: Use a type alias instead.
(lint/suspicious/noEmptyInterface)
core/metadata/test/fixtures/modules/optional-inject-module/OptionalInjectService.ts
Show resolved
Hide resolved
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Release Notes
New Features
InjectOptionaldecorator for marking parameters as optional.Improvements
EggPrototypeBuilderfor optional inject objects.Injectdecorator.Tests
Documentation