feat: impl MultiInstance inject MultiInstance #240
Conversation
WalkthroughThe changes include significant updates across multiple files, primarily focusing on the introduction of multi-instance prototype handling within the application. Key modifications involve the addition of new classes, methods, and properties to manage multi-instance dependencies, as well as updates to existing methods to accommodate these new functionalities. Configuration files for various modules have also been created or updated to support the new structure, enhancing the application's modular capabilities. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 21
🧹 Outside diff range and nitpick comments (35)
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/module.yml (1)
1-4: Clarify the purpose and enhance the test fixtureThis new configuration file introduces a
secretsection withkeys, which is good for testing multi-instance configurations. However, there are a few points to consider:
The file lacks comments explaining its purpose and usage within the test suite. Adding comments would improve maintainability.
The secret keys ('1' and '2') are overly simplistic. While this might be intentional for testing, using more realistic keys could enhance the test's effectiveness.
Consider applying the following changes:
+# Configuration for app2 module in multi-instance inject test secret: keys: - - '1' - - '2' + - 'test_key_instance1' + - 'test_key_instance2'This change adds a comment explaining the file's purpose and uses more descriptive key names, which could make the tests more robust and easier to understand.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/module.yml (1)
1-4: LGTM with suggestions for improvementThe YAML structure is correct and the configuration is clear. However, I have a few suggestions to enhance this test fixture:
- Consider adding a comment to explain the purpose of this configuration and how it relates to the multi-instance injection feature.
- Use more realistic (but still fake) secret values to make the test more robust. For example, you could use UUIDs or longer alphanumeric strings.
Example:
# Configuration for testing multi-instance secret injection secret: keys: - 'test-secret-key-7e9d4f5a3b1c8e2d' - 'test-secret-key-2c4b6a8d9f1e3g5h'This approach maintains the test's simplicity while making it more representative of real-world scenarios.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/package.json (1)
1-3: LGTM! Minimal package.json structure is suitable for test fixture.The package.json file is correctly structured and the name property matches the directory name, which is a good practice. As this appears to be a test fixture for the multi-instance injection feature, the minimal structure is acceptable.
If this were for a real project rather than a test fixture, consider adding more fields such as "version", "description", "main", "scripts", and "dependencies" to provide a more complete package configuration.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/package.json (1)
1-6: LGTM! Consider adding a brief description.The
package.jsonfile for the "bar" module is correctly structured and consistent in its naming. It follows the expected format for an Egg.js module configuration.Consider adding a "description" field to provide a brief explanation of the module's purpose. This can enhance maintainability and clarity, especially in a test fixtures context. For example:
{ "name": "bar", + "description": "Test fixture for multi-instance injection in Egg.js", "eggModule": { "name": "bar" } }core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/package.json (1)
1-6: LGTM! Consider adding a description field.The
package.jsonfile for the "app2" module is correctly structured and consistent in its naming. It appropriately defines the module name and the correspondingeggModuleproperty, which is crucial for the multi-instance injection feature mentioned in the PR objectives.Consider adding a "description" field to provide more context about the purpose of this module, especially since it's part of a test fixture. This can enhance clarity for other developers working on or reviewing these tests. For example:
{ "name": "app2", + "description": "Test module for multi-instance injection scenarios", "eggModule": { "name": "app2" } }core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (1)
1-4: LGTM! Consider adding a comment for clarity.The
BizManagerstructure withclientslooks good. It sets up a configuration for multiple clients (fooandbar) which aligns with the multi-instance inject feature mentioned in the PR objectives.Consider adding a comment above the
BizManagersection to explain its purpose and the significance of thefooandbarclients. This would enhance code readability and maintainability. For example:# BizManager configuration for handling multiple client instances BizManager: clients: foo: {} # Configuration for foo client bar: {} # Configuration for bar clientplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/module.yml (1)
1-4: LGTM! Consider adding comments for clarity.The
BizManagerconfiguration structure looks good and aligns with the multi-instance injection feature. Theclientssection correctly defines two clients,fooandbar, with empty configurations, which allows for flexible setups.To improve maintainability, consider adding comments to explain:
- The purpose of the
BizManagercomponent.- The intended use of the
fooandbarclients.- Why the client configurations are left empty (if this is intentional).
Example:
# BizManager: Manages multiple business logic instances BizManager: # Clients represent different business logic instances clients: # foo: Instance for handling X operations foo: {} # bar: Instance for handling Y operations bar: {}plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/plugin.js (1)
1-2: Remove redundant 'use strict' directiveThe 'use strict' directive is unnecessary in JavaScript modules as they are automatically in strict mode.
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)
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/App.ts (3)
4-5: LGTM: Class declaration is correct. Consider a more descriptive name.The
App2class is correctly declared and exported, with the@SingletonProto()decorator appropriately applied. This setup aligns with the multi-instance injection feature mentioned in the PR objectives.Consider using a more descriptive name for the class (e.g.,
SecondaryApporAppInstanceTwo) to clarify its role in the multi-instance setup. This would improve code readability and make the purpose of this class more evident.
6-8: LGTM: Property declaration aligns with multi-instance injection. Consider adding a comment.The
secretproperty is correctly declared with appropriate decorators for dependency injection and qualification. This setup effectively implements the multi-instance injection feature.Consider adding a brief comment explaining the purpose of this
secretproperty and why it's qualified with 'app2'. This would enhance code readability and make it easier for other developers to understand the multi-instance setup.Example:
/** * Injects the Secret instance qualified for 'app2' in the multi-instance setup. */ @Inject() @SecretQualifier('app2') secret: Secret;
1-9: Overall, excellent implementation of multi-instance injection.This file successfully implements the
App2class with multi-instance injection capabilities, aligning perfectly with the PR objectives. The code is well-structured, uses appropriate decorators, and follows good TypeScript practices.As this file is part of a larger multi-instance setup:
- Ensure consistent naming and documentation patterns across related files (e.g.,
App1if it exists).- Consider creating a README or documentation file explaining the overall multi-instance architecture, including how different
Appinstances interact and how they're used in the broader system.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/App.ts (2)
4-5: LGTM: Class declaration is appropriate. Consider a more descriptive name.The
@SingletonProto()decorator correctly indicates that this class is intended to be a singleton service. The class is properly exported for use in other modules.Consider using a more descriptive name than
App2. For example, if this class represents a specific functionality or module, you could name it accordingly (e.g.,UserService,DataProcessor, etc.). This would improve code readability and make the purpose of the class clearer.
6-8: LGTM: Property declaration implements multi-instance injection as intended.The
secretproperty is correctly decorated for dependency injection with a specific qualifier, which aligns with the PR objective of implementing multi-instance injection.Consider using a non-null assertion operator (
!) or initializing the property to ensure type safety:@Inject() @SecretQualifier('app2') secret!: Secret;This change would indicate to TypeScript that the property will be definitely assigned before use, preventing potential null reference errors.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/App.ts (2)
4-5: LGTM: Class declaration is correct, but consider a more specific name.The
@SingletonProto()decorator is correctly applied, ensuring a single instance of this class. However, the class name 'App' is quite generic. While this might be intentional for a test fixture, in a larger codebase, a more specific name could improve clarity.Consider renaming the class to something more specific, e.g.,
TestApporMultiInstanceApp, to better reflect its purpose in the test suite.
6-8: LGTM: Property declaration demonstrates correct use of dependency injection and qualifiers.The
bizManagerproperty is correctly set up with dependency injection using the@Inject()decorator, and the@BizManagerQualifier('foo')decorator effectively specifies which instance ofBizManagershould be injected.Consider the following improvements:
- Add an access modifier to the property for better encapsulation:
@Inject() @BizManagerQualifier('foo') private bizManager: BizManager;
- If
bizManagerneeds to be accessed from outside the class, consider adding a getter method instead of making the property public:private bizManager: BizManager; public getBizManager(): BizManager { return this.bizManager; }These changes would improve encapsulation and provide more control over how
bizManageris accessed and potentially modified.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.ts (1)
6-8: LGTM: Property declaration aligns with multi-instance injection, but consider encapsulation.The
bizManagerproperty is correctly set up for dependency injection with the@Inject()decorator, and the@BizManagerQualifier('foo')decorator appropriately specifies which instance ofBizManagerto inject. This implementation aligns well with the PR objective of multi-instance injection.However, consider making this property private or readonly to better encapsulate the class's internal state:
@Inject() @BizManagerQualifier('foo') private readonly bizManager: BizManager;This change would prevent external modification of the
bizManagerproperty while still allowing it to be injected.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/config.default.js (1)
5-15: Consider utilizing or removing the unused 'appInfo' parameter.The exported function takes an 'appInfo' parameter, but it's not used within the function body. This might indicate missing functionality or an oversight.
If 'appInfo' is not needed, consider removing it:
-module.exports = function(appInfo) { +module.exports = function() {Alternatively, if 'appInfo' should be used, consider adding the necessary logic to utilize it in the configuration.
core/types/core-decorator/model/EggPrototypeInfo.ts (1)
33-36: Approved with a suggestion for improved documentation.The addition of the
properQualifiersproperty is a logical extension of the interface. It follows the existing code style and naming conventions, and its optional nature ensures backward compatibility.However, the documentation could be more descriptive to clarify the distinction between
qualifiersandproperQualifiers.Consider expanding the documentation comment to provide more context:
/** * Qualifiers specific to the properties of the EggPrototype. * While 'qualifiers' apply to the EggPrototype as a whole, * 'properQualifiers' are used for individual properties within the prototype. */ properQualifiers?: QualifierInfo[];plugin/tegg/test/MultiInstanceInjectMultiInstance.test.ts (2)
7-28: Test suite setup follows best practices, but could benefit from error handling.The setup is well-structured with appropriate hooks for initialization and cleanup. The mock application is created correctly, and environment variables are properly set.
Consider adding error handling to the
beforehook:before(async () => { - mm(process.env, 'EGG_TYPESCRIPT', true); - mm(process, 'cwd', () => { - return path.join(__dirname, '..'); - }); - app = mm.app({ - baseDir: path.join(__dirname, 'fixtures/apps/app-multi-inject-multi'), - framework: require.resolve('egg'), - }); - await app.ready(); + try { + mm(process.env, 'EGG_TYPESCRIPT', true); + mm(process, 'cwd', () => { + return path.join(__dirname, '..'); + }); + app = mm.app({ + baseDir: path.join(__dirname, 'fixtures/apps/app-multi-inject-multi'), + framework: require.resolve('egg'), + }); + await app.ready(); + } catch (error) { + console.error('Error setting up test environment:', error); + throw error; + } });This will provide better visibility into any setup failures.
30-40: Test case is well-structured but could benefit from more descriptive assertions.The test case "dynamic inject should work" effectively verifies the functionality of dynamic injection by checking both the retrieval of instances and their property values. The assertions cover the main aspects of the expected behavior.
Consider enhancing the assertions with more descriptive messages:
- assert.equal(app2Secret, 'mock233'); - assert.equal(appName, 'foo'); - assert.equal(appSecret, 'foo233'); + assert.equal(app2Secret, 'mock233', 'App2 secret should be "mock233"'); + assert.equal(appName, 'foo', 'App business manager name should be "foo"'); + assert.equal(appSecret, 'foo233', 'App secret should be "foo233"');This will make it easier to identify which assertion failed if the test doesn't pass.
Additionally, consider adding a test case for error handling, such as trying to inject a non-existent module or passing invalid parameters to
getEggObject().standalone/standalone/src/EggModuleLoader.ts (2)
41-41: Approve with suggestions: New multi-instance setup addedThe new line
LoadUnitMultiInstanceProtoHook.setAllClassList(appGraph.getClazzList());has been added to set up multi-instance prototype handling. While this change looks good, consider the following suggestions:
- Add error handling to ensure
appGraph.getClazzList()returns a valid result before passing it tosetAllClassList().- Consider the performance implications of setting a global list of all classes, especially for large applications.
- Add a comment explaining the purpose of this new line for better code maintainability.
Here's a suggested improvement:
// Set up multi-instance prototype handling for all classes const classList = appGraph.getClazzList(); if (classList && classList.length > 0) { LoadUnitMultiInstanceProtoHook.setAllClassList(classList); } else { console.warn('No classes found in appGraph. Multi-instance prototype handling may not work as expected.'); }
Ensure Consistent Usage of LoadUnitMultiInstanceProtoHook in preLoad Method
The
LoadUnitMultiInstanceProtoHookis utilized in theloadmethod but is absent in thepreLoadmethod. This inconsistency may lead to unexpected behavior during the preloading of modules. Please address the following:
- Consistency: Apply
LoadUnitMultiInstanceProtoHook.setAllClassList(appGraph.getClazzList());within thepreLoadmethod to ensure consistent handling of multi-instance prototypes.- Testing: Update and add tests to verify that the
preLoadmethod correctly applies the multi-instance prototype handling.- Documentation: Reflect these changes in the relevant documentation to guide future developers on the module loading process.
- Performance Assessment: Evaluate any performance implications of adding this hook to the
preLoadmethod, especially in large-scale applications.🔗 Analysis chain
Line range hint
1-71: Consider broader implications and consistencyThe introduction of multi-instance prototype handling is a significant change that could affect the overall behavior of the application. Please consider the following points:
Consistency: The new functionality is added to the
loadmethod but not to thepreLoadmethod. Consider if it should also be implemented inpreLoadfor consistency.Testing: Ensure that comprehensive tests are added to verify the correct behavior of the new multi-instance functionality.
Documentation: Update any relevant documentation to reflect these changes and explain how multi-instance prototype handling works in the context of this loader.
Performance: Assess the performance impact of this change, especially for large applications with many classes.
To help verify the consistency between
loadandpreLoadmethods, you can run the following script:This script will help identify if
LoadUnitMultiInstanceProtoHookis used consistently in both methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if LoadUnitMultiInstanceProtoHook is used consistently in load and preLoad methods # Search for LoadUnitMultiInstanceProtoHook usage in the file echo "Searching for LoadUnitMultiInstanceProtoHook usage:" rg --type typescript "LoadUnitMultiInstanceProtoHook" standalone/standalone/src/EggModuleLoader.ts echo "\nChecking load method:" rg --type typescript -A 10 "async load\(\):" standalone/standalone/src/EggModuleLoader.ts echo "\nChecking preLoad method:" rg --type typescript -A 10 "static async preLoad\(" standalone/standalone/src/EggModuleLoader.tsLength of output: 627
Script:
#!/bin/bash # Description: Check if LoadUnitMultiInstanceProtoHook is used consistently in load and preLoad methods without using the --type flag # Search for LoadUnitMultiInstanceProtoHook usage in the file echo "Searching for LoadUnitMultiInstanceProtoHook usage in .ts files:" rg "LoadUnitMultiInstanceProtoHook" --type-add 'ts=*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.ts echo "\nChecking load method:" rg -A 10 "async load\(\):" --type-add 'ts=*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.ts echo "\nChecking preLoad method:" rg -A 10 "static async preLoad\(" --type-add 'ts=*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.tsLength of output: 767
Script:
#!/bin/bash # Description: Check if LoadUnitMultiInstanceProtoHook is used consistently in load and preLoad methods with correct type definitions # Search for LoadUnitMultiInstanceProtoHook usage in the file echo "Searching for LoadUnitMultiInstanceProtoHook usage in .ts files:" rg "LoadUnitMultiInstanceProtoHook" --type-add 'ts:*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.ts echo "\nChecking load method:" rg -A 10 "async load\(\):" --type-add 'ts:*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.ts echo "\nChecking preLoad method:" rg -A 10 "static async preLoad\(" --type-add 'ts:*.ts' -g '*.ts' standalone/standalone/src/EggModuleLoader.tsLength of output: 1949
plugin/tegg/app.ts (2)
70-70: Approve the addition with a minor suggestion for robustness.The addition of
LoadUnitMultiInstanceProtoHook.clear()is a good practice for cleaning up resources and state associated with the multi-instance prototype hook. This aligns well with the PR objective of implementing multi-instance injection.For improved robustness, consider wrapping the call in a try-catch block to handle any potential errors during cleanup:
- LoadUnitMultiInstanceProtoHook.clear(); + try { + LoadUnitMultiInstanceProtoHook.clear(); + } catch (error) { + this.app.logger.error('Error clearing LoadUnitMultiInstanceProtoHook:', error); + }This ensures that any unexpected errors during the cleanup process won't prevent the rest of the
beforeClosemethod from executing.
Line range hint
62-71: Consider refactoring thebeforeClosemethod for improved maintainability.The
beforeClosemethod is responsible for cleaning up various resources. To improve its maintainability and consistency, consider the following suggestions:
- Extract the cleanup logic for each hook into separate private methods.
- Implement a consistent error handling strategy for all cleanup operations.
Here's an example of how you could refactor the
beforeClosemethod:private cleanupHook(hook: any, name: string) { try { if (hook) { this.app.loadUnitLifecycleUtil.deleteLifecycle(hook); if (typeof hook.clear === 'function') { hook.clear(); } } } catch (error) { this.app.logger.error(`Error cleaning up ${name}:`, error); } } async beforeClose() { CompatibleUtil.clean(); await this.app.moduleHandler.destroy(); this.cleanupHook(this.compatibleHook, 'compatibleHook'); this.cleanupHook(this.eggQualifierProtoHook, 'eggQualifierProtoHook'); this.cleanupHook(this.configSourceEggPrototypeHook, 'configSourceEggPrototypeHook'); this.cleanupHook(this.loadUnitMultiInstanceProtoHook, 'loadUnitMultiInstanceProtoHook'); }This refactoring improves code organization, reduces duplication, and ensures consistent error handling for all hook cleanups.
core/metadata/test/AppGraph.test.ts (1)
40-76: LGTM: New test case for multi-instance injection, with suggestions for improvementThe new test case "multi instance inject multi instance should work" is a good addition to verify the functionality of multi-instance injection. It correctly sets up an AppGraph with multiple ModuleNodes and checks the final order of modules.
However, consider the following suggestions to enhance the test:
- Add comments explaining why the specific module order is expected.
- Include assertions to verify properties of the injected instances, demonstrating the multi-instance behavior.
- Consider adding a test case that shows how the injection differs from single-instance behavior.
Here's an example of how you might enhance the test:
it('multi instance inject multi instance should work', () => { const graph = new AppGraph(); // ... (existing setup code) ... graph.build(); graph.sort(); // Verify the module order assert.deepStrictEqual(graph.moduleConfigList.map(t => t.name), [ 'app', 'app2', 'bar', 'foo', ], 'Modules should be sorted in the correct order'); // Verify multi-instance behavior const appInstance = graph.getModule('app'); const app2Instance = graph.getModule('app2'); assert.notStrictEqual(appInstance, app2Instance, 'App and App2 should be separate instances'); // Verify injected dependencies assert(appInstance.bizManager instanceof BizManager, 'App should have BizManager injected'); assert(app2Instance.bizManager instanceof BizManager, 'App2 should have BizManager injected'); assert.notStrictEqual(appInstance.bizManager, app2Instance.bizManager, 'App and App2 should have different BizManager instances'); });This enhanced version includes explanatory comments, verifies the multi-instance behavior, and checks the injected dependencies.
plugin/dal/lib/DataSource.ts (1)
66-72: Consider removing the TypeScript ignore comment.The addition of the
transactionalAOPproperty with the@Injectdecorator is good. It suggests the implementation of transaction-related functionality, which aligns with the class's purpose.However, the presence of a TypeScript ignore comment (
@ts-ignore) might indicate a temporary workaround or incomplete type definitions. If possible, consider addressing the underlying type issue instead of using@ts-ignore.If the type issue can be resolved, apply this diff:
- // eslint-disable-next-line @typescript-eslint/ban-ts-comment - // @ts-ignore private transactionalAOP: TransactionalAOP;plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.ts (1)
27-27: Consistently use braces for control statementsFor better readability and to prevent potential bugs during future modifications, it's advisable to use braces
{}even for single-lineifstatements.Apply this change to include braces:
if (!keys || keys.length === 0) - return []; +{ + return []; +}core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (2)
28-29: Use consistent language for code commentsThe comment on lines 28-29 is in Chinese. To maintain consistency and readability for all team members, consider translating code comments into English.
Update the comment to:
// Dynamically get configuration from module.yml to decide how many objects need to be initialized
30-32: Strongly type the configuration objectCurrently,
configis typed asany. To enhance type safety and prevent potential runtime errors, consider defining an interface for the configuration object and using it instead ofany.Define an interface for the expected configuration structure:
interface ModuleConfig { BizManager?: { clients?: Record<string, any>; }; } const config = ModuleConfigUtil.loadModuleConfigSync(ctx.unitPath) as ModuleConfig;plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.ts (1)
28-28: Translate comment to English for consistency.The comment on line 28 is in Chinese. For consistency and maintainability, it's recommended to have all comments in English.
Update the comment as follows:
initType: ObjectInitType.SINGLETON, - // 从 module.yml 中动态获取配置来决定需要初始化几个对象 + // Dynamically load configuration from module.yml to decide how many objects to initialize getObjects(ctx: MultiInstancePrototypeGetObjectsContext) {core/metadata/src/model/AppGraph.ts (1)
159-159: Remove commented-out code to maintain code cleanlinessThe line
// result.add(obj.ownerModule);is commented out. If addingobj.ownerModuleto the result set is no longer necessary, consider removing this commented-out line to keep the code clean. If it's needed for future reference, adding a comment explaining why it's commented out would be helpful.core/metadata/test/LoadUnit.test.ts (1)
152-152: Consider rephrasing the test description for clarityThe current test description
'should multi instance inject multi instance work'is grammatically unclear. Rephrasing it can improve readability and accurately convey the purpose of the test.Apply this diff to improve the test description:
-it('should multi instance inject multi instance work', async () => { +it('should correctly inject multi-instance dependencies', async () => {core/metadata/src/impl/ModuleLoadUnit.ts (3)
256-258: Review the necessity of cloningclazzListIn the
doLoadClazzmethod,clazzList.slice()is used to create a shallow copy assigned toresult. SinceclazzListis not modified elsewhere in the method, cloning might be unnecessary. Verify whether cloning is required or ifclazzListcan be used directly to optimize memory usage.
296-297: Avoid redundant calls toloadClazzinpreLoadThe
loadClazzmethod is called in bothpreLoadandinit. SinceloadClazzcaches the result after the first call, subsequent calls may be unnecessary. Review whether both calls are needed, and if not, remove the redundant invocation to enhance performance.
306-308: Redundant call toloadClazzininitmethodSimilar to
preLoad, theinitmethod callsloadClazz, which may already have been called and cached. Assess whether this call is required or if the logic can be adjusted to prevent unnecessary execution.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (36)
- core/metadata/src/impl/LoadUnitMultiInstanceProtoHook.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (8 hunks)
- core/metadata/src/model/AppGraph.ts (6 hunks)
- core/metadata/test/AppGraph.test.ts (2 hunks)
- core/metadata/test/LoadUnit.test.ts (3 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/App.ts (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/package.json (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/App.ts (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/module.yml (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/package.json (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/package.json (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/Secret.ts (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/package.json (1 hunks)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/package.json (1 hunks)
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/types/core-decorator/model/EggPrototypeInfo.ts (1 hunks)
- plugin/dal/lib/DataSource.ts (3 hunks)
- plugin/tegg/app.ts (1 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (2 hunks)
- plugin/tegg/test/MultiInstanceInjectMultiInstance.test.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/module.yml (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/App.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/module.yml (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.ts (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/package.json (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/config.default.js (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/plugin.js (1 hunks)
- plugin/tegg/test/fixtures/apps/app-multi-inject-multi/package.json (1 hunks)
- standalone/standalone/src/EggModuleLoader.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- core/metadata/test/fixtures/modules/app-multi-inject-multi/package.json
🧰 Additional context used
🪛 Biome
core/metadata/src/impl/LoadUnitMultiInstanceProtoHook.ts
[error] 12-12: 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] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/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/app-multi-inject-multi/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 (44)
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/module.yml (1)
1-4: Ensure proper handling of sensitive data in test fixturesWhile this file is located in a test fixtures directory, it's important to ensure that any sensitive data, even if it's for testing purposes, is properly handled.
To ensure this file is only used for testing and not accidentally included in any production builds or deployments, please run the following verification script:
If the script returns any results, please review those locations to ensure they are appropriate for test usage.
✅ Verification successful
Sensitive data handling in test fixtures verified successfully
No issues found. The
module.ymlsecret configuration is only referenced within test directories.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the secret configuration is only referenced in test files # Test: Check if the module.yml file is referenced outside of test directories # Expect: No matches outside of test directories rg --type yaml -g '!*test*' -g '!*spec*' "secret:\s*keys:" --files-with-matches # Test: Check for any import or require statements that might be loading this configuration # Expect: Only matches in test files rg --type typescript --type javascript "require.*app-multi-inject-multi.*module\.yml" --files-with-matches rg --type typescript --type javascript "import.*app-multi-inject-multi.*module\.yml" --files-with-matchesLength of output: 368
Script:
#!/bin/bash # Description: Re-verify that the secret configuration is only referenced in test files with corrected file types # Test: Check if the module.yml file is referenced outside of test directories # Expect: No matches outside of test directories rg --type yaml -g '!*test*' -g '!*spec*' "secret:\s*keys:" --files-with-matches # Test: Check for any import or require statements that might be loading this configuration # Using correct type identifiers: 'ts' for TypeScript and 'js' for JavaScript # Expect: Only matches in test files rg --type ts --type js "require.*app-multi-inject-multi.*module\.yml" --files-with-matches rg --type ts --type js "import.*app-multi-inject-multi.*module\.yml" --files-with-matchesLength of output: 266
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/module.yml (1)
1-4: Verify the relationship with multi-instance injectionWhile this configuration is part of the test fixtures for the multi-instance injection feature, it's not immediately clear how it's utilized in the context of the feature. Could you please provide more context on how this
secretconfiguration is used in the multi-instance injection tests?To help understand the usage, we can search for references to this configuration file:
This will help us understand how the configuration is loaded and used in the multi-instance injection tests.
✅ Verification successful
Verification of Multi-Instance Injection Usage of Secret Configuration
The
secretconfiguration inmodule.ymlis actively utilized for multi-instance injection as evidenced by the following:
app/modules/foo/Secret.ts
- Accesses
config.secret.keysto manage secret keys.- Checks for the presence and length of
keys.
app/modules/bar/BizManager.ts
- Uses
Object.keys(clients)to initialize multiple client instances based on the provided configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for references to the module.yml file or its content rg --type yaml --type typescript --type javascript -i "module\.yml|secret.*keys" plugin/tegg/test/fixtures/apps/app-multi-inject-multiLength of output: 171
Script:
#!/bin/bash # Search for references to the module.yml file or its content excluding unsupported types rg --type yaml --type javascript -i "module\.yml|secret.*keys" plugin/tegg/test/fixtures/apps/app-multi-inject-multiLength of output: 153
Script:
#!/bin/bash # Search for references to the module.yml file or its content across common file extensions rg -i "module\.yml|secret.*keys" --glob "**/*.{yml,yaml,js,jsx,ts,tsx}" plugin/tegg/test/fixtures/apps/app-multi-inject-multiLength of output: 368
Script:
#!/bin/bash # Search for all usages of 'keys' within Secret.ts and BizManager.ts rg -i "keys" plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.ts plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/bar/BizManager.tsLength of output: 568
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/package.json (1)
1-6: LGTM! This package.json file is correctly structured for the multi-instance injection feature.The file is well-formed and contains the necessary information for the "app" module. It's appropriately placed in the test fixtures, which aligns with the PR objective of implementing multi-instance injection. The redundancy in naming (both "name" and "eggModule.name" set to "app") appears intentional and likely required by the egg framework.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/package.json (2)
1-6: LGTM! Module configuration looks correct.The
package.jsonfile for the "bar" module is well-structured and consistent. It correctly defines the module name and includes the necessaryeggModuleconfiguration. This file appears to be part of a test fixture for multi-instance injection in an Egg.js application.
1-6: Verify module integration in test suiteWhile this configuration file is correct, it's important to ensure that the "bar" module is properly integrated and utilized in the test suite for multi-instance injection.
To verify the module's integration, you can run the following script:
This script will help ensure that the "bar" module is being used in the test suite and that there are specific tests for the multi-instance injection feature.
✅ Verification successful
Module Integration Verified
The "bar" module is appropriately referenced in the test suite, and there are specific tests for multi-instance injection.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to the "bar" module in test files # Search for references to the "bar" module in test files echo "Searching for 'bar' module references in test files:" rg --type js --type ts "bar" core/metadata/test # Check if there are any tests specifically for multi-instance injection echo "Checking for multi-instance injection tests:" rg --type js --type ts "multi.*instance.*inject" core/metadata/testLength of output: 2541
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/foo/package.json (1)
1-6: LGTM! This package.json file correctly defines the 'foo' module for testing purposes.The file structure is correct and aligns with the PR objectives of implementing multi-instance injection. Here's a breakdown:
- The top-level "name" property correctly identifies the module as "foo".
- The "eggModule" object with its "name" property set to "foo" is likely used by the egg framework to recognize and handle this module.
This file, being in a test fixtures directory, will be crucial for testing the new multi-instance injection feature. It provides a controlled, predictable module definition that can be used in various test scenarios.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/package.json (1)
1-6: LGTM! The package.json file is correctly structured for the test fixture.The file correctly defines a module named "app" and includes the necessary "eggModule" property. This setup appears to be appropriate for testing multi-instance injection scenarios in an Egg.js application.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/package.json (1)
1-6: LGTM! The package.json file is correctly structured for the "foo" module.The file correctly defines the module configuration for the "foo" module in the Egg.js framework. It includes the necessary "name" property and the "eggModule" object with a matching name. This structure is consistent with best practices for Egg.js module definitions and will enable proper module recognition and loading within the application.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/package.json (1)
1-6: LGTM! The package.json file is correctly structured for the app2 module.The file is well-formed and follows the expected structure for an Egg.js module configuration. The consistency in naming between the root "name" property and the "eggModule.name" property is correct and likely required for proper module recognition.
Given that this file is located in a test fixtures directory, it appears to be part of a test setup for a multi-inject scenario. This configuration will help in testing the behavior of the application when dealing with multiple module instances.
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/module.yml (1)
6-9: Consider enhancing security for secret keys.The
secretstructure withkeysis set up correctly. However, there are some security considerations to keep in mind:
- The current keys ('1' and '2') are very simple and predictable. For better security, consider using more complex, randomly generated keys.
- Storing secret keys in a configuration file might not be the most secure approach, especially if this file is committed to version control.
To ensure that no sensitive information is accidentally committed, let's check if this file is gitignored:
If this file is not gitignored, consider moving the secret keys to a separate, gitignored file or using environment variables for sensitive information.
Consider implementing the following improvements:
- Use a secure key generation method to create more complex keys.
- If possible, use environment variables or a secure secret management system instead of storing keys in configuration files.
- Add a comment explaining the purpose of these keys and how they relate to the
BizManagerconfiguration.Example of a more secure configuration:
secret: # Keys for encrypting communication between BizManager clients keys: - ${SECRET_KEY_1} # Set this environment variable with a securely generated key - ${SECRET_KEY_2} # Set this environment variable with a securely generated keyplugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/module.yml (1)
6-9:⚠️ Potential issueReview security implications of key values.
The
secretconfiguration structure is correct. However, there are some potential security concerns to address:
- Using sequential numbers ('1' and '2') as secret keys might not be sufficiently secure.
- The purpose and usage of these keys in relation to the
BizManageris not clear.To ensure this isn't a widespread practice, let's check for similar patterns in other configuration files:
Consider the following improvements:
- Use more complex, non-sequential values for secret keys.
- Add comments explaining the purpose and usage of these keys.
- If these are test/dummy keys, clearly indicate so in a comment.
Example:
secret: # Keys used for encrypting communication between BizManager instances keys: - 'complex_random_string_1' - 'another_complex_random_string'If these are indeed test keys, consider:
secret: # Test keys - DO NOT USE IN PRODUCTION keys: - 'test_key_1' - 'test_key_2'plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/plugin.js (3)
3-6: LGTM: Tracer configuration looks goodThe tracer configuration enables the 'egg-tracer' package, which aligns with the PR objectives and can be beneficial for debugging and monitoring in a multi-instance environment.
8-11: LGTM: Tegg configuration looks goodThe teggConfig configuration enables the '@eggjs/tegg-config' package, which is appropriate for managing configurations in a multi-instance environment.
13-13: Verify watcher settingThe watcher is disabled, which might be intentional for performance reasons in a multi-instance setup. However, please confirm if this is the intended behavior for all environments.
✅ Verification successful
Watcher setting verified
All instances of
exports.watcherare consistently set tofalseacross the codebase, indicating that disabling the watcher is intentional for performance in multi-instance setups.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any environment-specific watcher configurations # Test: Search for watcher configurations in other files rg --type js 'exports\.watcher' --glob '!plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/plugin.js'Length of output: 1473
core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app2/App.ts (1)
1-2: LGTM: Import statements are appropriate and concise.The import statements are well-structured, importing only the necessary decorators and types. The use of relative import for the
Secretmodule maintains a modular structure.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app2/App.ts (2)
1-2: LGTM: Imports are appropriate and well-structured.The import statements are concise and import only the necessary decorators and types. The use of a relative import for the
Secrettype and its qualifier suggests a good modular structure.
1-9: Overall, the implementation successfully achieves multi-instance injection.The
App2class is well-structured and correctly implements multi-instance injection using appropriate decorators. This aligns with the PR objective of implementing "MultiInstance inject MultiInstance". The code is concise and follows good practices for dependency injection in TypeScript.To ensure consistency across the codebase, let's verify the usage of
@SecretQualifier:This will help ensure that the
@SecretQualifierdecorator andSecretimport are used consistently across the project.core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/app/App.ts (2)
1-2: LGTM: Import statements are appropriate and concise.The import statements correctly bring in the necessary decorators and types for the class implementation. The use of relative import for
BizManagerandBizManagerQualifiersuggests good modularization of the codebase.
1-9: Overall, the implementation is correct and serves its purpose as a test fixture.The
Appclass demonstrates the correct use of decorators for singleton behavior and dependency injection with qualifiers. While the current implementation is functional, consider the suggested improvements for better encapsulation and naming. These changes would make the code more robust and easier to maintain, even in a test context.plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.ts (2)
1-2: LGTM: Imports are appropriate and well-structured.The import statements are concise and import only the necessary decorators and types. The use of both external package imports and local module imports is correct and follows TypeScript best practices.
1-9: Summary: Implementation aligns well with PR objectivesThis file successfully sets up a basic structure for testing multi-instance injection, which aligns perfectly with the PR objective of implementing "MultiInstance inject MultiInstance". The use of
@SingletonProto(),@Inject(), and@BizManagerQualifier()decorators effectively demonstrates the intended functionality.While there are minor suggestions for improvement (class naming and property encapsulation), the overall implementation is solid and achieves the desired goal. Good job on implementing this feature!
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/config.default.js (1)
6-13: Configuration structure looks good, with a note on the 'keys' value.The configuration object is well-structured with appropriate security settings, particularly for CSRF protection. The explicit setting of
ignoreJSON: falsefor CSRF is a good security practice.Note: The static 'test key' for the 'keys' property is acceptable for a test fixture but should never be used in a production environment. Ensure that this configuration file is only used for testing purposes.
core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1)
9-9: LGTM! Consider adding documentation for the new property.The addition of the
properQualifiersproperty to theObjectInfointerface is a good enhancement. It allows for more granular association of qualifiers with specific object properties, which can be useful for complex object structures.Consider adding a brief comment above the new property to explain its purpose and usage. For example:
/** * Maps object property keys to their associated qualifiers. * This allows for more granular qualifier associations than the general `qualifiers` array. */ properQualifiers?: Record<PropertyKey, QualifierInfo[]>;To ensure consistency across the codebase, please verify the following:
- Check if any documentation needs to be updated to reflect this new property.
- Verify if any code that creates or consumes
ObjectInfoinstances needs to be updated to handle this new property.You can use the following script to find potential areas that might need updates:
Please review the results of this script to ensure all necessary updates are made.
✅ Verification successful
LGTM! The addition of the
properQualifiersproperty is optional and does not affect existing code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of ObjectInfo that might need updates # Search for ObjectInfo usages echo "Searching for ObjectInfo usages:" rg --type typescript "ObjectInfo" -C 3 # Search for places where objects with qualifiers are created echo "\nSearching for qualifier creations:" rg --type typescript "qualifiers.*:" -C 3Length of output: 310
Script:
#!/bin/bash # Description: Find usages of ObjectInfo that might need updates # Search for ObjectInfo usages in .ts and .tsx files echo "Searching for ObjectInfo usages:" rg "ObjectInfo" --glob "*.ts" --glob "*.tsx" -C 3 # Search for places where objects with qualifiers are created in .ts and .tsx files echo "\nSearching for qualifier creations:" rg "qualifiers.*:" --glob "*.ts" --glob "*.tsx" -C 3Length of output: 55790
plugin/tegg/test/MultiInstanceInjectMultiInstance.test.ts (1)
1-5: Imports look good and are relevant to the test file's purpose.The necessary testing libraries and application modules are correctly imported. The use of 'egg-mock' is appropriate for creating a mock application environment, and the specific imports of App2 and App modules indicate they will be the focus of the tests.
plugin/tegg/lib/EggModuleLoader.ts (2)
1-8: LGTM: New import added correctlyThe addition of
LoadUnitMultiInstanceProtoHookto the import statement is correct and well-placed. This import suggests the introduction of multi-instance functionality to the module loader, which aligns with the PR objectives.
54-54: Please provide more context and consider error handlingThe addition of
LoadUnitMultiInstanceProtoHook.setAllClassList(appGraph.getClazzList());introduces multi-instance functionality to the module loader. While the placement afterappGraph.sort();seems logical, please consider the following:
- Can you elaborate on the impact of this change on the rest of the application?
- Consider adding error handling or validation around this call.
- It would be helpful to add a comment explaining the purpose and necessity of this new functionality.
To ensure this change doesn't introduce unintended side effects, please run the following script:
standalone/standalone/src/EggModuleLoader.ts (1)
1-7: LGTM: New import added correctlyThe new import for
LoadUnitMultiInstanceProtoHookis correctly added to the existing import statement from '@eggjs/tegg-metadata'. This import is necessary for the new functionality introduced in the file.plugin/tegg/app.ts (2)
Line range hint
1-71: Summary of review and next stepsThe addition of
LoadUnitMultiInstanceProtoHook.clear()in thebeforeClosemethod is a positive step towards proper resource management for the multi-instance injection feature. However, there are a few areas for potential improvement:
- Error handling: Consider wrapping the clear call in a try-catch block.
- Code organization: The
beforeClosemethod could benefit from refactoring for improved maintainability.- System-wide impact: The implications of the static clear method call need to be verified.
Next steps:
- Implement the suggested error handling improvement.
- Consider refactoring the
beforeClosemethod as proposed.- Run the provided verification script and analyze its results to ensure the change doesn't have unintended consequences.
- Update documentation if necessary, especially regarding the usage and effects of
LoadUnitMultiInstanceProtoHook.clear().These improvements will enhance the robustness and maintainability of the code while ensuring the safe implementation of the multi-instance injection feature.
70-70: Verify the impact ofLoadUnitMultiInstanceProtoHook.clear()on the broader system.The static call to
LoadUnitMultiInstanceProtoHook.clear()suggests that it might affect global state. It's important to ensure that this doesn't cause unintended side effects, especially if multiple instances of theAppclass can exist simultaneously.To verify the impact and usage of this method, please run the following script:
This script will help us understand:
- If there are other places in the codebase calling this method.
- The implementation details of the
LoadUnitMultiInstanceProtoHookclass.- Any existing documentation or comments about the
clearmethod.Based on the results, we can better assess the potential impact of this change on the system and determine if any additional safeguards or documentation are needed.
core/metadata/test/AppGraph.test.ts (2)
7-10: LGTM: New imports added for multi-instance injection testThe new imports are correctly added and necessary for the new test case. They reflect the new directory structure for multi-instance injection tests.
Line range hint
1-76: Summary: Good addition of multi-instance injection test, with room for improvementThe changes to this file introduce a new test case for multi-instance injection, which is a valuable addition to the test suite. The new imports and the test case structure are well-implemented. However, the test case could be enhanced to provide more comprehensive coverage of the multi-instance behavior and to improve its explanatory value.
Recommendations:
- Implement the suggested improvements to the test case to verify multi-instance behavior more explicitly.
- Add comments to explain the expected behavior and module order.
- Consider adding additional test cases to cover edge cases or compare with single-instance behavior.
Overall, this is a good start in testing the new multi-instance injection feature, but with some enhancements, it could provide even better coverage and documentation of the expected behavior.
plugin/dal/lib/DataSource.ts (1)
3-3: LGTM: Import statements are correctly updated.The addition of
Injectto the import list and the new import forTransactionalAOPare consistent with the changes in the class implementation.Also applies to: 29-29
core/metadata/src/impl/LoadUnitMultiInstanceProtoHook.ts (1)
21-22: Verify compatibility of the updated 'preCreate' method signature with the 'LifecycleHook' interface.The
preCreatemethod signature has been changed to remove its parametersctxandloadUnit. Ensure that this modification aligns with theLifecycleHookinterface's contract and does not introduce implementation inconsistencies.Run the following script to check if the
LifecycleHookinterface requirespreCreateto have parameters:plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/foo/Secret.ts (1)
39-41: Clarify the purpose of appending'233'ingetSecretmethodThe
getSecretmethod appends'233'to the providedkey. If'233'is a placeholder or magic number, consider replacing it with a meaningful constant or adding documentation to explain its purpose.Is
'233'intended to represent a specific value or is it a placeholder? Please clarify its purpose or update the code accordingly.core/metadata/test/fixtures/modules/app-multi-inject-multi/app/modules/bar/BizManager.ts (2)
17-22: Proper use of custom qualifiers for dependency injectionThe
BizManagerQualifierfunction effectively creates a custom decorator to apply qualifiers to property injections, enhancing modularity and flexibility.
25-50: Well-structured MultiInstanceProto implementationThe
@MultiInstanceProtodecorator is well-implemented, dynamically creating instances based on the module configuration. The use ofgetObjectsto map clients to object instances with appropriate qualifiers demonstrates good use of the framework's features.core/metadata/src/model/AppGraph.ts (5)
11-11: Clarify the purpose ofproperQualifiersinInstanceClazzMetaThe addition of
properQualifiersto theInstanceClazzMetainterface enhances the tracking of qualifiers specific to properties. Ensure that this new property is consistently utilized throughout the codebase whereInstanceClazzMetainstances are handled.
69-69: InitializeproperQualifiersproperly in multi-instance contextIn the multi-instance prototype handling,
properQualifiersis initialized usinginfo.properQualifiers || {}. Confirm thatinfo.properQualifiersis always defined when expected, and that defaulting to an empty object{}does not lead to missing qualifiers in dependency resolution.
86-86: Ensure consistent initialization ofproperQualifiersWhen handling regular prototypes (non-multi-instance),
properQualifiersis initialized to an empty object. Verify that this aligns with the intended logic and that there are no scenarios whereproperQualifiersshould contain data for regular prototypes.
221-229:getClazzList()method correctly aggregates classesThe new
getClazzList()method in theAppGraphclass effectively collects all uniqueEggProtoImplClassinstances from the graph nodes. This implementation ensures that all classes are accessible without duplication.
249-250: Confirm handling ofproperQualifierswhen undefinedWhen constructing
properQualifiers, you use the optional chaining operator and nullish coalescing:...info.properQualifiers?.[injectObject.refName] ?? []Ensure that this correctly handles cases where
info.properQualifiersor the specificinjectObject.refNamedoes not exist, and that it does not lead to unexpected missing qualifiers.core/metadata/src/impl/ModuleLoadUnit.ts (3)
27-27: Import statement forLoadUnitMultiInstanceProtoHookis appropriateThe import of
LoadUnitMultiInstanceProtoHookis necessary and correctly references the module used for handling multi-instance prototypes.
Line range hint
138-163: Ensure type compatibility infindInjectNodemethodThe
findInjectNodemethod now returns aGraphNodethat can contain either aProtoNodeor aMultiInstanceProtoNode. Verify that all usages of this method properly handle both node types, and ensure that any methods called on these nodes are available and behave correctly for both classes.
274-283: Verify correct loading of multi-instance classesThe addition of classes from
LoadUnitMultiInstanceProtoHook.multiInstanceClazzSetrelies on the presence of aninstanceProperty. Ensure that all multi-instance prototypes are properly registered and that no required classes are omitted due to missing properties.
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/app/modules/app/App.ts
Show resolved
Hide resolved
plugin/tegg/test/fixtures/apps/app-multi-inject-multi/config/config.default.js
Show resolved
Hide resolved
| static clear() { | ||
| this.multiInstanceClazzSet.clear(); | ||
| } |
There was a problem hiding this comment.
Replace 'this' with class name in static method for clarity.
Similarly, in the clear method, replacing this with the class name makes it explicit and avoids confusion. Change this.multiInstanceClazzSet to LoadUnitMultiInstanceProtoHook.multiInstanceClazzSet.
Apply this diff to enhance clarity:
static clear() {
- this.multiInstanceClazzSet.clear();
+ LoadUnitMultiInstanceProtoHook.multiInstanceClazzSet.clear();
}📝 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 clear() { | |
| this.multiInstanceClazzSet.clear(); | |
| } | |
| static clear() { | |
| LoadUnitMultiInstanceProtoHook.multiInstanceClazzSet.clear(); | |
| } |
🧰 Tools
🪛 Biome
[error] 18-18: Using this in a static context can be confusing.
this refers to the class.
Unsafe fix: Use the class name instead.(lint/complexity/noThisInStatic)
| const appModuleNode = new ModuleNode({ | ||
| name: 'app', | ||
| path: path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app'), | ||
| }); | ||
| appModuleNode.addClazz(App); | ||
| graph.addNode(appModuleNode); | ||
|
|
||
| const app2ModuleNode = new ModuleNode({ | ||
| name: 'app2', | ||
| path: path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app2'), | ||
| }); | ||
| app2ModuleNode.addClazz(App2); | ||
| graph.addNode(app2ModuleNode); | ||
|
|
||
| const barOptionalModuleNode = new ModuleNode({ | ||
| name: 'bar', | ||
| path: path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/bar'), | ||
| }); | ||
| barOptionalModuleNode.addClazz(BizManager); | ||
| graph.addNode(barOptionalModuleNode); | ||
| const fooOptionalModuleNode = new ModuleNode({ | ||
| name: 'foo', | ||
| path: path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/foo'), | ||
| }); | ||
| fooOptionalModuleNode.addClazz(Secret); | ||
| graph.addNode(fooOptionalModuleNode); | ||
| graph.build(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated code when creating ModuleNode instances
The code for creating ModuleNode instances is repeated multiple times with similar structure. Refactoring this into a helper function can reduce code duplication and enhance maintainability.
Consider adding a helper function:
function createModuleNode(name: string, modulePath: string, clazz: any): ModuleNode {
const moduleNode = new ModuleNode({
name,
path: modulePath,
});
moduleNode.addClazz(clazz);
return moduleNode;
}Then, you can replace the repeated code with calls to this function:
- const appModuleNode = new ModuleNode({
- name: 'app',
- path: path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app'),
- });
- appModuleNode.addClazz(App);
- graph.addNode(appModuleNode);
+ const appModuleNode = createModuleNode(
+ 'app',
+ path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app'),
+ App
+ );
+ graph.addNode(appModuleNode);Apply similar changes for the other ModuleNode creations to streamline the code.
| class MultiInstanceProtoNode implements GraphNodeObj { | ||
| readonly clazz: EggProtoImplClass; | ||
| readonly name: EggPrototypeName; | ||
| readonly id: string; | ||
| readonly qualifiers: QualifierInfo[]; | ||
| readonly initType: ObjectInitTypeLike; | ||
| readonly unitPath: string; | ||
| readonly moduleName: string; | ||
|
|
||
| constructor(clazz: EggProtoImplClass, objName: EggPrototypeName, unitPath: string, moduleName: string) { | ||
| this.name = objName; | ||
| this.id = '' + (id++); | ||
| this.clazz = clazz; | ||
| this.qualifiers = QualifierUtil.getProtoQualifiers(clazz); | ||
| this.initType = PrototypeUtil.getInitType(clazz, { | ||
| unitPath, | ||
| moduleName, | ||
| })!; | ||
| this.unitPath = unitPath; | ||
| this.moduleName = moduleName; | ||
| } | ||
|
|
||
| verifyQualifiers(qualifiers: QualifierInfo[]): boolean { | ||
| const property = PrototypeUtil.getMultiInstanceProperty(this.clazz, { | ||
| unitPath: this.unitPath, | ||
| moduleName: this.moduleName, | ||
| }); | ||
| if (!property) { | ||
| return false; | ||
| } | ||
| for (const obj of property.objects) { | ||
| const selfQualifiers = [ | ||
| ...this.qualifiers, | ||
| ...obj.qualifiers, | ||
| ]; | ||
| if (this.verifyInstanceQualifiers(selfQualifiers, qualifiers)) { | ||
| return true; | ||
| } | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| verifyInstanceQualifiers(selfQualifiers: QualifierInfo[], qualifiers: QualifierInfo[]): boolean { | ||
| for (const qualifier of qualifiers) { | ||
| if (!selfQualifiers.find(t => t.attribute === qualifier.attribute && t.value === qualifier.value)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } | ||
|
|
||
| toString(): string { | ||
| return `${this.clazz.name}@${PrototypeUtil.getFilePath(this.clazz)}`; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider refactoring to reduce code duplication between ProtoNode and MultiInstanceProtoNode
The classes ProtoNode and MultiInstanceProtoNode share several properties and methods, such as clazz, name, id, qualifiers, initType, and the toString() method. To enhance maintainability and reduce code duplication, consider extracting the shared functionality into a base class or interface that both classes can extend or implement.
| const protoGraphNodes: GraphNode<ProtoNode | MultiInstanceProtoNode>[] = []; | ||
| for (const clazz of this.clazzList) { | ||
| const objNames = PrototypeUtil.getObjNames(clazz, { | ||
| unitPath: this.unitPath, | ||
| moduleName: this.name, | ||
| }); | ||
| for (const objName of objNames) { | ||
| protoGraphNodes.push(new GraphNode(new ProtoNode(clazz, objName, this.unitPath, this.name))); | ||
| if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | ||
| protoGraphNodes.push(new GraphNode(new MultiInstanceProtoNode(clazz, objName, this.unitPath, this.name))); | ||
| } else { | ||
| protoGraphNodes.push(new GraphNode(new ProtoNode(clazz, objName, this.unitPath, this.name))); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify node creation logic in the build method
In the build method, the logic for creating GraphNode instances checks whether each class is a multi-instance prototype to decide which node type to instantiate. Consider refactoring this section to reduce complexity, possibly by using a factory pattern or a common constructor that handles the differentiation internally.
| if (PrototypeUtil.isEggMultiInstancePrototype(node.val.clazz)) { | ||
| const property = PrototypeUtil.getMultiInstanceProperty(node.val.clazz, { | ||
| moduleName: this.name, | ||
| unitPath: this.unitPath, | ||
| }); | ||
| for (const objectInfo of property?.objects || []) { | ||
| const injectObjects = PrototypeUtil.getInjectObjects(node.val.clazz); | ||
| for (const injectObject of injectObjects) { | ||
| const qualifiers = [ | ||
| ...QualifierUtil.getProperQualifiers(node.val.clazz, injectObject.refName), | ||
| ...objectInfo.properQualifiers?.[injectObject.refName] ?? [], | ||
| ]; | ||
| const injectNode = this.findInjectNode(injectObject.objName, qualifiers, node.val.initType); | ||
| // If not found maybe in other module | ||
| if (injectNode) { | ||
| this.graph.addEdge(node, injectNode); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| const injectObjects = PrototypeUtil.getInjectObjects(node.val.clazz); | ||
| for (const injectObject of injectObjects) { | ||
| const qualifiers = QualifierUtil.getProperQualifiers(node.val.clazz, injectObject.refName); | ||
| const injectNode = this.findInjectNode(injectObject.objName, qualifiers, node.val.initType); | ||
| // If not found maybe in other module | ||
| if (injectNode) { | ||
| this.graph.addEdge(node, injectNode); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Eliminate code duplication when adding graph edges
The process of adding edges to the graph for dependency resolution is similar for both multi-instance prototypes and regular prototypes. To improve readability and reduce maintenance effort, consider refactoring the common logic into a shared helper function or method.
|
|
||
| private loadClazz() { | ||
| if (!this.clazzList) { | ||
| const clazzList = this.doLoadClazz(); | ||
| const protoGraph = new ModuleGraph(clazzList, this.unitPath, this.name); | ||
| protoGraph.sort(); | ||
| this.clazzList = protoGraph.clazzList; | ||
| } |
There was a problem hiding this comment.
Ensure thread safety in loadClazz method
The loadClazz method checks if this.clazzList is uninitialized before loading classes. If loadClazz can be called concurrently, consider adding synchronization mechanisms to prevent race conditions and ensure that this.clazzList is correctly initialized before use.
|
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
New Features
MultiInstanceProtoNode,BizManager,Secret,App,App2) to facilitate dependency injection and configuration management.Bug Fixes
Tests
Documentation