feat: impl MultiInstanceInfo decorator #239
Conversation
Add multi instances info for constructor inject mode.
WalkthroughThe changes introduce a new decorator, Changes
Possibly related PRs
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: 8
🧹 Outside diff range and nitpick comments (4)
core/metadata/src/impl/EggPrototypeImpl.ts (1)
44-45: LGTM: Constructor updated correctly, with a minor suggestionThe constructor has been properly updated to include the new properties:
- The signature includes the new optional parameters with correct types.
- The new properties are correctly initialized in the constructor body.
For consistency with other optional parameters, consider adding default values:
constructor( // ... other parameters ... - multiInstanceConstructorIndex?: number, - multiInstanceConstructorAttributes?: QualifierAttribute[], + multiInstanceConstructorIndex: number | undefined = undefined, + multiInstanceConstructorAttributes: QualifierAttribute[] | undefined = undefined, ) { // ... other initializations ... this.multiInstanceConstructorIndex = multiInstanceConstructorIndex; this.multiInstanceConstructorAttributes = multiInstanceConstructorAttributes; }This change would make the new parameters consistent with the
injectTypeparameter, which has a default value.Also applies to: 58-59
core/core-decorator/src/util/PrototypeUtil.ts (1)
159-165: LGTM: New methods for managing multi-instance constructor indexThe
setMultiInstanceConstructorIndexandgetMultiInstanceConstructorIndexmethods are well-implemented:
- They use
MetadataUtilconsistently with other methods in the class.- The methods follow the existing code style and naming conventions.
For consistency with the
getMultiInstanceConstructorAttributesmethod, consider initializing the index with a default value in the getter:static getMultiInstanceConstructorIndex(clazz: EggProtoImplClass): number | undefined { - return MetadataUtil.getMetaData(PrototypeUtil.MULTI_INSTANCE_CONSTRUCTOR_INDEX, clazz); + return MetadataUtil.getMetaData(PrototypeUtil.MULTI_INSTANCE_CONSTRUCTOR_INDEX, clazz) ?? -1; }This change would ensure that the method always returns a number, making it easier to use in other parts of the codebase.
core/metadata/src/impl/EggPrototypeBuilder.ts (1)
38-39: Add documentation for new propertiesTo improve code maintainability and readability, consider adding JSDoc comments for the newly added properties
multiInstanceConstructorIndexandmultiInstanceConstructorAttributesto explain their purpose and usage.core/runtime/src/impl/EggObjectImpl.ts (1)
Line range hint
95-106: Consider handling emptyinjectObjectsarrayWhen mapping over
this.proto.injectObjects!, ifinjectObjectsis an empty array,Promise.allwill resolve to an empty array. Ensure that this is the intended behavior and thatconstructArgswill be correctly formed even when there are no inject objects.If necessary, you can add a check or a comment to clarify that this case has been considered.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- core/core-decorator/index.ts (1 hunks)
- core/core-decorator/src/decorator/MultiInstanceInfo.ts (1 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (3 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (4 hunks)
- core/metadata/src/impl/EggPrototypeImpl.ts (4 hunks)
- core/runtime/src/impl/EggObjectImpl.ts (2 hunks)
- core/runtime/test/LoadUnitInstance.test.ts (2 hunks)
- core/runtime/test/fixtures/modules/multi-instance-module/MultiInstanceConstructor.ts (1 hunks)
- core/types/metadata/model/EggPrototype.ts (1 hunks)
🔇 Additional comments (14)
core/core-decorator/src/decorator/MultiInstanceInfo.ts (2)
1-2: LGTM: Imports are appropriate.The imports are correctly structured and seem to provide the necessary dependencies for the implementation.
1-9: Overall, the implementation looks good with room for minor improvements.The
MultiInstanceInfodecorator is well-structured and focused on its purpose. The suggested improvements in type safety and input validation will enhance its robustness. Consider adding unit tests to verify the behavior of this decorator, especially with different input scenarios.To ensure proper test coverage, let's check for existing tests:
core/core-decorator/index.ts (1)
10-10: LGTM! Verify implementation and update documentation.The new export for
MultiInstanceInfois correctly added and follows the existing pattern in the file. This aligns with the PR objective of implementing the MultiInstanceInfo decorator.To ensure completeness:
- Verify that the
MultiInstanceInfodecorator is properly implemented in./src/decorator/MultiInstanceInfo.ts.- Update the module's documentation to include information about this new decorator.
Run the following script to check the implementation:
core/metadata/src/impl/EggPrototypeImpl.ts (3)
1-1: LGTM: Import statement updated correctlyThe addition of
QualifierAttributeto the import statement is consistent with its usage in the new propertymultiInstanceConstructorAttributes.
29-30: LGTM: New properties added correctlyThe new properties
multiInstanceConstructorIndexandmultiInstanceConstructorAttributesare well-defined:
- They are correctly declared as readonly, which is good for immutability.
- The optional modifier (?) is appropriate, as these properties might not always be needed.
- The types are consistent with their intended use.
- The naming is clear and descriptive.
Line range hint
1-59: Summary: MultiInstanceInfo implementation looks goodThe changes to
EggPrototypeImplsuccessfully implement the MultiInstanceInfo functionality:
- New properties
multiInstanceConstructorIndexandmultiInstanceConstructorAttributesare added.- The constructor is updated to initialize these properties.
- Import statements are correctly updated.
These changes enhance the class's capability to handle multi-instance constructors without altering existing functionality, aligning well with the PR objectives.
To ensure the changes are used correctly throughout the codebase, run the following script:
✅ Verification successful
Verification Successful: MultiInstanceInfo properties are correctly utilized
The new properties
multiInstanceConstructorIndexandmultiInstanceConstructorAttributesare actively used across the codebase:
multiInstanceConstructorIndex
core/types/metadata/model/EggPrototype.tscore/runtime/src/impl/EggObjectImpl.tscore/metadata/src/impl/EggPrototypeBuilder.tscore/metadata/src/impl/EggPrototypeImpl.tscore/core-decorator/src/util/PrototypeUtil.tsmultiInstanceConstructorAttributes
core/core-decorator/src/util/PrototypeUtil.tscore/metadata/src/impl/EggPrototypeImpl.tscore/metadata/src/impl/EggPrototypeBuilder.tscore/types/metadata/model/EggPrototype.tscore/runtime/src/impl/EggObjectImpl.tsNo related TODOs were found, ensuring there are no pending tasks concerning these changes. The implementation aligns with the PR objectives, enhancing the class's capability to handle multi-instance constructors without impacting existing functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify usage of new MultiInstanceInfo properties # Test: Search for usages of the new properties echo "Searching for usages of multiInstanceConstructorIndex:" rg --type typescript 'multiInstanceConstructorIndex' echo "Searching for usages of multiInstanceConstructorAttributes:" rg --type typescript 'multiInstanceConstructorAttributes' # Test: Check for any TODOs related to these changes echo "Checking for related TODOs:" rg --type typescript -i 'todo.*multiinstance'Length of output: 576
Script:
#!/bin/bash # Description: Verify usage of new MultiInstanceInfo properties without specifying file type # Test: Search for usages of the new properties echo "Searching for usages of multiInstanceConstructorIndex:" rg 'multiInstanceConstructorIndex' echo "Searching for usages of multiInstanceConstructorAttributes:" rg 'multiInstanceConstructorAttributes' # Test: Check for any TODOs related to these changes echo "Checking for related TODOs:" rg -i 'todo.*multiinstance'Length of output: 2618
core/types/metadata/model/EggPrototype.ts (1)
112-113: LGTM! Consider adding documentation for new properties.The addition of
multiInstanceConstructorIndexandmultiInstanceConstructorAttributesaligns well with the PR objective of implementing functionality for handling multiple instances information in the constructor injection mode. These optional properties maintain backward compatibility while extending the interface's capabilities.Consider adding JSDoc comments for these new properties to improve clarity:
/** * Index of the constructor parameter that handles multiple instances. */ readonly multiInstanceConstructorIndex?: number; /** * Additional metadata for qualifying multiple instances. */ readonly multiInstanceConstructorAttributes?: QualifierAttribute[];Let's verify if these changes are consistent across the codebase:
✅ Verification successful
Let's adjust the shell scripts to correctly target TypeScript files by specifying the file patterns directly.
LGTM! The added properties are consistently used across the codebase. Consider adding documentation for the new properties.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of new properties and related multi-instance functionality # Test 1: Search for usages of multiInstanceConstructorIndex echo "Searching for multiInstanceConstructorIndex usage:" rg --type typescript "multiInstanceConstructorIndex" # Test 2: Search for usages of multiInstanceConstructorAttributes echo "Searching for multiInstanceConstructorAttributes usage:" rg --type typescript "multiInstanceConstructorAttributes" # Test 3: Search for other multi-instance related functionality echo "Searching for other multi-instance related functionality:" rg --type typescript "multiInstance"Length of output: 609
Script:
# #!/bin/bash # Description: Check for usage of new properties and related multi-instance functionality # Test 1: Search for usages of multiInstanceConstructorIndex echo "Searching for multiInstanceConstructorIndex usage in .ts and .tsx files:" rg -g "*.ts" -g "*.tsx" "multiInstanceConstructorIndex" # Test 2: Search for usages of multiInstanceConstructorAttributes echo "Searching for multiInstanceConstructorAttributes usage in .ts and .tsx files:" rg -g "*.ts" -g "*.tsx" "multiInstanceConstructorAttributes" # Test 3: Search for other multi-instance related functionality echo "Searching for other multi-instance related functionality in .ts and .tsx files:" rg -g "*.ts" -g "*.tsx" "multiInstance"Length of output: 7033
core/runtime/test/LoadUnitInstance.test.ts (1)
Line range hint
1-141: Overall, good addition to the test suite with room for minor improvements.The new test case "should load multi instance with constructor" is a valuable addition to the test suite. It extends the coverage to include constructor-based multi-instance creation, which is important for ensuring the robustness of the
MultiInstanceInfodecorator implementation.After addressing the issues mentioned in the previous comment, this test will provide a solid verification of the multi-instance functionality with constructors. Good job on maintaining test coverage alongside feature development.
core/core-decorator/src/util/PrototypeUtil.ts (3)
10-10: LGTM: New import added forQualifierAttributeThe addition of the
QualifierAttributeimport is appropriate for the new functionality being implemented.
25-26: LGTM: New symbols added for multi-instance constructor metadataThe addition of
MULTI_INSTANCE_CONSTRUCTOR_INDEXandMULTI_INSTANCE_CONSTRUCTOR_ATTRIBUTESsymbols is consistent with the existing code style and naming conventions. These symbols will be used to manage metadata for multi-instance constructors.
151-157: LGTM: New methods for managing multi-instance constructor attributesThe
setMultiInstanceConstructorAttributesandgetMultiInstanceConstructorAttributesmethods are well-implemented:
- They use
MetadataUtilconsistently with other methods in the class.- The getter method returns an empty array if no attributes are found, which is a good practice.
- The methods follow the existing code style and naming conventions.
core/runtime/test/fixtures/modules/multi-instance-module/MultiInstanceConstructor.ts (1)
37-40: Constructor correctly initializes properties with injected valuesThe constructor effectively uses
@Inject()to inject an instance ofBarand@MultiInstanceInfo()to access theObjectInfo. The propertiesfooandbarare properly initialized using the qualifier value and the injectedBarinstance, respectively.core/metadata/src/impl/EggPrototypeBuilder.ts (1)
2-2: Import ofQualifierAttributeis appropriateThe import statement correctly includes
QualifierAttributefrom@eggjs/core-decorator, which is used later in the code.core/runtime/src/impl/EggObjectImpl.ts (1)
118-118: EnsuremultiInstanceConstructorIndexis within boundsWhen using
constructArgs.splice(this.proto.multiInstanceConstructorIndex, 0, objInfo);, there's a risk ofmultiInstanceConstructorIndexbeing out of the bounds of theconstructArgsarray, which could lead to unexpected behavior.Verify that
multiInstanceConstructorIndexis a valid index within the range ofconstructArgsto prevent potential runtime errors.Run the following script to check the validity of
multiInstanceConstructorIndex:
core/runtime/test/fixtures/modules/multi-instance-module/MultiInstanceConstructor.ts
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
plugin/tegg/typings/index.d.ts (1)
62-62: LGTM! Consider adding JSDoc comments for clarity.The addition of
nameandqualifiersparameters to thegetEggObjectmethod signature enhances its flexibility and aligns with the PR objective of handling multiple instances. This change is well-implemented and consistent with the summary.Consider adding JSDoc comments to explain the purpose and usage of these new parameters. For example:
/** * Retrieves an egg object based on the provided class, name, and qualifiers. * @param clazz The class of the egg object to retrieve. * @param name Optional name to identify the egg object. * @param qualifiers Optional qualifiers to further specify the egg object. * @returns A promise that resolves to the requested egg object. */ getEggObject<T>(clazz: EggProtoImplClass<T>, name?: string, qualifiers?: QualifierInfo | QualifierInfo[]): Promise<T>;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- core/runtime/src/impl/EggObjectImpl.ts (3 hunks)
- plugin/tegg/typings/index.d.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- core/runtime/src/impl/EggObjectImpl.ts
🔇 Additional comments (2)
plugin/tegg/typings/index.d.ts (2)
71-71: LGTM! Consistent implementation across interfaces.The changes to the
getEggObjectmethod in theTEggContextinterface mirror those in theTEggApplicationinterface. This consistency is commendable and maintains coherence across the codebase.
Line range hint
1-85: Overall assessment: Well-implemented changes with minor suggestions.The modifications to the
getEggObjectmethod signatures in bothTEggApplicationandTEggContextinterfaces are well-implemented and align with the PR objectives. They enhance the flexibility of the egg module in handling multiple instances. Consider adding JSDoc comments to improve documentation, as suggested earlier.
|
Add multi instances info for constructor inject mode.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
MultiInstanceInfodecorator for enhanced multi-instance management.getEggObjectmethod to accept additional parameters for improved flexibility.Bug Fixes
EggObjectto handle multi-instance information.Tests
Documentation