feat: support inject in constructor #237
Conversation
WalkthroughThe changes enhance the dependency injection framework within the Egg.js ecosystem. Key updates include the addition of new decorators for managing injections, modifications to existing decorator functions to support constructor injection, and the introduction of new utility methods for handling metadata and constructor argument names. New classes and interfaces have been created to facilitate these features, along with corresponding test cases to ensure functionality. Additionally, several configuration files and modules have been added to support dynamic features and application structure. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant Controller
participant Service
User->>App: Request /config
App->>Controller: Call baseDir()
Controller->>Service: Retrieve configuration
Service-->>Controller: Return config values
Controller-->>App: Send response with config
App-->>User: Return response
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
🔇 Additional comments (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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (39)
plugin/tegg/test/fixtures/apps/constructor-module-config/package.json (1)
1-3: LGTM! Minimal package.json for test fixture.The package.json file is appropriately minimal for a test fixture, containing only the essential "name" field. The name "egg-app" is suitable for a generic test application.
Consider adding a "private": true field to prevent accidental publication of this test package. Additionally, if the test case requires simulating a more realistic package.json, you might want to include fields like "version" and "description". However, this is optional and depends on the specific testing requirements.
plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/module.unittest.yml (1)
1-3: LGTM! Consider adding documentation for this configuration file.The YAML structure is correct, and the configuration seems appropriate for its purpose. As this file is located in a test fixtures directory and named with 'unittest', it's clear that it's intended for testing scenarios.
Consider adding a comment at the top of the file to explain its purpose and how it's used in the unit tests. This would help other developers understand the context quickly. For example:
# Configuration file for testing dynamic feature handling in module-with-config # Used in unit tests to verify proper loading and interpretation of module configurations features: dynamic: bar: 'foo'plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/module.yml (1)
1-3: LGTM! Consider expanding test coverage.The YAML structure is correct and provides a good basic test case for dynamic feature configuration. The
features.dynamic.foo: 'bar'entry should allow testing of the module's ability to read and utilize dynamic configuration settings.To enhance test coverage, consider adding more test cases:
- Include multiple key-value pairs under
dynamicto test handling of multiple configurations.- Add different data types (e.g., numbers, booleans, lists) to ensure proper parsing of various value types.
- Consider adding a comment to explain the purpose of this configuration file for other developers.
Example expansion:
features: dynamic: # Test cases for dynamic feature configuration foo: 'bar' number_test: 42 boolean_test: true list_test: - item1 - item2This expansion would provide more comprehensive test scenarios for the module's configuration handling capabilities.
core/types/core-decorator/enum/InjectType.ts (1)
1-4: LGTM! Consider minor optimizations and adding a comment.The
InjectTypeenum is well-structured and aligns with the PR objective of supporting constructor injection. It follows TypeScript best practices and provides clear semantics for different injection types.Consider the following suggestions:
- Remove redundant string values:
export enum InjectType { PROPERTY, CONSTRUCTOR, }This change doesn't affect functionality but makes the code more concise.
- Add a brief comment explaining the enum's purpose:
/** * Defines the types of dependency injection supported in the framework. * Used to distinguish between property injection and constructor injection. */ export enum InjectType { PROPERTY, CONSTRUCTOR, }These changes are optional and don't affect the functionality, but they can improve code readability and maintainability.
core/runtime/test/fixtures/modules/inject-constructor-context-to-singleton/package.json (1)
1-6: LGTM! The package.json file is correctly structured.The file is well-formed and includes the necessary information for an Egg.js module. The
eggModuleproperty with thenamefield set to "extendsModule" aligns with the PR objective of supporting injection in constructors.Consider adding more standard fields to the
package.jsonfile to improve its completeness:{ "name": "multi-module-service", + "version": "1.0.0", + "description": "A module for testing constructor injection in Egg.js", + "main": "index.js", + "scripts": { + "test": "echo \"Error: no test specified\" && exit 1" + }, "eggModule": { "name": "extendsModule" } }These additional fields provide more context about the module and can be helpful for maintenance and documentation purposes.
plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/package.json (1)
1-6: LGTM! Consider adding aversionfield for completeness.The
package.jsonfile is well-structured and correctly defines the module name andeggModuleconfiguration. This setup aligns with the PR objective of supporting dependency injection in constructors, as it provides the necessary module configuration for the Egg.js framework.For consistency with standard
package.jsonfiles, consider adding aversionfield:{ "name": "constructorSimple", + "version": "1.0.0", "eggModule": { "name": "constructorSimple" } }plugin/tegg/test/fixtures/apps/constructor-module-config/app/router.ts (1)
3-5: Consider adding a comment explaining the purpose of this route.Since this file is located in a test fixtures directory, it would be helpful to add a comment explaining that this route is intended for testing purposes. This can prevent confusion and potential security concerns if the code were mistakenly used in a production environment.
Here's a suggested modification:
import { Application } from 'egg'; module.exports = (app: Application) => { + // Test route: Returns the base directory for configuration testing app.router.get('/config', app.controller.app.baseDir); };plugin/tegg/test/fixtures/apps/constructor-module-config/config/plugin.js (2)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in JavaScript modules as they are always in strict mode by default.
Apply this diff to remove the redundant directive:
-'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)
13-13: LGTM: Watcher configuration is clear, consider adding a commentThe watcher configuration is correctly set to
false, disabling this functionality. This is likely intentional for this test fixture.Consider adding a brief comment explaining why the watcher is disabled in this context, for better clarity:
+// Disable watcher for this test fixture exports.watcher = false;plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/application.unittest.ts (1)
4-10: LGTM:mockUsermethod implementation is correct.The
mockUsermethod is well-implemented:
- It correctly uses
this: MockApplicationfor type safety.- The mock context is set up with a basic user object.
Consider enhancing the mock user object with additional properties that might be relevant for more comprehensive testing scenarios. For example:
this.mockContext({ user: { userName: 'mock_user', id: 'mock_id', email: '[email protected]', // Add any other relevant properties }, });This would provide a more robust mock user object for various test cases.
core/metadata/test/fixtures/modules/app-graph-modules/root/RootConstructor.ts (2)
4-5: LGTM: Class declaration is clear. Consider adding documentation.The
RootConstructorProtoclass is correctly declared and exported, using the@SingletonProto()decorator to ensure a single instance.Consider adding a brief JSDoc comment to describe the purpose of this class and explain the 'Proto' suffix in its name. This would enhance code readability and maintainability.
6-7: LGTM: Constructor correctly uses dependency injection. Consider adding type annotation.The constructor is well-implemented, using the
@Inject()decorator for dependency injection andreadonlyfor immutability.Consider adding an explicit return type annotation to the constructor for better code clarity:
constructor(@Inject() readonly usedProto: UsedProto): void { }This makes the constructor's return type explicit, although it's generally understood that constructors don't have a return type.
plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/context.ts (1)
4-9: LGTM: Well-implemented counter with a minor optimization suggestionThe
countergetter is well-implemented. It correctly initializes the counter if it doesn't exist and uses the post-increment operator to return the current value before incrementing.Consider using the pre-increment operator (
++this[COUNTER]) instead of the post-increment operator (this[COUNTER]++) if you want to return the incremented value. This is a minor optimization as it avoids creating a temporary copy of the value. However, if the current behavior (returning the value before incrementing) is intended, then the current implementation is correct.plugin/tegg/test/fixtures/apps/constructor-module-config/app/typings/index.d.ts (2)
1-3: Remove unused import.The
Bartype is imported but not used in this file. Consider removing this import to keep the code clean and avoid potential confusion.Apply this diff to remove the unused import:
import 'egg'; import { Foo } from '../../modules/module-with-config/foo'; -import { Bar } from '../../modules/module-with-overwrite-config/bar';
6-10: Enhance clarity and add documentation for the new property.While the interface extension is correctly implemented, consider the following improvements:
- The property name 'constructorSimple' might not be descriptive enough. Consider a more specific name that reflects its purpose or content.
- Add JSDoc comments to document the purpose and usage of this new property.
Here's a suggested improvement:
export interface EggModule { /** * Represents the configuration for constructor injection. * @property {Foo} foo - The Foo instance to be injected. */ constructorInjectionConfig: { foo: Foo; }, }core/types/core-decorator/model/InjectConstructorInfo.ts (2)
4-15: Consider standardizing JSDoc comment style.For improved consistency, consider standardizing the JSDoc comments. Currently, two styles are used: 'inject args index' and "obj's name will be injected". It's generally better to stick to one style throughout the codebase.
Here's a suggested improvement for consistency:
/** - * inject args index + * Inject args index */ refIndex: number; /** - * inject args name + * Inject args name */ refName: string; /** - * obj's name will be injected + * Name of the object to be injected */ objName: EggObjectName;
1-16: Summary: New interface aligns well with PR objectives.This new
InjectConstructorInfointerface provides a solid foundation for implementing constructor injection support, which directly addresses the main objective of the PR. The interface's structure allows for precise control over which dependencies are injected and where, enhancing the flexibility of the dependency injection system in the Egg.js framework.To further improve the implementation:
- Ensure that this interface is used consistently across the codebase where constructor injection is implemented.
- Consider adding unit tests that validate the correct usage of this interface in the injection process.
- Update the project documentation to explain how developers can use this new constructor injection feature, including examples of how to properly populate the
InjectConstructorInfointerface.core/core-decorator/src/decorator/InitTypeQualifier.ts (1)
6-7: LGTM! Consider enhancing type safety.The changes effectively support constructor injection by modifying the decorator function signature and updating the
QualifierUtilmethod call. This aligns well with the PR objective of supporting inject in constructors.To improve type safety, consider using more specific types for the
targetparameter:- return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { + return function(target: object, propertyKey?: PropertyKey, parameterIndex?: number) {This change ensures that only objects (which includes classes and their prototype chains) can be decorated, preventing potential misuse on primitive values.
core/types/core-decorator/index.ts (2)
9-9: LGTM! Consider updating documentation forInjectConstructorInfo.The addition of
InjectConstructorInfoexport is consistent with the file structure and supports the new constructor injection feature.To ensure proper understanding and usage of this new model, consider updating the relevant documentation to include information about
InjectConstructorInfoand its role in constructor injection.
5-9: Summary: New exports enhance dependency injection capabilities.The additions of
InjectTypeandInjectConstructorInfoexports are well-structured and align with the PR objective of supporting injection in constructors. These new types will likely play a crucial role in implementing and managing the new injection features throughout the codebase.As you continue to develop this feature:
- Ensure consistent usage of these new types across the codebase.
- Consider creating or updating developer documentation to explain the new injection capabilities and how to use them effectively.
- Be mindful of potential breaking changes for existing users of the dependency injection system.
core/core-decorator/test/fixtures/decators/ConstructorObject.ts (1)
10-16: LGTM: Constructor implements dependency injection as intended.The constructor correctly implements dependency injection using the
@Inject()decorator, which aligns with the PR objective. The use of additional qualifiers for thexCacheparameter and the readonly modifier for both parameters are good practices.Consider renaming the parameters for better clarity:
xCachecould be renamed tofooCacheto match its injection name.cachecould be more descriptive, e.g.,defaultCache.Example:
constructor( @InitTypeQualifier(ObjectInitType.SINGLETON) @ModuleQualifier('foo') @Inject({ name: 'fooCache'}) readonly fooCache: ICache, @Inject() readonly defaultCache: ICache, ) { }plugin/tegg/test/ConstructorModuleConfig.test.ts (1)
29-39: LGTM with suggestions: Test case is good, but could be more comprehensive.The test case correctly checks the
/configendpoint and asserts the expected response structure. However, consider the following suggestions:
- Add a comment explaining what ConstructorModuleConfig is and why these specific values are expected.
- Consider adding more test cases to cover different scenarios or edge cases.
- It might be helpful to test for error cases as well, such as when the config is not available.
Here's an example of how you could add more context and test cases:
// Test the ConstructorModuleConfig // This config is expected to inject values into the constructor of a module describe('ConstructorModuleConfig', () => { it('should return the correct config values', async () => { // Existing test case }); it('should handle missing config gracefully', async () => { // Test case for when config is not available }); it('should update config values correctly', async () => { // Test case for updating config values }); });core/metadata/test/ModuleGraph.test.ts (1)
15-28: LGTM! Consider enhancing test coverage.The new test case effectively verifies the sorting functionality of
ModuleGraphfor constructor-extended classes. The structure and implementation of the test are sound.To further improve the test coverage and clarity, consider the following suggestions:
- Add a brief comment explaining the expected order and why it's correct for constructor-extended classes.
- Consider adding an assertion to verify the initial order of classes before sorting, to ensure the test is robust against changes in the loading order.
- If possible, add a test case with a more complex inheritance structure to ensure the sorting algorithm handles various scenarios correctly.
Here's an example of how you might implement these suggestions:
it('should sort constructor extends class success', () => { const modulePath = path.join(__dirname, './fixtures/modules/extends-constructor-module'); const loader = new TestLoader(modulePath); const clazzList = loader.load(); // Assert initial order to ensure test robustness const initialOrder = clazzList.map(t => t.name); console.log('Initial order:', initialOrder); const graph = new ModuleGraph(clazzList, modulePath, 'foo'); graph.sort(); // Expected order: base classes first, then derived classes const expectedOrder = [ 'Logger', 'Bar', 'ConstructorBase', 'FooConstructor', 'FooConstructorLogger', ]; assert.deepStrictEqual(graph.clazzList.map(t => t.name), expectedOrder, 'Classes should be sorted with base classes first, followed by derived classes'); });This enhancement provides more context and ensures the test is more robust against potential changes in the loading order of classes.
core/core-decorator/src/util/MetadataUtil.ts (1)
34-46: LGTM! New method looks good, but consider addressing static analysis warnings.The new
initArrayMetaDatamethod is well-implemented and properly documented. It correctly initializes array metadata without inheriting from parent metadata, which aligns with the intended behavior.Consider addressing the static analysis warnings about using
thisin static methods. While this is a common practice in TypeScript and works as intended, you might want to use the class nameMetadataUtilinstead ofthisfor clarity:static initArrayMetaData<T>(metadataKey: MetaDataKey, clazz: EggProtoImplClass, defaultValue: Array<T>): Array<T> { const ownMetaData: Array<T> | undefined = MetadataUtil.getOwnMetaData(metadataKey, clazz); if (!ownMetaData) { MetadataUtil.defineMetaData(metadataKey, defaultValue, clazz); } return MetadataUtil.getOwnMetaData<Array<T>>(metadataKey, clazz)!; }This change would resolve the static analysis warnings while maintaining the same functionality.
🧰 Tools
🪛 Biome
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 45-45: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
core/core-decorator/test/decorators.test.ts (3)
71-77: LGTM! Consider adding a descriptive test name.The new test case effectively verifies the functionality of constructor injection using
PrototypeUtil.getInjectConstructors. It checks both the reference indices and names for the injected dependencies, which is crucial for ensuring correct dependency injection in constructors.Consider updating the test description to be more specific:
-it('constructor should work', () => { +it('should correctly retrieve constructor injection information', () => {This change would make the test's purpose clearer at a glance.
101-112: LGTM! Consider enhancing test clarity.The new test case effectively verifies the functionality of qualifiers in constructors using
QualifierUtil.getProperQualifiers. It checks bothLoadUnitNameandInitTypequalifiers for two different properties, ensuring consistency in qualifier attributes.To improve clarity, consider:
- Updating the test description to be more specific:
-it('constructor should work', () => { +it('should correctly retrieve qualifiers for constructor properties', () => {
- Adding a comment to explain why you're testing two different properties:
// Test qualifiers for two different properties to ensure consistency const constructorQualifiers = QualifierUtil.getProperQualifiers(ConstructorObject, 'xCache'); const constructorQualifiers2 = QualifierUtil.getProperQualifiers(ConstructorObject, 'cache');These changes would make the test's purpose and structure clearer.
Line range hint
18-112: Overall, excellent additions to the test suite!The new test cases for constructor injection and qualifiers in constructors are well-implemented and effectively cover the new functionality. They maintain consistency with the existing test patterns and provide good coverage for the new features.
If you'd like any assistance in implementing the suggested improvements or adding any additional test cases to further strengthen the test coverage, please let me know. I'd be happy to help!
core/runtime/test/EggObject.test.ts (3)
119-126: Clarify the purpose of mocking ContextHandler twice.The ContextHandler is mocked twice with different behaviors. It might be helpful to add comments explaining the purpose of each mock:
// Mock ContextHandler to return undefined before creating the singleton mm(ContextHandler, 'getContext', () => { return; }); // ... (code to create load unit instance) // Mock ContextHandler to return the test context when creating the singleton mm(ContextHandler, 'getContext', () => { return ctx; });This will make the test's intention clearer and easier to understand.
129-130: Consider adding more specific assertions.The current assertion only checks if the returned message is 'hello from depth2'. It might be beneficial to add more specific assertions to ensure that the context is properly injected into the singleton's constructor. For example:
const msg = await bar.hello(); assert.strictEqual(msg, 'hello from depth2', 'Unexpected message from singleton'); assert(bar.context === ctx, 'Context was not properly injected into singleton constructor');This will provide more confidence that the constructor injection is working as expected.
131-132: Ensure proper cleanup in case of test failure.The current cleanup process is good, but it might not run if an assertion fails. Consider using a try-finally block to ensure cleanup always occurs:
try { // ... (test code) assert(msg === 'hello from depth2'); } finally { await TestUtil.destroyLoadUnitInstance(instance); await ctx.destroy({}); }This ensures that resources are always cleaned up, even if the test fails.
core/metadata/test/LoadUnit.test.ts (2)
10-30: LGTM! Well-structured test case with clear assertions.The test case is well-organized and follows good practices:
- It uses async/await syntax correctly.
- It sets up the test environment with TestLoader and LoadUnit.
- It makes specific assertions about the structure of the EggPrototypes.
- It cleans up resources by destroying the LoadUnit after the test.
Consider adding comments to explain the significance of not inheriting from the parent class constructor. This would improve the test's readability and maintainability.
To enhance the test coverage, consider adding the following scenarios:
- A negative test case where inheritance does occur unexpectedly.
- Edge cases with different numbers of constructor arguments.
- A test case with multiple levels of inheritance to ensure the behavior is consistent.
Example:
it('should throw an error when inheritance occurs unexpectedly', async () => { // Setup code similar to the existing test // ... await assert.rejects( async () => { // Call a method that should throw an error if inheritance occurs }, { name: 'AssertionError', message: /expected error message/ } ); });
10-30: Consider grouping related test suites for better organization.The new test suite for "inject with constructor" is well-placed as a separate suite. However, for better organization and readability, consider grouping it with other related test suites if they exist.
If there are other test suites related to constructor injection or similar functionality, consider placing this new suite adjacent to them. This grouping can improve the overall structure of the test file and make it easier for developers to find and maintain related tests.
For example:
describe('Constructor Injection', () => { describe('inject with constructor', () => { // ... (your new test case) }); describe('other constructor injection scenarios', () => { // ... (other related test cases) }); });This structure helps in maintaining a logical flow in the test file and makes it easier to add more related tests in the future.
core/common-util/test/ObjectUtil.test.ts (1)
41-41: Remove or clarify the incomplete commentOn line 41, there is a comment
// fpp...which appears to be incomplete or a typo. Please clarify its purpose or remove it to improve code readability.core/core-decorator/src/decorator/Inject.ts (2)
30-43: Address the TODO inconstructorInjectto improveobjNameresolutionThere's a TODO comment indicating that retrieving
objNamefromdesign:typeis pending implementation in theconstructorInjectfunction. Completing this will ensure that the correct object name is injected whenparamis not provided, enhancing reliability.Would you like assistance in implementing this functionality? I can help provide a code solution or open a GitHub issue to track this task.
Line range hint
21-24: Refactor duplicated logic when creatinginjectObjectThe code for creating
injectObjectin bothpropertyInject(lines 21-24) andconstructorInject(lines 35-39) shares similar structure. Refactoring this into a shared helper function would reduce duplication and improve maintainability.Apply this diff to extract a helper function:
+ function createInjectObject(refName: PropertyKey | string | undefined, objName: PropertyKey | string | undefined) { + return { + refName, + objName: objName || refName, + }; + } function propertyInject(target: any, propertyKey: PropertyKey) { let objName: PropertyKey | undefined; // existing logic to determine objName // ... - const injectObject: InjectObjectInfo = { - refName: propertyKey, - objName: objName || propertyKey, - }; + const injectObject = createInjectObject(propertyKey, objName); PrototypeUtil.setInjectType(target.constructor, InjectType.PROPERTY); PrototypeUtil.addInjectObject(target.constructor as EggProtoImplClass, injectObject); } function constructorInject(target: any, parameterIndex: number) { // existing logic to determine argName and objName // ... - const injectObject: InjectConstructorInfo = { - refIndex: parameterIndex, - refName: argName, - objName: objName || argName, - }; + const injectObject = { + refIndex: parameterIndex, + ...createInjectObject(argName, objName), + }; PrototypeUtil.setInjectType(target, InjectType.CONSTRUCTOR); PrototypeUtil.addInjectConstructor(target as EggProtoImplClass, injectObject); }Also applies to: 35-39
core/types/metadata/model/EggPrototype.ts (1)
34-55: Improve property comments inInjectConstructorProtofor clarityThe comments for the properties in the
InjectConstructorProtointerface could be clearer and more consistent:
refIndex: The comment currently reads "inject args index". Consider rephrasing to "Index of the constructor argument to be injected."refName: The comment is "property name obj inject to". Adjust to "Name of the property where the object is injected."objName: Currently "obj's name will be injected". Change to "Name of the object to be injected."qualifiers: Improve the comment to "Qualifiers used for injection."proto: Update the comment to "The prototype of the object to be injected."core/core-decorator/src/util/PrototypeUtil.ts (3)
154-154: Improve error message clarity in 'setInjectType' methodThe error message in the
throw new Errorstatement has grammatical errors and could be clearer.Consider updating it as follows:
- throw new Error(`class ${clazz.name} already use inject type ${injectType} can not use ${type}`); + throw new Error(`Class ${clazz.name} already uses inject type ${injectType}; cannot use ${type}.`);
189-195: Remove commented-out code to enhance maintainabilityThe code segment from lines 189 to 195 is commented out. Removing unused commented code can improve code readability and maintainability.
Consider deleting the following code:
- // static getInjectConstructors(clazz: EggProtoImplClass): Array<InjectConstructorInfo> { - // return MetadataUtil.getArrayMetaData<InjectConstructorInfo>(PrototypeUtil.INJECT_CONSTRUCTOR_NAME_SET, clazz) - // .sort((a, b) => { - // return a.refIndex - b.refIndex; - // }); - // }
149-187: Add JSDoc comments to newly introduced methodsSeveral newly added methods lack JSDoc comments. Adding documentation will improve understandability and maintainability.
Consider adding JSDoc comments to the following methods:
setInjectTypeaddInjectObjectaddInjectConstructorgetInjectTypegetInjectObjects
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (45)
- README.md (1 hunks)
- core/common-util/src/ObjectUtils.ts (2 hunks)
- core/common-util/test/ObjectUtil.test.ts (2 hunks)
- core/core-decorator/src/decorator/ConfigSource.ts (1 hunks)
- core/core-decorator/src/decorator/EggQualifier.ts (1 hunks)
- core/core-decorator/src/decorator/InitTypeQualifier.ts (1 hunks)
- core/core-decorator/src/decorator/Inject.ts (2 hunks)
- core/core-decorator/src/decorator/ModuleQualifier.ts (1 hunks)
- core/core-decorator/src/util/MetadataUtil.ts (1 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (9 hunks)
- core/core-decorator/src/util/QualifierUtil.ts (2 hunks)
- core/core-decorator/test/decorators.test.ts (3 hunks)
- core/core-decorator/test/fixtures/decators/ConstructorObject.ts (1 hunks)
- core/dal-decorator/src/decorator/DataSourceQualifier.ts (1 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (5 hunks)
- core/metadata/src/impl/EggPrototypeImpl.ts (5 hunks)
- core/metadata/test/LoadUnit.test.ts (1 hunks)
- core/metadata/test/ModuleGraph.test.ts (2 hunks)
- core/metadata/test/fixtures/modules/app-graph-modules/root/RootConstructor.ts (1 hunks)
- core/metadata/test/fixtures/modules/extends-constructor-module/Base.ts (1 hunks)
- core/metadata/test/fixtures/modules/extends-constructor-module/package.json (1 hunks)
- core/runtime/src/impl/EggObjectImpl.ts (3 hunks)
- core/runtime/src/impl/EggObjectUtil.ts (1 hunks)
- core/runtime/test/EggObject.test.ts (2 hunks)
- core/runtime/test/fixtures/modules/inject-constructor-context-to-singleton/object.ts (1 hunks)
- core/runtime/test/fixtures/modules/inject-constructor-context-to-singleton/package.json (1 hunks)
- core/types/core-decorator/enum/InjectType.ts (1 hunks)
- core/types/core-decorator/enum/Qualifier.ts (1 hunks)
- core/types/core-decorator/index.ts (1 hunks)
- core/types/core-decorator/model/InjectConstructorInfo.ts (1 hunks)
- core/types/metadata/model/EggPrototype.ts (5 hunks)
- plugin/tegg/test/ConstructorModuleConfig.test.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/controller/app.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/application.unittest.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/context.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/router.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/app/typings/index.d.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/config/config.default.js (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/config/plugin.js (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/foo.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/module.unittest.yml (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/module.yml (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/constructor-module-config/package.json (1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
core/common-util/src/ObjectUtils.ts
[failure] 45-45: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings starting with 'constructor(' and with many repetitions of 'constructor(('.
This regular expression that depends on library input may run slow on strings starting with 'constructor(' and with many repetitions of 'constructor(('.
[failure] 50-50: Polynomial regular expression used on uncontrolled data
This regular expression that depends on library input may run slow on strings with many repetitions of 'a'.
This regular expression that depends on library input may run slow on strings with many repetitions of ' '.
This regular expression that depends on library input may run slow on strings with many repetitions of 'a'.
This regular expression that depends on library input may run slow on strings with many repetitions of ' '.
🪛 Biome
core/common-util/test/ObjectUtil.test.ts
[error] 10-16: Do not export from a test file.
(lint/suspicious/noExportsInTest)
[error] 17-23: Do not export from a test file.
(lint/suspicious/noExportsInTest)
core/core-decorator/src/util/MetadataUtil.ts
[error] 41-41: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 43-43: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
[error] 45-45: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
core/runtime/src/impl/EggObjectUtil.ts
[error] 40-40: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 45-45: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 50-50: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 59-59: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 67-67: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 84-84: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 89-89: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 102-102: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 108-108: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 114-114: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 124-124: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 155-155: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 161-161: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
plugin/tegg/test/fixtures/apps/constructor-module-config/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/constructor-module-config/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 (67)
core/metadata/test/fixtures/modules/extends-constructor-module/package.json (1)
1-6: LGTM! Well-structured package.json for test module.The
package.jsonfile is correctly structured and contains the necessary information for the "multi-module-service" test module. It properly defines the module name and its dependency on the "extendsModule" using theeggModuleproperty. This setup is suitable for testing the module extension functionality in the Egg.js framework.plugin/tegg/test/fixtures/apps/constructor-module-config/app/router.ts (1)
1-5: LGTM! The router configuration looks correct.The implementation follows Egg.js conventions and correctly sets up a GET route for '/config' using the Application instance.
plugin/tegg/test/fixtures/apps/constructor-module-config/config/plugin.js (3)
3-6: LGTM: Tracer configuration looks goodThe configuration for the 'egg-tracer' package is correctly set up and enabled. This will be useful for tracing and debugging purposes in the application.
8-11: LGTM: teggConfig configuration is properly set upThe configuration for the '@eggjs/tegg-config' package is correctly defined and enabled. This will provide additional configuration capabilities for the Egg.js framework in the application.
1-13: Overall, the plugin configuration looks good and aligns with PR objectivesThis configuration file sets up the necessary plugins for testing the new constructor injection feature. The
teggConfigplugin is particularly relevant as it likely supports the new dependency injection capabilities. Thetracerplugin will be useful for debugging during development and testing.A few minor improvements were suggested:
- Removing the redundant 'use strict' directive.
- Adding a comment to explain the disabled watcher.
These configurations provide a solid foundation for testing the new "inject in constructor" feature in a controlled environment.
🧰 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/constructor-module-config/app/extend/application.unittest.ts (3)
1-1: LGTM: Import statement is correct.The import of
MockApplicationfrom 'egg-mock' is properly typed and used in the file.
3-3: LGTM: Default export object is well-structured.The default export object is correctly defined and contains the
mockUsermethod as expected.Also applies to: 11-11
1-11: Overall assessment: Well-implemented test fixture.This file effectively extends the
MockApplicationfor testing purposes:
- The import and export statements are correct.
- The
mockUsermethod is well-typed and implements a basic mock user context.- The code follows TypeScript best practices.
The file serves its purpose well in the context of testing fixtures. Consider the earlier suggestion to enhance the mock user object for more comprehensive testing scenarios if needed.
core/metadata/test/fixtures/modules/app-graph-modules/root/RootConstructor.ts (2)
1-2: LGTM: Imports are correctly structured.The import statements are well-organized, importing necessary decorators from the core library and a related type from a sibling module. This structure promotes modularity and clear dependencies.
1-8: Overall, the implementation looks good and aligns with the PR objectives.This new file successfully introduces a class that supports constructor injection, which is the main goal of the PR. The code is concise, uses TypeScript features effectively, and follows good practices for dependency injection and immutability. The minor suggestions for documentation and type annotation, if implemented, would further enhance the code quality.
plugin/tegg/test/fixtures/apps/constructor-module-config/app/extend/context.ts (3)
1-1: LGTM: Good use of Symbol for private state managementThe use of a Symbol for
COUNTERis a good practice for maintaining private state. The naming convention 'Context#counter' clearly indicates its intended use.
1-14: Overall review: Good implementation with minor suggestions for improvementThis file, being part of a test fixture, implements a
counterand ausergetter. Thecounterimplementation is well done, with a minor suggestion for optimization. Theusergetter might benefit from clarification on its intended use.Given that this is a test fixture, some simplifications are expected. However, ensuring clarity and following best practices even in test code is beneficial for maintainability and preventing confusion.
11-13: Clarify the purpose of theusergetterThe
usergetter currently returns a new empty object on each call. This implementation might lead to unexpected behavior if callers expect to modify the returned object and have those modifications persist.Consider the following:
- Is this a placeholder for future implementation?
- Should it return a shared object instead of a new one each time?
- Is there any specific reason for returning an empty object?
Please clarify the intended behavior. If it's not a placeholder, consider one of these alternatives:
private _user = {}; get user() { return this._user; }or
get user() { if (!this._user) { this._user = {}; } return this._user; }These alternatives would provide a consistent object reference across multiple calls.
To verify the usage of this getter in the codebase, you can run the following script:
This will help understand how the
usergetter is being used and whether the current implementation meets those use cases.✅ Verification successful
The
usergetter is not used in the codebaseVerification shows that the
usergetter is not referenced anywhere. If this getter is intended for future use, consider documenting its purpose. Otherwise, consider removing it to simplify the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usage of the user getter in the codebase rg --type typescript -A 5 'context\.user'Length of output: 78
Script:
#!/bin/bash # Description: Search for usage of the user getter in the codebase rg --type ts -A 5 'context\.user'Length of output: 35
plugin/tegg/test/fixtures/apps/constructor-module-config/app/controller/app.ts (2)
1-1: LGTM: Import statement is correct.The import of
Controllerfrom 'egg' is appropriate for extending theControllerclass in this context.
3-10: 🛠️ Refactor suggestionConsider renaming the
baseDirmethod for clarity.The current method name
baseDirdoesn't clearly reflect its functionality of retrieving configuration values. Consider renaming it to something more descriptive, such asgetConfigValuesorretrieveConstructorConfig.Add error handling and property validation.
The method assumes that
this.app.module.constructorSimple.fooand its properties always exist. Consider adding error handling and property validation to make the code more robust.Here's a suggested implementation:
async getConfigValues() { try { const config = this.app.module.constructorSimple?.foo; if (!config || typeof config.foo === 'undefined' || typeof config.bar === 'undefined') { throw new Error('Invalid configuration'); } this.ctx.body = { foo: config.foo, bar: config.bar, }; } catch (error) { this.ctx.status = 500; this.ctx.body = { error: 'Failed to retrieve configuration' }; } }Clarify the purpose of this controller in the context of constructor injection.
While this controller demonstrates accessing values from
this.app.module.constructorSimple.foo, it's not immediately clear how this relates to the PR objective of supporting injection in constructors. Could you provide more context on how this controller is used to test or demonstrate the new injection capabilities?To better understand the context, let's check for related test files:
plugin/tegg/test/fixtures/apps/constructor-module-config/app/typings/index.d.ts (2)
5-11: LGTM: Correct module declaration.The module declaration for 'egg' is correctly implemented, allowing for the extension of the existing module.
1-11: Overall assessment: Good implementation with room for minor improvements.This file successfully extends the EggModule interface to support constructor injection, which aligns with the PR objective of "feat: support inject in constructor". The implementation is correct and type-safe.
To further improve the code:
- Remove the unused import of
Bar.- Consider renaming the
constructorSimpleproperty to something more descriptive.- Add JSDoc comments to document the new property.
These changes will enhance code clarity and maintainability while fully achieving the PR's objectives.
core/types/core-decorator/model/InjectConstructorInfo.ts (1)
1-16: Well-structured interface with clear documentation.The
InjectConstructorInfointerface is well-defined and properly documented. It provides a clear structure for managing constructor injection information, which aligns well with the PR objective of supporting injection in constructors. The use of TypeScript and JSDoc comments enhances code readability and maintainability.plugin/tegg/test/fixtures/apps/constructor-module-config/app.ts (3)
1-5: LGTM: Class structure and import look good.The import of
Applicationfrom 'egg' and the class structure ofAppBootare well-defined and follow common practices for Egg.js applications. The class name suggests its purpose in the application boot process.
6-8: LGTM: Constructor implements dependency injection correctly.The constructor follows the dependency injection pattern by accepting an
Applicationinstance and storing it as a class property. This approach facilitates easier testing and promotes loose coupling between theAppBootclass and the Egg.jsApplication.
10-14: 🛠️ Refactor suggestionConsider improving type safety and documenting the runtime configuration modification.
The
configWillLoadmethod modifies the application configuration at runtime, which can lead to unexpected behavior. Consider the following suggestions:
- Use more specific types instead of
Record<string, any>to improve type safety.- Document the purpose of this runtime configuration modification and its potential impacts.
- Consider using a more robust configuration management system that allows for such overrides without direct mutation.
Here's a suggested refactor to improve type safety:
interface OverwriteConfig { features: { dynamic: { bar: string; }; }; } configWillLoad() { const overwriteConfig = this.app.moduleConfigs?.overwrite?.config as OverwriteConfig; if (overwriteConfig) { overwriteConfig.features.dynamic.bar = 'overwrite foo'; } }To ensure this configuration override doesn't conflict with other parts of the application, let's search for other usages of this configuration:
✅ Verification successful
Configuration modification verified with limited usages.
The
features.dynamic.barconfiguration is only used in the intended locations, minimizing the risk of unexpected behavior from runtime modifications. Proceed with the suggested improvements to type safety and documentation.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other usages of the 'bar' configuration rg --type typescript "features\.dynamic\.bar"Length of output: 82
Script:
#!/bin/bash # Search for other usages of the 'bar' configuration in .ts and .tsx files fd --extension ts --extension tsx --exec rg "features\.dynamic\.bar" {}Length of output: 347
plugin/tegg/test/fixtures/apps/constructor-module-config/config/config.default.js (4)
5-6: Function structure looks good.The exported function follows the standard pattern for Egg.js configuration files. It correctly takes
appInfoas a parameter, allowing for dynamic configuration based on application information.
8-12: Custom logger configuration looks good.The custom logger configuration is well-structured and uses the
appInfo.rootto set the log file path, which is a good practice for maintaining consistent file locations across different environments.
13-17: Good security practice for CSRF protection.Setting
ignoreJSON: falsefor CSRF protection is a good security practice as it ensures that JSON requests are not exempt from CSRF checks.
19-20: Return statement is correct.The function correctly returns the
configobject, following the expected pattern for Egg.js configuration files.plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/foo.ts (2)
1-5: LGTM: Imports and decorator usage are correct.The imports from '@eggjs/tegg' and the usage of the
@SingletonProtodecorator with public access level are appropriate for the implemented functionality.
6-8: LGTM: Class declaration and properties are well-defined.The
Fooclass is correctly exported, and the use of readonly properties forfooandbarpromotes immutability, which is a good practice.core/core-decorator/src/decorator/ModuleQualifier.ts (3)
6-6: LGTM: Function signature updated to support constructor injection.The updated function signature now includes an optional
parameterIndexparameter and makespropertyKeyoptional. This change allows theModuleQualifierdecorator to be used for both property injection and constructor parameter injection, aligning with the PR objective of supporting injection in constructors while maintaining backward compatibility.
Line range hint
1-9: Overall assessment: Changes successfully implement constructor injection support.The modifications to the
ModuleQualifierfunction effectively implement support for constructor injection while maintaining compatibility with existing property injection. The changes are concise, focused, and align well with the PR objectives. Good job on keeping the implementation clean and backward-compatible.
7-7: Implementation updated to support both property and constructor injection.The change to use
QualifierUtil.addInjectQualifierinstead ofaddProperQualifierappears to be a more flexible approach that can handle both property and constructor parameter injection. This is consistent with the function signature update and aligns with the PR objective.To ensure the correctness of this change, please verify the implementation of
QualifierUtil.addInjectQualifier. Run the following script to check its implementation:✅ Verification successful
Verified the implementation of
QualifierUtil.addInjectQualifier.The
addInjectQualifiermethod correctly handles both property and constructor parameter injection by differentiating based on the presence ofparameterIndex. Its implementation aligns with the intended functionality, ensuring flexibility and consistency.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of QualifierUtil.addInjectQualifier # Test: Search for the addInjectQualifier method in QualifierUtil ast-grep --lang typescript --pattern $'class QualifierUtil { $$$ static addInjectQualifier($_) { $$$ } $$$ }'Length of output: 122
Script:
#!/bin/bash # Description: Search for any occurrence of addInjectQualifier in TypeScript files rg 'addInjectQualifier' --glob '*.ts'Length of output: 1148
Script:
#!/bin/bash # Description: Verify the implementation of QualifierUtil.addInjectQualifier with a flexible pattern ast-grep --lang typescript --pattern $'class QualifierUtil { $$$ static addInjectQualifier($_, $_, $_, $_, $_) { $$$ } $$$ }'Length of output: 6707
Script:
# Script 1: Flexible ast-grep pattern to find addInjectQualifier method ast-grep --lang typescript --pattern $'class QualifierUtil { $$$ static addInjectQualifier($_) { $$$ } $$$ }'Length of output: 122
Script:
# Script 2: Extract the implementation of addInjectQualifier using rg rg -A 5 'static addInjectQualifier' --glob '*.ts' core/core-decorator/src/util/QualifierUtil.tsLength of output: 528
core/core-decorator/src/decorator/ConfigSource.ts (1)
6-7: LGTM! Changes align with PR objective.The modifications to the
ConfigSourceQualifierfunction successfully implement support for constructor injection. The addition of theparameterIndexparameter and the use ofQualifierUtil.addInjectQualifierallow this decorator to be used on constructor parameters, which is consistent with the PR objective of supporting injection in constructors.core/core-decorator/src/decorator/InitTypeQualifier.ts (1)
6-7: Verify implementation and usage of the new feature.To ensure the changes are correctly implemented and used:
- Let's verify the implementation of
QualifierUtil.addInjectQualifier.- Check for usage of
InitTypeQualifierdecorator on constructor parameters.- Look for documentation updates related to this new feature.
Run the following script to perform these checks:
These checks will help ensure that the new feature is properly implemented, used, and documented across the codebase.
core/types/core-decorator/enum/Qualifier.ts (1)
12-13: LGTM! Verify usage in related files.The addition of
CONSTRUCTOR_QUALIFIER_META_DATAis consistent with existing qualifier metadata constants and follows the established naming convention. This suggests an expansion of the qualifier system to include constructor-level qualifiers, which aligns with the PR objective of supporting injection in constructors.To ensure this change is properly integrated, please verify its usage in related files:
✅ Verification successful
Verified! The
CONSTRUCTOR_QUALIFIER_META_DATAconstant is successfully added and utilized withinQualifierUtil.ts, aligning with the PR's objective of expanding the qualifier system.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of the new CONSTRUCTOR_QUALIFIER_META_DATA constant # Search for imports or usage of CONSTRUCTOR_QUALIFIER_META_DATA rg --type typescript "CONSTRUCTOR_QUALIFIER_META_DATA" # Search for potential decorator implementations that might use this constant ast-grep --lang typescript --pattern $'@$_($$$) class $_ { constructor($$$) { $$$ } }'Length of output: 177
Script:
#!/bin/bash # Description: Check for usage of the new CONSTRUCTOR_QUALIFIER_META_DATA constant # Search for imports or usage of CONSTRUCTOR_QUALIFIER_META_DATA in .ts and .tsx files rg "CONSTRUCTOR_QUALIFIER_META_DATA" --glob "*.ts" --glob "*.tsx" # Search for potential decorator implementations that might use this constant ast-grep --lang typescript --pattern $'@$_($$$) class $_ { constructor($$$) { $$$ } }'Length of output: 672
core/types/core-decorator/index.ts (1)
5-5: LGTM! Verify usage of newInjectTypeenum.The addition of
InjectTypeexport is consistent with the file structure and aligns with the PR objective of supporting injection in constructors.To ensure proper integration, let's verify the usage of
InjectType:✅ Verification successful
InjectType Export Verified
The
InjectTypeenum is correctly exported and utilized across multiple files, ensuring consistent injection functionalities.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of InjectType in the codebase # Test: Search for InjectType usage rg --type typescript "InjectType" -g '!core/types/core-decorator/index.ts'Length of output: 109
Script:
#!/bin/bash # Description: Check usage of InjectType in the TypeScript codebase # Test: Search for InjectType usage using the correct type identifier rg --type ts "InjectType" -g '!core/types/core-decorator/index.ts'Length of output: 2535
core/metadata/test/fixtures/modules/extends-constructor-module/Base.ts (7)
1-1: LGTM: Imports are correctly specified and used.The import statement is concise and imports only the necessary decorators from '@eggjs/core-decorator'.
3-5: LGTM: Logger class is correctly defined with @ContextProto() decorator.The Logger class is properly decorated with @ContextProto(). Note that the class body is empty, which is unusual but may be intentional for this test fixture.
7-9: LGTM: Bar class is correctly defined with @ContextProto() decorator.The Bar class is properly decorated with @ContextProto(). Similar to the Logger class, the body is empty, which may be intentional for this test fixture.
11-15: LGTM: ConstructorBase class demonstrates correct use of decorators and dependency injection.The ConstructorBase class is well-structured:
- It's properly decorated with @ContextProto().
- The constructor correctly uses @Inject() for dependency injection of the Logger instance.
- The injected Logger is appropriately typed and marked as readonly.
This class serves as a good base for demonstrating constructor injection in the test fixture.
24-29: LGTM: FooConstructorLogger class demonstrates correct inheritance and multiple constructor injections.The FooConstructorLogger class is well-implemented:
- It's properly decorated with @ContextProto().
- It correctly extends ConstructorBase.
- The constructor uses @Inject() for dependency injection of both Bar and Logger instances.
- The super() call correctly passes the injected logger to the superclass constructor.
This class effectively demonstrates how to handle multiple injections and proper superclass constructor calling in the context of this test fixture.
1-29: Overall: Well-structured test fixture for constructor injection scenarios.This file effectively demonstrates various use cases for constructor injection and class inheritance within the context of the @eggjs/core-decorator framework. Key points:
- Proper use of @ContextProto() and @Inject() decorators throughout.
- Clear demonstration of class inheritance with ConstructorBase as the parent class.
- Multiple scenarios of constructor injection, including single and multiple parameter injections.
- Clean and easy-to-understand structure that serves well as a test fixture.
The only point to double-check is the use of
consolein the FooConstructor class's super() call, which might be intentional for testing but differs from the expected Logger type.Great job on creating a comprehensive test fixture for these scenarios!
17-22: LGTM with a note: FooConstructor class demonstrates inheritance and constructor injection.The FooConstructor class is well-structured:
- It's properly decorated with @ContextProto().
- It correctly extends ConstructorBase.
- The constructor uses @Inject() for dependency injection of the Bar instance.
However, note that the super() call uses console instead of a Logger instance. While this might be intentional for testing purposes, it's worth verifying if this is the desired behavior, as it doesn't match the type expected by the ConstructorBase.
✅ Verification successful
LGTM with confirmation: The use of
super(console)inFooConstructoris unique within the codebase, indicating it's an intentional design choice.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify if console is used elsewhere in a similar context rg --type typescript 'super\(console\)' -g '!**/Base.ts'Length of output: 93
Script:
#!/bin/bash # Verify if console is used elsewhere in a similar context rg --type ts 'super\(console\)' -g '!**/Base.ts'Length of output: 50
core/core-decorator/test/fixtures/decators/ConstructorObject.ts (2)
1-6: LGTM: Imports are well-structured and necessary.The imports are correctly structured and all seem to be used in the implementation. They suggest a well-organized project structure.
8-9: LGTM: Class declaration is well-structured.The
ConstructorObjectclass is correctly declared and exported. The use of the@SingletonProto()decorator aligns with the PR objective of supporting dependency injection in constructors.plugin/tegg/test/ConstructorModuleConfig.test.ts (4)
1-7: LGTM: Imports and test setup look good.The import statements are appropriate for the test file, and the describe block is correctly set up with the proper file path. The fixture directory is also well-defined.
9-15: LGTM: Proper cleanup in after and afterEach hooks.The after hook correctly closes the app, which is good for cleaning up resources. The afterEach hook restores the mock state, ensuring proper test isolation.
1-40: 🛠️ Refactor suggestionEnhance test coverage and provide context on PR objective.
While the test file correctly sets up the environment and includes a basic test case, there are opportunities for improvement:
- Add more test cases to cover different scenarios and edge cases of ConstructorModuleConfig.
- Include comments or documentation explaining what ConstructorModuleConfig is and how it relates to the PR objective of supporting inject in constructor.
- Consider testing the injection behavior directly, not just the config values.
Here's a suggestion for expanding the test suite:
import { Container } from 'your-di-container'; // Import your DI container describe('ConstructorModuleConfig', () => { // Existing test case it('should inject config values into constructor', async () => { const container = new Container(); const module = container.resolve('YourModule'); assert(module.foo === 'bar'); assert(module.bar === 'foo'); }); it('should throw error when required config is missing', async () => { // Test case for error handling }); });To better understand the context, let's search for ConstructorModuleConfig usage:
#!/bin/bash # Description: Search for ConstructorModuleConfig usage # Test: Look for ConstructorModuleConfig in TypeScript files rg --type typescript 'ConstructorModuleConfig' -C 5
17-27: LGTM with a minor query: Test environment setup looks good.The before hook correctly sets up the test environment by mocking necessary variables and creating the mock app. However, could you please clarify why process.cwd() is being mocked? It might be helpful to add a comment explaining the reason for this mock.
To understand the necessity of mocking process.cwd(), let's check if it's used in the application setup:
core/runtime/test/EggObject.test.ts (1)
117-134: Overall, good test coverage for constructor injection.The new test suite effectively covers the functionality of injecting context into a singleton via its constructor, which aligns well with the PR objective. The test case is structured logically and tests the core functionality. With the suggested improvements, this test will provide robust coverage for the new feature.
Good job on adding this test coverage!
core/metadata/test/LoadUnit.test.ts (1)
10-30: Overall, good addition to the test suite with room for minor improvements.The new test suite for "inject with constructor" is a valuable addition to the LoadUnit tests. It effectively verifies that parent class constructors are not inherited, which seems to be an important aspect of the implementation.
Summary of suggestions:
- Add comments explaining the significance of not inheriting from the parent class constructor.
- Enhance test coverage with additional scenarios and edge cases.
- Strengthen assertions to provide more comprehensive verification.
- Consider grouping related test suites for better organization.
These improvements will further enhance the robustness and maintainability of the test suite. Great job on implementing this new feature and its corresponding test!
README.md (1)
501-519: LGTM! Clear example of constructor injection.The added example effectively demonstrates the use of constructor injection as an alternative to property injection. It's well-formatted and consistent with the rest of the documentation.
A few points to note:
- The comment correctly emphasizes that property and constructor injection should not be mixed.
- The example clearly shows how to use the
@Inject()decorator in the constructor.- The
readonlymodifier on the injected property is a good practice for immutability.This addition enhances the documentation by providing users with multiple options for dependency injection, allowing for more flexible code organization.
core/runtime/test/fixtures/modules/inject-constructor-context-to-singleton/object.ts (3)
4-11: Well-defined context-scoped classContextFooDepth2.The
ContextFooDepth2class is correctly annotated with@ContextProtoand has a public access level. Thehellomethod is properly implemented and returns the expected string.
25-35: Proper singleton classSingletonBarConstructorDepth2with constructor injection.The
SingletonBarConstructorDepth2class is correctly defined as a singleton with a public access level. It properly injectsSingletonConstructorBarDepth3via its constructor, and thehellomethod delegates correctly.
38-48: Well-structured context-scoped classContextConstructorFoo.The
ContextConstructorFooclass is appropriately annotated with@ContextProtoand injectsSingletonBarConstructorDepth2in its constructor. Thehellomethod is correctly implemented, maintaining proper use of dependency injection.core/core-decorator/src/decorator/Inject.ts (1)
45-51: Logic for injection type determination is correctly implementedThe returned decorator function correctly determines whether to perform property injection or constructor injection based on the presence of
parameterIndex. The use oftypeof parameterIndex === 'undefined'ensures that parameter index0is handled appropriately.core/metadata/src/impl/EggPrototypeImpl.ts (5)
1-1: ImportingInjectTypeis appropriateThe addition of
InjectTypeto the imports ensures that the class can reference the injection types needed for the new functionality.
8-9: IncludingInjectConstructorProtoenhances injection capabilitiesBy importing
InjectConstructorProto, the class now supports constructor-based injection prototypes, expanding its flexibility.
25-26: Enhanced flexibility with updatedinjectObjectsand the newinjectTypeUpdating
injectObjectsto accept bothInjectObjectProtoandInjectConstructorProtoincreases the versatility of the injection system. The addition of theinjectTypeproperty allows for distinguishing between property and constructor injection types.
37-41: Constructor parameters updated to support new injection typesThe constructor now accepts the updated
injectObjectMapand an optionalinjectType, aligning with the new properties and ensuring proper initialization of instances with the correct injection configuration.
53-53: DefaultinginjectTypetoInjectType.PROPERTYensures backward compatibilityBy setting a default value for
injectType, existing functionality remains unaffected wheninjectTypeis not provided, preserving backward compatibility.core/types/metadata/model/EggPrototype.ts (2)
135-135: Verify implementations ofconstructEggObjectafter method signature changeThe
constructEggObjectmethod signature has been updated to accept a variable number of arguments (...args: any). This change may affect classes that implement theEggPrototypeinterface, as they need to update their method signatures to match this change.Please ensure that all implementations of
EggPrototypehave updated theconstructEggObjectmethod accordingly. Use the following script to find and inspect implementations:#!/bin/bash # Description: Find classes implementing `EggPrototype` and check `constructEggObject` methods. ast-grep --lang typescript --pattern $'class $_ implements EggPrototype { $$$ constructEggObject($_$$): $_ $$$ }'
109-109: Ensure proper handling of updatedinjectObjectsproperty typeThe
injectObjectsproperty type has been changed toArray<InjectObjectProto | InjectConstructorProto>. This change may impact code that iterates overinjectObjectsand assumes a single type. Ensure that all code handling this property correctly accounts for both types in the union to prevent runtime errors or type issues.Run the following script to find all usages of
injectObjectsand check for proper type handling:core/core-decorator/src/util/QualifierUtil.ts (1)
1-2: Imports are correctly updatedThe import statements appropriately include
ObjectUtilsandCONSTRUCTOR_QUALIFIER_META_DATA, which are used in the updated methods.core/metadata/src/impl/EggPrototypeBuilder.ts (4)
2-2: Imports updated to include injection-related types.The import statements now include
InjectType,InjectConstructor, andInjectConstructorProto, which are necessary for supporting constructor injection.Also applies to: 8-20
33-34: AddedinjectTypeproperty and updatedinjectObjectsto include constructor injection.The addition of
injectTypeand updatinginjectObjectsto includeInjectConstructorenables the builder to handle both property and constructor injections.
50-50: SettinginjectTypein the builder.The
injectTypeis set usingPrototypeUtil.getInjectType(clazz), allowing the builder to determine the injection method.
142-142: PassinginjectTypetoEggPrototypeImplconstructor.The
injectTypeis now passed toEggPrototypeImpl, enabling it to handle injection types appropriately.core/runtime/src/impl/EggObjectImpl.ts (1)
145-151: Initialization properly delegates based oninjectTypeThe
initmethod correctly delegates toinitWithInjectConstructororinitWithInjectPropertydepending on theinjectType. This effectively supports constructor-based injection as intended.
plugin/tegg/test/fixtures/apps/constructor-module-config/config/config.default.js
Show resolved
Hide resolved
plugin/tegg/test/fixtures/apps/constructor-module-config/config/config.default.js
Show resolved
Hide resolved
plugin/tegg/test/fixtures/apps/constructor-module-config/modules/module-with-config/foo.ts
Show resolved
Hide resolved
| return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { | ||
| QualifierUtil.addInjectQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, EggQualifierAttribute, eggType); |
There was a problem hiding this comment.
💡 Codebase verification
Action Required: Remaining addProperQualifier Usages Detected
- Multiple files still use
QualifierUtil.addProperQualifier, which may need to be updated toaddInjectQualifierto ensure consistency.
🔗 Analysis chain
LGTM! Enhanced flexibility for dependency injection.
The changes to the EggQualifier decorator function improve its flexibility by allowing it to be used for both property and parameter decorators. This aligns with the PR objective of supporting injection in constructors.
Key improvements:
- The inner function now accepts an optional
parameterIndex, enabling its use as a parameter decorator. - The use of
QualifierUtil.addInjectQualifierinstead ofaddProperQualifiersuggests a more specialized handling of inject qualifiers.
These changes appear to be part of a larger refactoring to support constructor injection in the Egg.js framework.
To ensure these changes are consistent with the rest of the codebase, let's verify the usage of QualifierUtil.addInjectQualifier:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the usage of QualifierUtil.addInjectQualifier
# Test: Search for other occurrences of addInjectQualifier
echo "Searching for other occurrences of addInjectQualifier:"
rg "QualifierUtil\.addInjectQualifier" --type typescript
# Test: Check if there are any remaining uses of addProperQualifier
echo "Checking for any remaining uses of addProperQualifier:"
rg "QualifierUtil\.addProperQualifier" --type typescript
# Test: Look for other decorators that might need similar updates
echo "Searching for other decorator functions that might need similar updates:"
rg "function.*\(target: any, propertyKey\?: PropertyKey(, parameterIndex\?: number)?\)" --type typescript
Length of output: 723
Script:
#!/bin/bash
# Description: Verify the usage of QualifierUtil.addInjectQualifier
# Test: Search for other occurrences of addInjectQualifier
echo "Searching for other occurrences of addInjectQualifier:"
rg "QualifierUtil\.addInjectQualifier" --type ts
# Test: Check if there are any remaining uses of addProperQualifier
echo "Checking for any remaining uses of addProperQualifier:"
rg "QualifierUtil\.addProperQualifier" --type ts
# Test: Look for other decorators that might need similar updates
echo "Searching for other decorator functions that might need similar updates:"
rg "function.*\(target: any, propertyKey\?: PropertyKey(, parameterIndex\?: number)?\)" --type ts
Length of output: 3408
| return function(target: any, propertyKey?: PropertyKey, parameterIndex?: number) { | ||
| QualifierUtil.addInjectQualifier(target as EggProtoImplClass, propertyKey, parameterIndex, ConfigSourceQualifierAttribute, moduleName); |
There was a problem hiding this comment.
💡 Codebase verification
Potential breaking changes detected related to ConfigSourceQualifier and QualifierUtil usage.
-
Existing usages of
ConfigSourceQualifier:standalone/standalone/test/fixtures/multi-modules/foo/foo.tsstandalone/standalone/test/fixtures/dependency/foo.tsstandalone/standalone/src/ConfigSourceLoadUnitHook.tsplugin/tegg/lib/ConfigSourceLoadUnitHook.tscore/core-decorator/src/util/QualifierUtil.ts
-
Potential calls to update from
addProperQualifiertoaddInjectQualifier:standalone/standalone/test/fixtures/multi-callback-instance-module/logger/DynamicLogger.tsplugin/tegg/lib/EggQualifierProtoHook.tsplugin/eventbus/lib/EventbusLoadUnitHook.tscore/core-decorator/src/util/QualifierUtil.ts
-
Documentation files that need updating:
standalone/standalone/README.md
🔗 Analysis chain
Verify impact on existing usage and documentation.
While the changes look good, it's important to ensure that this modification doesn't introduce breaking changes for existing usages of ConfigSourceQualifier. Additionally, please update any relevant documentation to reflect this new capability.
To help verify the impact, you can run the following script:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing usages of ConfigSourceQualifier and potential breaking changes
# Search for existing usages of ConfigSourceQualifier
echo "Existing usages of ConfigSourceQualifier:"
rg --type typescript "ConfigSourceQualifier\(" -C 3
# Check if there are any direct calls to QualifierUtil.addProperQualifier that might need updating
echo "Potential calls to update from addProperQualifier to addInjectQualifier:"
rg --type typescript "QualifierUtil\.addProperQualifier" -C 3
# Search for documentation files that might need updating
echo "Documentation files that might need updating:"
fd -e md | xargs rg "ConfigSourceQualifier" -l
Length of output: 615
Script:
#!/bin/bash
# Description: Check for existing usages of ConfigSourceQualifier and potential breaking changes with correct TypeScript file detection
# Search for existing usages of ConfigSourceQualifier
echo "Existing usages of ConfigSourceQualifier:"
fd -e ts -e tsx | xargs rg "ConfigSourceQualifier\(" -C 3
# Check if there are any direct calls to QualifierUtil.addProperQualifier that might need updating
echo "Potential calls to update from addProperQualifier to addInjectQualifier:"
fd -e ts -e tsx | xargs rg "QualifierUtil\.addProperQualifier" -C 3
# Search for documentation files that might need updating
echo "Documentation files that might need updating:"
fd -e md | xargs rg "ConfigSourceQualifier" -l
Length of output: 6924
| static contextEggObjectProxy(proto: EggPrototype, objName: PropertyKey): PropertyDescriptor { | ||
| const PROTO_OBJ_PROXY = Symbol(`EggPrototype#objProxy#${String(objName)}`); | ||
| if (!proto[PROTO_OBJ_PROXY]) { | ||
| proto[PROTO_OBJ_PROXY] = new Proxy({}, { | ||
| defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| Object.defineProperty(obj, property, attributes); | ||
| return true; | ||
| }, | ||
| deleteProperty(_target: {}, p: string | symbol): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| delete obj[p]; | ||
| return true; | ||
| }, | ||
| get(target: {}, p: string | symbol): any { | ||
| // make get be lazy | ||
| if (p === 'then') return; | ||
| if (Object.prototype[p]) { | ||
| return target[p]; | ||
| } | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return obj[p]; | ||
| }, | ||
| getOwnPropertyDescriptor(_target: {}, p: string | symbol): PropertyDescriptor | undefined { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return Object.getOwnPropertyDescriptor(obj, p); | ||
| }, | ||
| getPrototypeOf(): object | null { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return Object.getPrototypeOf(obj); | ||
| }, | ||
| has(_target: {}, p: string | symbol): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return p in obj; | ||
| }, | ||
| isExtensible(): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return Object.isExtensible(obj); | ||
| }, | ||
| ownKeys(): ArrayLike<string | symbol> { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| return Reflect.ownKeys(obj); | ||
| }, | ||
| preventExtensions(): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| Object.preventExtensions(obj); | ||
| return true; | ||
| }, | ||
| set(_target: {}, p: string | symbol, newValue: any): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| obj[p] = newValue; | ||
| return true; | ||
| }, | ||
| setPrototypeOf(_target: {}, v: object | null): boolean { | ||
| const eggObject = EggContainerFactory.getEggObject(proto, objName); | ||
| const obj = eggObject.obj; | ||
| Object.setPrototypeOf(obj, v); | ||
| return true; | ||
| }, | ||
| }); | ||
| } | ||
| return proto[PROTO_OBJ_PROXY]; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor contextEggObjectProxy to reduce code duplication
The handler methods in contextEggObjectProxy repeatedly retrieve eggObject and obj. Refactoring can enhance maintainability by eliminating this duplication.
Introduce a helper function to retrieve obj, similar to getObj in eggObjectProxy:
static contextEggObjectProxy(proto: EggPrototype, objName: PropertyKey): PropertyDescriptor {
const PROTO_OBJ_PROXY = Symbol(`EggPrototype#objProxy#${String(objName)}`);
if (!proto[PROTO_OBJ_PROXY]) {
function getObj() {
const eggObject = EggContainerFactory.getEggObject(proto, objName);
return eggObject.obj;
}
proto[PROTO_OBJ_PROXY] = new Proxy({}, {
defineProperty(_target: object, property: string | symbol, attributes: PropertyDescriptor): boolean {
const obj = getObj();
Object.defineProperty(obj, property, attributes);
return true;
},
deleteProperty(_target: object, p: string | symbol): boolean {
const obj = getObj();
delete obj[p];
return true;
},
get(target: object, p: string | symbol): any {
// make get be lazy
if (p === 'then') return;
if (Object.prototype[p]) {
return target[p];
}
const obj = getObj();
return obj[p];
},
// ...other handler methods using getObj()...
});
}
return proto[PROTO_OBJ_PROXY];
}This refactoring reduces repetitive calls to EggContainerFactory.getEggObject(proto, objName) and improves code readability.
🧰 Tools
🪛 Biome
[error] 102-102: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 108-108: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 114-114: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 124-124: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 134-134: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 155-155: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 161-161: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| const PROTO_OBJ_PROXY = Symbol(`EggPrototype#objProxy#${String(objName)}`); | ||
| if (!proto[PROTO_OBJ_PROXY]) { | ||
| proto[PROTO_OBJ_PROXY] = new Proxy({}, { | ||
| defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean { |
There was a problem hiding this comment.
Replace '{}' with 'object' in type annotations
Similar to the previous function, update the type annotations from '{}' to 'object' in the handler methods to accurately reflect the intended type.
Apply the following changes:
- defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean {
+ defineProperty(_target: object, property: string | symbol, attributes: PropertyDescriptor): boolean {
- deleteProperty(_target: {}, p: string | symbol): boolean {
+ deleteProperty(_target: object, p: string | symbol): boolean {
- get(target: {}, p: string | symbol): any {
+ get(target: object, p: string | symbol): any {
- getOwnPropertyDescriptor(_target: {}, p: string | symbol): PropertyDescriptor | undefined {
+ getOwnPropertyDescriptor(_target: object, p: string | symbol): PropertyDescriptor | undefined {
- has(_target: {}, p: string | symbol): boolean {
+ has(_target: object, p: string | symbol): boolean {
- set(_target: {}, p: string | symbol, newValue: any): boolean {
+ set(_target: object, p: string | symbol, newValue: any): boolean {
- setPrototypeOf(_target: {}, v: object | null): boolean {
+ setPrototypeOf(_target: object, v: object | null): boolean {Also applies to: 108-108, 114-114, 124-124, 134-134, 155-155, 161-161
🧰 Tools
🪛 Biome
[error] 102-102: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| static eggObjectProxy(eggObject: EggObject): PropertyDescriptor { | ||
| let _obj: object; | ||
| function getObj() { | ||
| if (!_obj) { | ||
| _obj = eggObject.obj; | ||
| } | ||
| return _obj; | ||
| } | ||
|
|
||
| const proxy = new Proxy({}, { | ||
| defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean { | ||
| const obj = getObj(); | ||
| Object.defineProperty(obj, property, attributes); | ||
| return true; | ||
| }, | ||
| deleteProperty(_target: {}, p: string | symbol): boolean { | ||
| const obj = getObj(); | ||
| delete obj[p]; | ||
| return true; | ||
| }, | ||
| get(target: {}, p: string | symbol): any { | ||
| // make get be lazy | ||
| if (p === 'then') return; | ||
| if (Object.prototype[p]) { | ||
| return target[p]; | ||
| } | ||
| const obj = getObj(); | ||
| return obj[p]; | ||
| }, | ||
| getOwnPropertyDescriptor(_target: {}, p: string | symbol): PropertyDescriptor | undefined { | ||
| const obj = getObj(); | ||
| return Object.getOwnPropertyDescriptor(obj, p); | ||
| }, | ||
| getPrototypeOf(): object | null { | ||
| const obj = getObj(); | ||
| return Object.getPrototypeOf(obj); | ||
| }, | ||
| has(_target: {}, p: string | symbol): boolean { | ||
| const obj = getObj(); | ||
| return p in obj; | ||
| }, | ||
| isExtensible(): boolean { | ||
| const obj = getObj(); | ||
| return Object.isExtensible(obj); | ||
| }, | ||
| ownKeys(): ArrayLike<string | symbol> { | ||
| const obj = getObj(); | ||
| return Reflect.ownKeys(obj); | ||
| }, | ||
| preventExtensions(): boolean { | ||
| const obj = getObj(); | ||
| Object.preventExtensions(obj); | ||
| return true; | ||
| }, | ||
| set(_target: {}, p: string | symbol, newValue: any): boolean { | ||
| const obj = getObj(); | ||
| obj[p] = newValue; | ||
| return true; | ||
| }, | ||
| setPrototypeOf(_target: {}, v: object | null): boolean { | ||
| const obj = getObj(); | ||
| Object.setPrototypeOf(obj, v); | ||
| return true; | ||
| }, | ||
| }); | ||
| return proxy; | ||
| } |
There was a problem hiding this comment.
Clarify the return type of eggObjectProxy method
The eggObjectProxy method is declared to return a PropertyDescriptor, but it returns a Proxy object instead. To maintain type consistency and correctness, consider returning a valid PropertyDescriptor or updating the return type.
Option 1: Return a valid PropertyDescriptor
Modify the method to return a PropertyDescriptor with the proxy as its value:
static eggObjectProxy(eggObject: EggObject): PropertyDescriptor {
let _obj: object;
function getObj() {
if (!_obj) {
_obj = eggObject.obj;
}
return _obj;
}
const proxy = new Proxy({}, {
// ...handler methods...
});
- return proxy;
+ return {
+ value: proxy,
+ configurable: true,
+ enumerable: true,
+ };
}Option 2: Update the return type
If the intention is to return the Proxy object directly, update the return type:
- static eggObjectProxy(eggObject: EggObject): PropertyDescriptor {
+ static eggObjectProxy(eggObject: EggObject): any {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| static eggObjectProxy(eggObject: EggObject): PropertyDescriptor { | |
| let _obj: object; | |
| function getObj() { | |
| if (!_obj) { | |
| _obj = eggObject.obj; | |
| } | |
| return _obj; | |
| } | |
| const proxy = new Proxy({}, { | |
| defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean { | |
| const obj = getObj(); | |
| Object.defineProperty(obj, property, attributes); | |
| return true; | |
| }, | |
| deleteProperty(_target: {}, p: string | symbol): boolean { | |
| const obj = getObj(); | |
| delete obj[p]; | |
| return true; | |
| }, | |
| get(target: {}, p: string | symbol): any { | |
| // make get be lazy | |
| if (p === 'then') return; | |
| if (Object.prototype[p]) { | |
| return target[p]; | |
| } | |
| const obj = getObj(); | |
| return obj[p]; | |
| }, | |
| getOwnPropertyDescriptor(_target: {}, p: string | symbol): PropertyDescriptor | undefined { | |
| const obj = getObj(); | |
| return Object.getOwnPropertyDescriptor(obj, p); | |
| }, | |
| getPrototypeOf(): object | null { | |
| const obj = getObj(); | |
| return Object.getPrototypeOf(obj); | |
| }, | |
| has(_target: {}, p: string | symbol): boolean { | |
| const obj = getObj(); | |
| return p in obj; | |
| }, | |
| isExtensible(): boolean { | |
| const obj = getObj(); | |
| return Object.isExtensible(obj); | |
| }, | |
| ownKeys(): ArrayLike<string | symbol> { | |
| const obj = getObj(); | |
| return Reflect.ownKeys(obj); | |
| }, | |
| preventExtensions(): boolean { | |
| const obj = getObj(); | |
| Object.preventExtensions(obj); | |
| return true; | |
| }, | |
| set(_target: {}, p: string | symbol, newValue: any): boolean { | |
| const obj = getObj(); | |
| obj[p] = newValue; | |
| return true; | |
| }, | |
| setPrototypeOf(_target: {}, v: object | null): boolean { | |
| const obj = getObj(); | |
| Object.setPrototypeOf(obj, v); | |
| return true; | |
| }, | |
| }); | |
| return proxy; | |
| } | |
| static eggObjectProxy(eggObject: EggObject): PropertyDescriptor { | |
| let _obj: object; | |
| function getObj() { | |
| if (!_obj) { | |
| _obj = eggObject.obj; | |
| } | |
| return _obj; | |
| } | |
| const proxy = new Proxy({}, { | |
| defineProperty(_target: {}, property: string | symbol, attributes: PropertyDescriptor): boolean { | |
| const obj = getObj(); | |
| Object.defineProperty(obj, property, attributes); | |
| return true; | |
| }, | |
| deleteProperty(_target: {}, p: string | symbol): boolean { | |
| const obj = getObj(); | |
| delete obj[p]; | |
| return true; | |
| }, | |
| get(target: {}, p: string | symbol): any { | |
| // make get be lazy | |
| if (p === 'then') return; | |
| if (Object.prototype[p]) { | |
| return target[p]; | |
| } | |
| const obj = getObj(); | |
| return obj[p]; | |
| }, | |
| getOwnPropertyDescriptor(_target: {}, p: string | symbol): PropertyDescriptor | undefined { | |
| const obj = getObj(); | |
| return Object.getOwnPropertyDescriptor(obj, p); | |
| }, | |
| getPrototypeOf(): object | null { | |
| const obj = getObj(); | |
| return Object.getPrototypeOf(obj); | |
| }, | |
| has(_target: {}, p: string | symbol): boolean { | |
| const obj = getObj(); | |
| return p in obj; | |
| }, | |
| isExtensible(): boolean { | |
| const obj = getObj(); | |
| return Object.isExtensible(obj); | |
| }, | |
| ownKeys(): ArrayLike<string | symbol> { | |
| const obj = getObj(); | |
| return Reflect.ownKeys(obj); | |
| }, | |
| preventExtensions(): boolean { | |
| const obj = getObj(); | |
| Object.preventExtensions(obj); | |
| return true; | |
| }, | |
| set(_target: {}, p: string | symbol, newValue: any): boolean { | |
| const obj = getObj(); | |
| obj[p] = newValue; | |
| return true; | |
| }, | |
| setPrototypeOf(_target: {}, v: object | null): boolean { | |
| const obj = getObj(); | |
| Object.setPrototypeOf(obj, v); | |
| return true; | |
| }, | |
| }); | |
| return { | |
| value: proxy, | |
| configurable: true, | |
| enumerable: true, | |
| }; | |
| } |
🧰 Tools
🪛 Biome
[error] 40-40: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 45-45: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 50-50: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 59-59: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 67-67: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 84-84: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
[error] 89-89: Don't use '{}' as a type.
Prefer explicitly define the object shape. '{}' means "any non-nullable value".
(lint/complexity/noBannedTypes)
| async initWithInjectConstructor(ctx: EggObjectLifeCycleContext) { | ||
| // 1. create inject deps | ||
| // 2. create obj | ||
| // 3. call obj lifecycle preCreate | ||
| // 4. call obj lifecycle postCreate | ||
| // 5. success create | ||
| try { | ||
| const constructArgs = await Promise.all(this.proto.injectObjects!.map(async injectObject => { | ||
| const proto = injectObject.proto; | ||
| const loadUnit = LoadUnitFactory.getLoadUnitById(proto.loadUnitId); | ||
| if (!loadUnit) { | ||
| throw new Error(`can not find load unit: ${proto.loadUnitId}`); | ||
| } | ||
| if (this.proto.initType !== ObjectInitType.CONTEXT && injectObject.proto.initType === ObjectInitType.CONTEXT) { | ||
| return EggObjectUtil.contextEggObjectProxy(proto, injectObject.objName); | ||
| } | ||
| const injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| return EggObjectUtil.eggObjectProxy(injectObj); | ||
| })); | ||
|
|
||
| this._obj = this.proto.constructEggObject(...constructArgs); | ||
| const objLifecycleHook = this._obj as EggObjectLifecycle; | ||
|
|
||
| // global hook | ||
| await EggObjectLifecycleUtil.objectPreCreate(ctx, this); | ||
| // self hook | ||
| const postConstructMethod = EggObjectLifecycleUtil.getLifecycleHook('postConstruct', this.proto) ?? 'postConstruct'; | ||
| if (objLifecycleHook[postConstructMethod]) { | ||
| await objLifecycleHook[postConstructMethod](ctx, this); | ||
| } | ||
|
|
||
| const preInjectMethod = EggObjectLifecycleUtil.getLifecycleHook('preInject', this.proto) ?? 'preInject'; | ||
| if (objLifecycleHook[preInjectMethod]) { | ||
| await objLifecycleHook[preInjectMethod](ctx, this); | ||
| } | ||
|
|
||
| // global hook | ||
| await EggObjectLifecycleUtil.objectPostCreate(ctx, this); | ||
|
|
||
| // self hook | ||
| const postInjectMethod = EggObjectLifecycleUtil.getLifecycleHook('postInject', this.proto) ?? 'postInject'; | ||
| if (objLifecycleHook[postInjectMethod]) { | ||
| await objLifecycleHook[postInjectMethod](ctx, this); | ||
| } | ||
|
|
||
| const initMethod = EggObjectLifecycleUtil.getLifecycleHook('init', this.proto) ?? 'init'; | ||
| if (objLifecycleHook[initMethod]) { | ||
| await objLifecycleHook[initMethod](ctx, this); | ||
| } | ||
|
|
||
| this.status = EggObjectStatus.READY; | ||
| } catch (e) { | ||
| this.status = EggObjectStatus.ERROR; | ||
| throw e; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to eliminate duplicate code
The methods initWithInjectConstructor and initWithInjectProperty contain similar logic for handling injected dependencies, lifecycle hooks, and error handling. To improve maintainability and adhere to the DRY (Don't Repeat Yourself) principle, consider extracting the shared logic into utility functions or a common private method.
| const constructArgs = await Promise.all(this.proto.injectObjects!.map(async injectObject => { | ||
| const proto = injectObject.proto; | ||
| const loadUnit = LoadUnitFactory.getLoadUnitById(proto.loadUnitId); | ||
| if (!loadUnit) { | ||
| throw new Error(`can not find load unit: ${proto.loadUnitId}`); | ||
| } | ||
| if (this.proto.initType !== ObjectInitType.CONTEXT && injectObject.proto.initType === ObjectInitType.CONTEXT) { | ||
| return EggObjectUtil.contextEggObjectProxy(proto, injectObject.objName); | ||
| } | ||
| const injectObj = await EggContainerFactory.getOrCreateEggObject(proto, injectObject.objName); | ||
| return EggObjectUtil.eggObjectProxy(injectObj); | ||
| })); |
There was a problem hiding this comment.
Ensure null safety when accessing this.proto.injectObjects
In the line this.proto.injectObjects!.map(...), the non-null assertion operator ! assumes that injectObjects is neither null nor undefined. To prevent potential runtime errors, consider adding a check to handle cases where this.proto.injectObjects might be undefined or null.
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
Injectannotation within theHelloServiceclass.ConstructorObjectclass created to showcase dependency injection with various decorators.MetadataUtilclass.Bug Fixes
Tests
Documentation