chore: use GlobalGraph to replace AppGraph/ModuleGraph #242
Conversation
WalkthroughThe pull request introduces changes across multiple files, focusing on the management of module loading and graph structures. Updates include modifications to the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
主要目的是将 MultiInstanceProto 的复杂性转移到 Loader 和 Graph 中，减少对其他部分的修改。 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 43
🧹 Outside diff range and nitpick comments (29)
core/types/metadata/enum/ProtoDescriptorType.ts (1)
1-3: LGTM! Consider future extensibility.The introduction of the
ProtoDescriptorTypeenum is a good addition. It provides type safety and improves code readability. The naming convention follows TypeScript best practices.Given that there's currently only one member (
CLASS), consider if there are plans to extend this enum in the future. If so, it might be helpful to add a comment indicating potential future additions or the purpose of this enum in the broader context of the MultiInstanceProto transition mentioned in the PR objectives.Would you like assistance in adding documentation comments to explain the purpose and potential future extensions of this enum?
core/metadata/src/model/graph/ProtoSelector.ts (1)
3-7: LGTM: Well-structured interface. Consider adding JSDoc comments.The
ProtoSelectorContextinterface is well-defined with appropriate types for its properties. It provides a clear structure for representing the context of a proto selector.Consider adding JSDoc comments to describe the purpose of the interface and each property. This would enhance code readability and provide better documentation for developers using this interface. For example:
/** * Represents the context for a proto selector. */ export interface ProtoSelectorContext { /** The name of the proto selector. */ name: PropertyKey; /** An array of qualifier information. */ qualifiers: QualifierInfo[]; /** The name of the module. */ moduleName: string; }core/types/common/ModuleConfig.ts (2)
5-5: LGTM: New property aligns with PR objectivesThe addition of the optional
loaderTypeproperty to theModuleReferenceinterface aligns well with the PR's objective of transitioning complexity into the Loader component. This change allows for more flexible and specific loader configurations for different module types.However, to ensure completeness:
- Consider adding a comment explaining the purpose and expected values of
loaderType.- Update any relevant documentation to reflect this new property.
- Ensure that any code using
ModuleReferenceis updated to handle this new property if necessary.Would you like me to suggest a comment for the
loaderTypeproperty?
Line range hint
1-38: Overall file structure remains consistentThe addition of the
loaderTypeproperty to theModuleReferenceinterface is consistent with the overall structure and purpose of this file. The file continues to define various interfaces related to module configuration, and the new property enhances the flexibility of module references without disrupting existing definitions.However, to maintain clarity and consistency:
- Consider adding comments to other interfaces in this file, explaining their purpose and usage.
- Ensure that the
ModuleConfiginterface (currently empty) is either populated or removed if not needed.- Review if any other interfaces might benefit from additional properties related to loader types, similar to the change made to
ModuleReference.Would you like suggestions for comments on the other interfaces or a review of the
ModuleConfiginterface?core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1)
13-16: LGTM! Consider adding a comment for theunitPathproperty.The new
MultiInstancePrototypeGetObjectsContextinterface is well-structured and aligns with the PR objectives. It provides clear context for multi-instance prototypes.To improve consistency and clarity, consider adding a comment for the
unitPathproperty, similar to the one provided formoduleName. For example:export interface MultiInstancePrototypeGetObjectsContext { // instance module, multi instance proto used in moduleName: string; // path of the unit within the module unitPath: string; }core/core-decorator/src/util/QualifierUtil.ts (1)
69-82: Summary of QualifierUtil changesThe additions to the
QualifierUtilclass align well with the PR objectives of shifting complexity into the Graph components. The newmatchQualifiersandequalQualifiersmethods provide useful functionality for comparing and matching qualifiers, which will likely be beneficial in implementing the GlobalGraph replacement for AppGraph/ModuleGraph.However, there are opportunities for optimization and clarity improvements in both methods:
- The
matchQualifiersmethod could be renamed and optimized for better performance.- The
equalQualifiersmethod could be optimized to combine the length check and subset check.These improvements would enhance the efficiency and readability of the code, contributing to the overall goal of minimizing modifications to other parts of the codebase while effectively handling the complexity of MultiInstanceProto.
As you continue to implement the GlobalGraph replacement, consider the following:
- Ensure that these methods are thoroughly tested, especially with edge cases and large datasets.
- Document the expected behavior and performance characteristics of these methods for future maintainers.
- Consider how these methods will be used in the context of loading protocol descriptions and enforcing strict mode verification.
These considerations will help ensure that the new GlobalGraph implementation is robust, efficient, and maintainable.
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
86-105: New methodcreateProtoByDescriptorlooks good overallThe implementation is consistent with the existing
createProtomethod and enhances the factory's functionality. It correctly uses theClassProtoDescriptorto create the prototype.However, there are a few points to consider:
The TODO comment on line 97 suggests a potential resource management issue:
// TODO release egg prototypeConsider implementing the release mechanism or add more context about when and how this should be addressed.
The use of
thisin a static context (e.g., line 87) is valid but might be confusing:const creator = this.getPrototypeCreator(protoDescriptor.protoImplType);Consider using the class name
EggPrototypeCreatorFactoryinstead ofthisfor clarity.Adding JSDoc comments would improve the method's documentation:
/** * Creates a prototype using a ClassProtoDescriptor. * @param protoDescriptor - The descriptor for the prototype. * @param loadUnit - The load unit for the prototype. * @returns A promise that resolves to the created EggPrototype. * @throws Error if no creator is found for the given protoImplType. */ static async createProtoByDescriptor(protoDescriptor: ClassProtoDescriptor, loadUnit: LoadUnit): Promise<EggPrototype> { // ... existing implementation ... }Would you like me to provide a revised implementation addressing these points?
🧰 Tools
🪛 Biome
[error] 87-87: 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/metadata/test/GlobalGraph.test.ts (3)
15-39: Consider enhancing the assertion for more specific validation.The test case correctly sets up the GlobalGraph with optional dependencies and verifies that unused optional modules are not included. However, the assertion could be more specific to ensure the correct modules are included.
Consider modifying the assertion to check not only the length but also the specific modules included:
assert.deepStrictEqual(graph.moduleConfigList.map(config => config.name), ['root', 'used']);This change would provide a more robust verification of the expected behavior.
83-102: Consider improving null handling in the assertion.The test case effectively verifies the sorting of extended classes. However, the use of non-null assertion (!) in the assertion could be improved for better type safety.
Consider modifying the assertion to handle potential null values more safely:
assert(moduleProtoDescriptors, 'moduleProtoDescriptors should not be null'); assert.deepStrictEqual(moduleProtoDescriptors.map(t => t.name), [ 'logger', 'base', 'foo', ]);This change ensures that the test fails with a clear message if
moduleProtoDescriptorsis unexpectedly null.
104-124: LGTM with a minor suggestion for null handling.This test case effectively verifies the sorting of constructor-extended classes. The setup and assertion are well-implemented.
Similar to the previous test case, consider improving the null handling in the assertion:
const moduleProtoDescriptors = graph.moduleProtoDescriptorMap.get('extendsModule'); assert(moduleProtoDescriptors, 'moduleProtoDescriptors should not be null'); assert.deepStrictEqual(moduleProtoDescriptors.map(t => t.name), [ 'logger', 'bar', 'constructorBase', 'fooConstructor', 'fooConstructorLogger', ]);This change ensures type safety and provides a clear error message if
moduleProtoDescriptorsis unexpectedly null.core/types/metadata/model/ProtoDescriptor.ts (2)
30-30: Consider renaming theequalmethod toequalsfor clarity.The method is currently named
equal:equal(protoDescriptor: ProtoDescriptor): boolean;Renaming it to
equalsmay improve readability and align with common naming conventions:equals(protoDescriptor: ProtoDescriptor): boolean;
12-31: Add documentation comments to interface properties and methods.Including JSDoc comments for the
ProtoDescriptorinterface will enhance code maintainability and assist other developers in understanding the purpose of each property and method.For example:
export interface ProtoDescriptor extends EggPrototypeInfo { /** * The name of the prototype. */ name: PropertyKey; // Additional property comments... }core/metadata/src/model/graph/GlobalModuleNode.ts (3)
5-9: Consider adding documentation to theGlobalModuleNodeOptionsinterface.Adding JSDoc comments for the properties of the
GlobalModuleNodeOptionsinterface can enhance code readability and assist other developers in understanding the purpose of each property.
11-23: Add documentation comments toModuleDependencyMetaclass and its methods.Including descriptive comments for the
ModuleDependencyMetaclass, its constructor, and methods likeequalandtoStringwill improve maintainability and readability.
25-47: Document theGlobalModuleNodeclass and its members.Providing documentation for the
GlobalModuleNodeclass, its constructor, and methods will help others understand its functionality and usage.core/loader/src/LoaderFactory.ts (1)
26-26: Handle the case whenmoduleReference.loaderTypeis undefined more explicitly.Currently, it defaults to
EggLoadUnitType.MODULEifloaderTypeis undefined. Ensure this behavior is intentional and documented.Add a comment to clarify the defaulting mechanism:
// Default to MODULE type if loaderType is not provided const loader = LoaderFactory.createLoader( moduleReference.path, moduleReference.loaderType || EggLoadUnitType.MODULE );core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1)
6-57: Consider adding documentation comments to the class and its methodsAdding JSDoc comments to the
GlobalModuleNodeBuilderclass and its methods would enhance code readability and maintainability by providing clear descriptions of their purposes and usages.core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts (1)
10-10: Add documentation comments for the interface and abstract classAdding JSDoc comments to the
AbstractProtoDescriptorOptionsinterface and theAbstractProtoDescriptorclass will improve code readability and maintainability by providing context and usage information.Consider adding comments like the following:
/** * Options for creating an AbstractProtoDescriptor. */ export interface AbstractProtoDescriptorOptions { // ... } /** * Abstract class representing a protocol descriptor. */ export abstract class AbstractProtoDescriptor implements ProtoDescriptor { // ... }Also applies to: 25-25
standalone/standalone/src/EggModuleLoader.ts (1)
24-24: Use 'this.globalGraph' instead of 'GlobalGraph.instance' to avoid non-null assertionSince
this.globalGraphis already assigned, you can use it directly to accessmoduleConfigListwithout accessing the singletonGlobalGraph.instanceand using the non-null assertion operator.You can apply the following change:
- const moduleConfigList = GlobalGraph.instance!.moduleConfigList; + const moduleConfigList = this.globalGraph.moduleConfigList;core/metadata/test/fixtures/LoaderUtil.ts (1)
3-6: Check for unnecessary or unused importsThe import statements at lines 3-6 add new dependencies to the file. Please verify that all imported modules are necessary and used within the code. Removing unused imports can improve code clarity and reduce potential maintenance overhead.
core/common-util/src/Graph.ts (2)
Line range hint
144-161: Handle potential cases wherenodes.indexOf(node)returns-1inaccessNode.In the
accessNodemethod, the lineconst index = nodes.indexOf(node);assumes that the node exists in thenodesarray. However, ifnodeis not found,indexOfwill return-1, leading to incorrect indexing in theaccessedarray and potential runtime errors. To ensure robustness, add a check forindex !== -1before proceeding.Apply this diff to address the issue:
accessNode(node: GraphNode<T, M>, nodes: Array<GraphNode<T, M>>, accessed: boolean[], res: Array<GraphNode<T, M>>) { const index = nodes.indexOf(node); + if (index === -1) { + // Node not found in nodes array; handle the error or return + return; + } if (accessed[index]) { return; } // Rest of the method... }
Line range hint
169-180: Document limitations and behaviors in thesortmethod.The
sortmethod includes a note that the sort result is not stable and that graphs with loops cannot be sorted:// notice: // 1. sort result is not stable // 2. graph with loop can not be sortConsider enhancing the documentation comments to explain why the sort is unstable and how it handles graphs with cycles. Providing more detailed comments or throwing errors when attempting to sort graphs with loops can improve code clarity and usability.
core/metadata/src/model/graph/GlobalGraph.ts (2)
155-159: Enhance error messages for recursive dependencies in#sortModuleWhen a recursive dependency loop is detected in
#sortModule, the error message could include more details about the loop path to aid debugging.Apply the following diff to improve the error message:
-throw new Error('module has recursive deps: ' + loopPath); +throw new Error('Module has recursive dependencies: ' + loopPath.join(' -> '));
196-200: Avoid unnecessary side effects in thecreatemethodThe call to
ModuleDescriptorDumper.dumpPath(moduleDescriptor);may produce side effects like logging or modifying the module descriptor. If not intended, consider removing or safeguarding this call to prevent potential issues.core/metadata/src/impl/ModuleLoadUnit.ts (3)
21-21: Import Ordering: Follow Standard Import Ordering GuidelinesConsider organizing your import statements to enhance readability. Typically, imports are ordered in the following sequence: built-in modules, external modules, internal modules, and relative paths.
194-194: Clean Up: Remove Commented-Out CodeThe line
// private loader: Loader;is commented out. If theloaderproperty is no longer needed, consider removing it entirely to keep the codebase clean.Apply this diff to remove the commented code:
- // private loader: Loader; private protoMap: Map<EggPrototypeName, EggPrototype[]> = new Map();
217-217: Address TODO Comment: Support All Proto DescriptorsThe TODO comment indicates that
ModuleLoadUnitshould support all proto descriptors, but currently, it filters only class proto descriptors.Would you like assistance in implementing support for all proto descriptors? I can help draft the necessary code changes or open a GitHub issue to track this task.
core/core-decorator/src/util/PrototypeUtil.ts (2)
149-152: Provide detailed documentation forgetStaticMultiInstancePropertyThe method
getStaticMultiInstancePropertylacks sufficient documentation to explain its purpose and usage. Enhancing the documentation will improve maintainability and clarity.Consider updating the documentation as follows:
- /** - * get class property - * @param {EggProtoImplClass} clazz - - */ + /** + * Retrieve the static multi-instance prototype property of a class. + * @param {EggProtoImplClass} clazz - The class from which to retrieve the property. + * @returns {EggMultiInstancePrototypeInfo | undefined} - The static multi-instance prototype info, or undefined if not found. + */
160-164: Enhance documentation forgetDynamicMultiInstancePropertyThe current documentation for
getDynamicMultiInstancePropertyis insufficient. Providing a clearer description will help future developers understand its functionality.Please update the documentation as shown:
- /** - * get class property - * @param {EggProtoImplClass} clazz - - * @param {MultiInstancePrototypeGetObjectsContext} ctx - - */ + /** + * Retrieve the dynamic multi-instance prototype property of a class based on the provided context. + * @param {EggProtoImplClass} clazz - The class from which to retrieve the property. + * @param {MultiInstancePrototypeGetObjectsContext} ctx - The context used to obtain the objects. + * @returns {EggMultiInstancePrototypeInfo | undefined} - The dynamic multi-instance prototype info, or undefined if not found. + */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (30)
- core/common-util/src/Graph.ts (6 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (3 hunks)
- core/core-decorator/src/util/QualifierUtil.ts (1 hunks)
- core/loader/src/LoaderFactory.ts (2 hunks)
- core/metadata/index.ts (1 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (0 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (6 hunks)
- core/metadata/src/model/ModuleDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptorHelper.ts (1 hunks)
- core/metadata/src/model/graph/GlobalGraph.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNode.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1 hunks)
- core/metadata/src/model/graph/ProtoNode.ts (1 hunks)
- core/metadata/src/model/graph/ProtoSelector.ts (1 hunks)
- core/metadata/test/GlobalGraph.test.ts (1 hunks)
- core/metadata/test/LoadUnit.test.ts (9 hunks)
- core/metadata/test/fixtures/LoaderUtil.ts (2 hunks)
- core/types/common/ModuleConfig.ts (1 hunks)
- core/types/core-decorator/enum/MultiInstanceType.ts (1 hunks)
- core/types/core-decorator/index.ts (1 hunks)
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/types/metadata/enum/ProtoDescriptorType.ts (1 hunks)
- core/types/metadata/index.ts (1 hunks)
- core/types/metadata/model/Loader.ts (1 hunks)
- core/types/metadata/model/ProtoDescriptor.ts (1 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (2 hunks)
- standalone/standalone/src/EggModuleLoader.ts (2 hunks)
💤 Files with no reviewable changes (1)
- core/metadata/src/impl/EggPrototypeBuilder.ts
✅ Files skipped from review due to trivial changes (3)
- core/types/core-decorator/enum/MultiInstanceType.ts
- core/types/metadata/index.ts
- core/types/metadata/model/Loader.ts
🧰 Additional context used
🪛 Biome
core/metadata/src/factory/EggPrototypeCreatorFactory.ts
[error] 87-87: 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/metadata/src/model/ModuleDescriptor.ts
[error] 16-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/src/model/ProtoDescriptorHelper.ts
[error] 20-203: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/test/LoadUnit.test.ts
[error] 22-22: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
standalone/standalone/src/EggModuleLoader.ts
[error] 37-37: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (22)
core/metadata/src/model/graph/ProtoSelector.ts (2)
1-1: LGTM: Import statement is appropriate.The import of
QualifierInfofrom '@eggjs/tegg-types' is correctly used in the interface definition, promoting modular design.
1-7: Overall assessment: Well-structured and aligned with PR objectives.This new file introduces a clear and focused
ProtoSelectorContextinterface, which aligns well with the PR objective of transitioning complexity into the Graph component. The code is concise, uses appropriate types, and follows good practices for modular design.To further improve the code:
- Consider adding JSDoc comments as suggested earlier.
- Ensure that this new interface is properly integrated with other parts of the codebase, particularly the Loader component mentioned in the PR objectives.
To verify the integration of this new interface, you can run the following script:
This script will help ensure that the new interface is being used appropriately across the codebase.
✅ Verification successful
Verification Successful:
ProtoSelectorContextis Properly Integrated.The
ProtoSelectorContextinterface is correctly imported and utilized in bothProtoNode.tsandProtoDescriptorHelper.ts, ensuring its integration aligns with the PR objectives of transitioning complexity into the Graph component. The implementation is consistent and follows best practices for modular design.To further enhance the code quality:
- Add JSDoc Comments: Enhance documentation by adding JSDoc comments to the
ProtoSelectorContextinterface and its implementations.- Review Integration with Loader Component: Ensure that the
Loadercomponent, as mentioned in the PR objectives, effectively integrates with the newProtoSelectorContextinterface.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of ProtoSelectorContext in other files # Search for imports of ProtoSelectorContext echo "Files importing ProtoSelectorContext:" rg --type typescript "import.*ProtoSelectorContext.*from.*ProtoSelector" --glob '!core/metadata/src/model/graph/ProtoSelector.ts' # Search for usage of ProtoSelectorContext echo "\nUsage of ProtoSelectorContext:" rg --type typescript "ProtoSelectorContext" --glob '!core/metadata/src/model/graph/ProtoSelector.ts'Length of output: 462
Script:
#!/bin/bash # Description: Check for usage of ProtoSelectorContext in other files using file extensions # Search for imports of ProtoSelectorContext echo "Files importing ProtoSelectorContext:" rg "import.*ProtoSelectorContext.*from.*ProtoSelector" --glob '!core/metadata/src/model/graph/ProtoSelector.ts' --glob '*.ts' --glob '*.tsx' # Search for usage of ProtoSelectorContext echo "\nUsage of ProtoSelectorContext:" rg "ProtoSelectorContext" --glob '!core/metadata/src/model/graph/ProtoSelector.ts' --glob '*.ts' --glob '*.tsx'Length of output: 1148
core/types/common/ModuleConfig.ts (1)
Line range hint
1-5: Verify usage of ModuleReference across the codebaseWhile the addition of the optional
loaderTypeproperty toModuleReferenceis a minimal change, it's important to ensure that it's properly utilized and doesn't cause any unintended side effects.Let's verify the usage of
ModuleReferenceacross the codebase:Please review the results to ensure that:
- All relevant files using
ModuleReferenceare aware of the new property.- Any code that might benefit from using
loaderTypeis updated accordingly.- There are no conflicts or inconsistencies introduced by this change.
✅ Verification successful
Verification of
loaderTypeAddition inModuleReference
- The optional
loaderTypeproperty is properly utilized incore/loader/src/LoaderFactory.ts:
- It sets the loader type with a fallback to
EggLoadUnitType.MODULE.- No other usages of
loaderTypefound, ensuring no unintended side effects.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of ModuleReference and potential impacts of the new loaderType property # Search for imports of ModuleReference echo "Imports of ModuleReference:" rg --type typescript "import.*ModuleReference" -g '!core/types/common/ModuleConfig.ts' # Search for usage of ModuleReference echo "\nUsage of ModuleReference:" rg --type typescript "ModuleReference" -g '!core/types/common/ModuleConfig.ts' # Search for potential usage of loaderType property echo "\nPotential usage of loaderType property:" rg --type typescript "loaderType" -g '!core/types/common/ModuleConfig.ts'Length of output: 569
Script:
#!/bin/bash # Description: Check usage of ModuleReference and potential impacts of the new loaderType property # Search for imports of ModuleReference echo "Imports of ModuleReference:" rg --type ts "import.*ModuleReference" -g '!core/types/common/ModuleConfig.ts' # Search for usage of ModuleReference echo "\nUsage of ModuleReference:" rg --type ts "ModuleReference" -g '!core/types/common/ModuleConfig.ts' # Search for potential usage of loaderType property echo "\nPotential usage of loaderType property:" rg --type ts "loaderType" -g '!core/types/common/ModuleConfig.ts'Length of output: 8940
core/metadata/src/model/ModuleDescriptor.ts (2)
1-5: LGTM: Imports and constant declaration look good.The imports are appropriate for the file's purpose, and using Node.js core modules ensures good compatibility. The
DUMP_PATHconstant allows for flexible configuration via an environment variable, which is a good practice for customization.
7-14: LGTM: ModuleDescriptor interface is well-defined.The
ModuleDescriptorinterface provides a clear and comprehensive structure for module descriptors. It appropriately uses optional properties and imported types, ensuring type consistency and flexibility in usage.core/metadata/index.ts (1)
12-19: LGTM! New exports align with PR objectives.The addition of these new exports aligns well with the PR objective of transitioning complexity into Loader and Graph components. The exports are organized logically and seem to provide a comprehensive set of tools for working with graphs and descriptors.
To ensure these changes don't introduce unintended side effects, please run the following script to check for any existing usage of these newly exported entities:
Don't forget to update the documentation to reflect these new exports and their usage. Also, ensure that appropriate tests are added or updated to cover the new functionality provided by these exports.
✅ Verification successful
Verified: All new exports are actively used and correctly integrated.
The shell script results confirm that the newly exported entities are utilized throughout the codebase, ensuring that their addition aligns with the PR objectives without introducing any issues related to export removals.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for existing usage of newly exported entities # Test: Search for usage of new exports for entity in GlobalGraph GlobalModuleNode GlobalModuleNodeBuilder ProtoNode ProtoSelector AbstractProtoDescriptor ClassProtoDescriptor ModuleDescriptor do echo "Checking usage of $entity:" rg --type typescript "\b$entity\b" --glob '!core/metadata/index.ts' echo "---" doneLength of output: 2928
Script:
#!/bin/bash # Description: Check for existing usage of newly exported entities # Test: Search for usage of new exports in TypeScript files for entity in GlobalGraph GlobalModuleNode GlobalModuleNodeBuilder ProtoNode ProtoSelector AbstractProtoDescriptor ClassProtoDescriptor ModuleDescriptor do echo "Checking usage of $entity:" rg "\b$entity\b" --glob '**/*.ts' --glob '**/*.tsx' --glob '!core/metadata/index.ts' echo "---" doneLength of output: 12779
core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (2)
Line range hint
1-69: Overall, the changes look good and align with the PR objectives.The modifications to this file are minimal and focused, introducing a new context interface and updating the
getObjectsmethod signature. These changes appear to be a solid first step in transitioning the complexity of MultiInstanceProto into the Loader and Graph components.However, please note that the PR checklist indicates some incomplete tasks:
- Ensure that
npm testpasses.- Include tests or benchmarks for these changes.
- Update documentation as necessary.
To address these points, please run the following verification steps:
#!/bin/bash # Description: Verify test status, test coverage, and documentation updates # Run npm test echo "Running npm test:" npm test # Check for new or updated test files echo "Checking for new or updated test files:" git diff --name-only | grep -E "test|spec|__tests__" # Check for documentation updates echo "Checking for documentation updates:" git diff --name-only | grep -E "README|docs|\.md$"If any of these checks fail or return empty results, please address them before merging the PR.
Line range hint
69-69: LGTM! Verify implementations ofEggMultiInstanceCallbackPrototypeInfo.The updated
getObjectsmethod signature improves type safety and aligns with the PR objectives. It now uses the specificMultiInstancePrototypeGetObjectsContexttype instead ofany.To ensure consistency across the codebase, please verify all implementations of
EggMultiInstanceCallbackPrototypeInfoto confirm they've been updated to use the new context type. Run the following script to find potential implementations:✅ Verification successful
Verified: No existing implementations of
EggMultiInstanceCallbackPrototypeInfowere found. The updatedgetObjectsmethod signature improves type safety without affecting current code. No further action is required.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find implementations of EggMultiInstanceCallbackPrototypeInfo # Search for class declarations that implement EggMultiInstanceCallbackPrototypeInfo echo "Searching for implementations of EggMultiInstanceCallbackPrototypeInfo:" ast-grep --lang typescript --pattern 'class $_ implements $$$EggMultiInstanceCallbackPrototypeInfo$$$' # Search for interface declarations that extend EggMultiInstanceCallbackPrototypeInfo echo "Searching for interfaces that extend EggMultiInstanceCallbackPrototypeInfo:" ast-grep --lang typescript --pattern 'interface $_ extends $$$EggMultiInstanceCallbackPrototypeInfo$$$'Length of output: 404
Script:
#!/bin/bash # Description: Alternative method to find implementations of EggMultiInstanceCallbackPrototypeInfo using ripgrep # Search for class declarations that implement EggMultiInstanceCallbackPrototypeInfo echo "Searching for classes implementing EggMultiInstanceCallbackPrototypeInfo using ripgrep:" rg 'class\s+\w+\s+implements\s+EggMultiInstanceCallbackPrototypeInfo' --files-with-matches # Search for interface declarations that extend EggMultiInstanceCallbackPrototypeInfo echo "Searching for interfaces extending EggMultiInstanceCallbackPrototypeInfo using ripgrep:" rg 'interface\s+\w+\s+extends\s+EggMultiInstanceCallbackPrototypeInfo' --files-with-matchesLength of output: 556
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
11-11: LGTM: New import added for ClassProtoDescriptorThe import for
ClassProtoDescriptoris correctly added and necessary for the new method's parameter type.core/metadata/test/GlobalGraph.test.ts (3)
1-12: LGTM: Import statements are well-organized and relevant.The import statements are logically organized and include all necessary modules for the test cases. No unused imports are apparent.
41-81: LGTM: Comprehensive test for multi-instance injection.This test case effectively verifies the behavior of GlobalGraph with multiple instances and applications. The setup is thorough, and the assertion correctly checks both the presence and order of the expected modules.
1-125: Overall, excellent test coverage for GlobalGraph.This test suite provides comprehensive coverage for the GlobalGraph class, including tests for optional dependencies, multi-instance injection, and class extension sorting. The tests are well-structured, use appropriate assertions, and cover the main functionalities of the GlobalGraph class.
A few minor suggestions have been made to improve null handling and assertion specificity, but these are relatively small enhancements to an already solid test suite.
core/metadata/src/model/graph/ProtoNode.ts (2)
6-24: TheProtoDependencyMetaclass is well-implementedThe class correctly implements the
EdgeMetainterface. TheequalandtoStringmethods are appropriately defined and function as expected.
26-52: TheProtoNodeclass is well-structured and correctly implementsGraphNodeObjApart from the issue noted in
createProtoId, the class properties and methods are properly defined and align with best practices.core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1)
24-24:⚠️ Potential issueFix typo: Assign to
clazzNameinstead ofclassNameIn the constructor, the property assignment should be to
clazzNameto match the declared property name.Apply this diff to correct the typo:
- this.className = this.clazz.name; + this.clazzName = this.clazz.name;Likely invalid or redundant comment.
core/loader/src/LoaderFactory.ts (2)
34-34:⚠️ Potential issueOptional property
moduleReference.optionalmay be undefined.If
optionalis not a mandatory field inModuleReference, ensure that the code handlesundefinedcases appropriately.Verify whether
optionalhas a default value or needs a fallback:optional: moduleReference.optional ?? false,If
optionalshould default tofalsewhen not provided, apply the modification above.
32-32: EnsuremultiInstanceClazzListis correctly assigned to eachModuleDescriptor.After moving
multiInstanceClazzListinside the loop, make sure it is properly assigned to theresobject for each module.Verify that
multiInstanceClazzListis unique per module:core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1)
37-47:build()method implementation is correctThe
build()method accurately constructs aGlobalModuleNode, adds all prototypes, and returns the node as expected.core/metadata/test/fixtures/LoaderUtil.ts (1)
48-48: Verify reassignment ofGlobalGraph.instanceis safeAt line 48,
GlobalGraph.instanceis reassigned with a newGlobalGraph()instance. IfGlobalGraph.instanceis used elsewhere as a singleton, reassigning it might have unintended side effects.Ensure that reassigning
GlobalGraph.instanceis appropriate in this context. If this code is intended for testing purposes and isolation, consider documenting this to prevent confusion.core/common-util/src/Graph.ts (1)
109-117: Clarify behavior when metadata is undefined infindToNode.In the
findToNodemethod, you check for matching edges based on metadata:for (const { node: toNode, meta: edgeMeta } of node.toNodeMap.values()) { if (edgeMeta && meta.equal(edgeMeta)) { return toNode; } }If
edgeMetais undefined, the condition skips that edge. However, if it's possible that edges can exist without metadata (i.e.,metais optional), you might want to define howfindToNodeshould behave in such cases. Should it return edges without metadata whenmetais not provided? Clarifying this behavior can prevent confusion and potential bugs.To verify the usage of
findToNode, run the following script to find calls wheremetamight be undefined:This script searches for calls to
findToNodewith only one argument, which could indicate cases wheremetais omitted.✅ Verification successful
Verification Complete: All calls to
findToNodeinclude themetaargument. No instances were found wheremetais undefined.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of findToNode where meta could be undefined. # Search for findToNode calls without meta argument. rg --type typescript 'findToNode\(\s*[^,]+?\s*\)'Length of output: 86
Script:
#!/bin/bash # Description: Find usages of findToNode where meta could be undefined. # Search for findToNode calls without meta argument in TypeScript files. rg 'findToNode\(\s*[^,]+?\s*\)' --glob '*.ts' --glob '*.tsx'Length of output: 209
core/metadata/src/model/graph/GlobalGraph.ts (1)
103-145:⚠️ Potential issueHandle potential infinite recursion in
#findDependencyProtoNodeThe method
#findDependencyProtoNoderecursively calls itself whenloadUnitQualifieris not found (lines 133-143). Ensure there is a base case that prevents infinite recursion and that the recursion will terminate under all circumstances.Here's a script to analyze the recursive calls:
✅ Verification successful
Infinite Recursion Prevention Verified
The method
#findDependencyProtoNodeincludes a base case by adding aLoadUnitNameQualifierAttributewhenloadUnitQualifieris not found. This ensures that subsequent recursive calls will terminate, preventing infinite recursion.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Analyze recursive calls in #findDependencyProtoNode # Find all recursive calls to #findDependencyProtoNode within the file rg --type typescript -A 3 -B 3 '#findDependencyProtoNode.*\(' core/metadata/src/model/graph/GlobalGraph.tsLength of output: 143
Script:
#!/bin/bash # Description: Analyze recursive calls in #findDependencyProtoNode by searching .ts files # Find all recursive calls to #findDependencyProtoNode within TypeScript files rg '#findDependencyProtoNode\s*\(' --type-add 'ts:*.ts' --type ts -A 3 -B 3 core/metadata/src/model/graph/GlobalGraph.tsLength of output: 1080
core/metadata/src/impl/ModuleLoadUnit.ts (1)
298-298: 🛠️ Refactor suggestionUpdate Comment: Remove Unnecessary ESLint Disable Directive
The ESLint disable directive
@typescript-eslint/no-var-requiresis not needed if you convert therequirestatement to animportstatement.Apply this diff to use an
importstatement:-// eslint-disable-next-line @typescript-eslint/no-var-requires -const pkg = require(pkgPath); +import * as pkg from pkgPath;Alternatively, if dynamic importing is necessary, ensure that the ESLint directive is correctly applied.
Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (8)
core/test-util/TestLoader.ts (1)
Line range hint
1-33: Alignment with PR objectives and potential improvementsThe current changes in this file don't seem to directly address the main PR objective of transitioning the complexity of MultiInstanceProto into the Loader and Graph components. Consider the following points:
The TestLoader implementation doesn't show any significant changes related to handling MultiInstanceProto complexity.
There might be opportunities to enhance this Loader implementation to better support the PR objectives. For example:
- Adding support for loading protocol descriptions instead of adding protocols at runtime.
- Implementing strict mode verification for protocol existence before booting.
- Adding capabilities to support dump/restore functionality for GlobalGraph.
The PR checklist indicates that some tasks are incomplete (e.g., ensuring
npm testpasses, including tests or benchmarks, updating documentation). These might be related to pending changes in this file.Consider revisiting this file to ensure it fully supports the PR objectives. You might want to:
- Evaluate if any changes are needed in the
loadmethod to handle MultiInstanceProto complexity.- Add methods or properties to support new functionalities mentioned in the PR description.
- Update or add tests to cover any new functionality.
- Add comments or documentation to explain any new complexity handling mechanisms.
If you need any assistance in implementing these changes or have any questions about how to proceed, please don't hesitate to ask.
core/runtime/test/util.ts (1)
11-13: LGTM: New conditional block for building the global graph.The new conditional block aligns with the PR objective of shifting complexity into the Loader and Graph components. It correctly uses the new
buildGraphparameter to control whenLoaderUtil.buildGlobalGraphis called.Consider adding a brief comment explaining the purpose of building the global graph for better code clarity:
if (buildGraph) { + // Build the global graph before creating the loader and load unit instance LoaderUtil.buildGlobalGraph([ absolutePath ]); }core/test-util/CoreTestHelper.ts (1)
36-36: Approve change with suggestion: Consider adding error handling.The addition of
LoaderUtil.buildGlobalGraph(moduleDirs)aligns well with the PR objective of transitioning complexity into the Loader and Graph components. This change integrates the global graph building functionality without altering the method signature or existing logic, which is good for maintaining backward compatibility and current functionality.However, I recommend adding error handling for the
buildGlobalGraphcall to ensure robustness:Consider wrapping the new call in a try-catch block:
static async prepareModules(moduleDirs: string[]): Promise<Array<LoadUnitInstance>> { - LoaderUtil.buildGlobalGraph(moduleDirs); + try { + await LoaderUtil.buildGlobalGraph(moduleDirs); + } catch (error) { + console.error('Failed to build global graph:', error); + throw error; // or handle it as appropriate for your use case + } EggContextStorage.register(); // ... rest of the method }This change assumes
buildGlobalGraphis an async function. If it's synchronous, remove theawaitkeyword.core/common-util/src/Graph.ts (3)
56-87: LGTM: GraphPath updates with minor suggestion.The changes to
GraphPathconsistently implement metadata support throughout the class. The updates topushVertex,popVertex, andtoStringmethods are correct and align with the PR objectives.Suggestion for
toStringmethod:
Consider using template literals for better readability:toString() { return this.nodes.map(({ node, meta }, index) => { const connector = index > 0 ? ' -> ' : ''; const metaStr = meta ? `${meta.toString()} ` : ''; return `${connector}${metaStr}${node.val.toString()}`; }).join(''); }
95-119: LGTM: Graph class updates with optimization suggestion.The changes to the
Graphclass consistently implement metadata support and align with the PR objectives. The newfindToNodemethod is a useful addition for retrieving nodes with specific metadata.Suggestion for
findToNodemethod:
Consider short-circuiting the search if no metadata is provided:findToNode(id: string, meta?: M): GraphNode<T, M> | undefined { const node = this.nodes.get(id); if (!node || !meta) return node; for (const { node: toNode, meta: edgeMeta } of node.toNodeMap.values()) { if (edgeMeta && meta.equal(edgeMeta)) { return toNode; } } return undefined; }This change allows the method to return the node immediately if no metadata is provided, potentially improving performance in some cases.
Line range hint
1-184: Summary: PR objectives achieved with minor suggestions.The changes in this file successfully implement metadata support throughout the Graph-related classes, aligning well with the PR objective of shifting complexity into the Graph component. The implementation is consistent and generally correct.
Some suggestions for improvement:
- Consider supporting multiple edges between nodes with different metadata.
- Refactor the
appendVertexToPathmethod to use an iterative approach to prevent potential stack overflow issues.- Minor optimizations for the
toStringmethod inGraphPathand thefindToNodemethod inGraph.Overall, the changes achieve the stated objectives while maintaining the existing functionality. After addressing the suggestions, this implementation should provide a solid foundation for future enhancements mentioned in the PR description, such as loading protocol descriptions and implementing dump/restore functionality for GlobalGraph.
core/runtime/test/LoadUnitInstance.test.ts (1)
Line range hint
1-210: Summary of changes and suggestions for next steps.The modifications to this test file align well with the PR objective of transitioning complexity into the Loader and Graph components. The new imports and the use of
LoaderUtil.buildGlobalGraphindicate a more structured approach to setting up the test environment for multi-module scenarios.Suggestions for next steps:
- Provide documentation or comments explaining the purpose of the
falseargument increateLoadUnitInstancecalls.- Ensure that these changes are consistent with any related modifications in other parts of the codebase.
- Update the PR checklist to reflect the progress made, particularly regarding passing tests and updating documentation.
Consider adding a brief comment or documentation within the test file explaining the new setup process using
LoaderUtil.buildGlobalGraph. This will help other developers understand the purpose and importance of this change in the context of the larger refactoring effort.core/test-util/LoaderUtil.ts (1)
44-74: Usefor...ofloops for cleaner iteration over module paths.Instead of using index-based
forloops, consider usingfor...ofloops for better readability and modern coding practices.For example, replace:
-for (let i = 0; i < modulePaths.length; i++) { - const modulePath = modulePaths[i]; +for (const modulePath of modulePaths) {This makes the code more concise and easier to understand.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- core/common-util/src/Graph.ts (6 hunks)
- core/runtime/test/LoadUnitInstance.test.ts (2 hunks)
- core/runtime/test/util.ts (1 hunks)
- core/test-util/CoreTestHelper.ts (2 hunks)
- core/test-util/LoaderUtil.ts (2 hunks)
- core/test-util/TestLoader.ts (1 hunks)
🔇 Additional comments (15)
core/runtime/test/util.ts (3)
6-6: LGTM: New import statement for LoaderUtil.The import statement for
LoaderUtilfrom '@eggjs/module-test-util' is correctly added and follows the existing import style. This import is necessary for the new functionality in thecreateLoadUnitInstancemethod.
9-9: LGTM: Updated method signature with new optional parameter.The
createLoadUnitInstancemethod signature has been updated to include a new optional parameterbuildGraphwith a default value oftrue. This change provides more flexibility while maintaining backward compatibility. The parameter name is descriptive and its purpose is clear.
6-13: Summary: Changes align with PR objectives and improve flexibility.The modifications to the
TestUtilclass successfully shift some complexity into the Loader and Graph components, as intended by the PR objectives. The addition of thebuildGraphparameter and the conditional block for building the global graph provide more flexibility in the instance creation process while maintaining backward compatibility.These changes lay the groundwork for future implementations mentioned in the PR description, such as loading protocol descriptions for the loader and implementing dump/restore functionality for GlobalGraph.
To ensure that these changes don't introduce any unintended side effects, please run the following verification script:
This script will help ensure that:
- Existing tests still pass with the new changes.
- There are no unexpected usages of
createLoadUnitInstancewithout the new parameter.- The new parameter is being used correctly in other parts of the codebase.
✅ Verification successful
Verification Complete: No issues found.
All usages of
createLoadUnitInstanceeither maintain backward compatibility by omitting the new parameter or correctly utilize it with appropriate boolean values. The changes align with the PR objectives and ensure flexibility without introducing any issues.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that existing tests pass and that the new parameter is used correctly. # Test: Run npm test npm test # Test: Check for any usages of createLoadUnitInstance without the new parameter rg --type typescript "createLoadUnitInstance\([^,\)]*\)" -g '!core/runtime/test/util.ts' # Test: Check for any usages of createLoadUnitInstance with the new parameter rg --type typescript "createLoadUnitInstance\([^,\)]*,\s*(true|false)\)" -g '!core/runtime/test/util.ts'Length of output: 349
Script:
#!/bin/bash # Description: Verify that the new parameter is used correctly in createLoadUnitInstance without running tests. # Check for any usages of createLoadUnitInstance without the new parameter rg "createLoadUnitInstance\([^,\)]*\)" --glob '!core/runtime/test/util.ts' # Check for any usages of createLoadUnitInstance with the new parameter rg "createLoadUnitInstance\([^,]+,\s*(true|false)\)" --glob '!core/runtime/test/util.ts'Length of output: 2801
core/test-util/CoreTestHelper.ts (2)
12-12: LGTM: New import statement is correctly added.The import of
LoaderUtilfrom the local module is properly formatted and consistent with the existing import style. This import is necessary for the new functionality added to theprepareModulesmethod.
Line range hint
35-41: Suggestion: Consider adding tests for the new functionality.The changes to
prepareModulesmethod are minimal and well-integrated. However, to ensure the newbuildGlobalGraphfunctionality works as expected and doesn't introduce any regressions, consider adding or updating tests for this method.To verify the impact and usage of the new
buildGlobalGraphfunction, you can run the following script:This script will help identify where
buildGlobalGraphis being used and which test files might need updating to cover the new functionality.core/common-util/src/Graph.ts (6)
5-8: LGTM: New EdgeMeta interface.The introduction of the
EdgeMetainterface is a good addition. It provides a clear contract for edge metadata, supporting equality comparison and string representation. This aligns well with the PR objective of shifting complexity into the Graph component.
10-13: Consider supporting multiple edges between nodes.The updates to
GraphNodeto include metadata are good and align with the PR objectives. However, the current implementation usingnode.idas the key intoNodeMapandfromNodeMapmay not support multiple edges between the same nodes with different metadata.Consider modifying the data structure to allow for this scenario, perhaps by using a composite key or storing arrays of edges.
23-35: LGTM: Updated vertex addition methods.The changes to
addToVertexandaddFromVertexcorrectly implement the storage of metadata alongside the node. The methods maintain their existing behavior while accommodating the new metadata parameter, which is consistent with the class property changes.
122-128: Consider refactoring to avoid potential stack overflow.The updates to
appendVertexToPathcorrectly handle metadata propagation. However, the recursive nature of this method could lead to a stack overflow for graphs with large depth or cycles.Consider refactoring this method to use an iterative approach with a stack or queue to manage traversal. This can improve performance and prevent potential stack issues in large graphs.
135-135: LGTM: Updated loopPath method signature.The
loopPathmethod signature has been correctly updated to use the new generic types. This change maintains consistency with the rest of the class updates.
146-146: LGTM: Updated accessNode and sort method signatures.The
accessNodeandsortmethod signatures have been correctly updated to use the new generic types. These changes maintain consistency with the rest of the class updates while preserving the existing functionality.Also applies to: 171-171
core/runtime/test/LoadUnitInstance.test.ts (2)
15-16: LGTM: New imports added for enhanced functionality.The addition of
pathfrom 'node:path' andLoaderUtilfrom '@eggjs/module-test-util' is appropriate for the changes made in the test setup. These imports provide necessary functionality for working with file paths and loading modules in the test environment.
153-160: Approve changes with a request for clarification.The modifications to the test setup look good and align with the PR objective of transitioning complexity into the Loader and Graph components. The use of
LoaderUtil.buildGlobalGraphprovides a more structured approach to setting up the test environment.However, could you please clarify the purpose of the
falseargument in thecreateLoadUnitInstancecalls? It would be helpful to understand how this affects the instantiation behavior and why it's necessary.To ensure consistency across the codebase, let's check for other occurrences of
createLoadUnitInstance:✅ Verification successful
Changes verified successfully.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other occurrences of createLoadUnitInstance rg "createLoadUnitInstance" --type tsLength of output: 3567
Script:
#!/bin/bash # Find the definition of createLoadUnitInstance rg "createLoadUnitInstance\s*\(" --type ts --context 3Length of output: 13115
core/test-util/LoaderUtil.ts (2)
69-74: Verify ifmultiInstanceEggProtoClassshould be module-specific.In the call to
LoaderUtil.buildModuleNode(lines 69-74), themultiInstanceEggProtoClassarray, which aggregates multi-instance classes from all modules, is passed to each module node. This means every module node receives multi-instance classes from all modules.Is this the intended behavior? Should the multi-instance classes be scoped to their respective modules instead?
38-38: Ensure thread safety when modifyingGlobalGraph.instance.Directly setting
GlobalGraph.instance = new GlobalGraph();on line 38 may introduce concurrency issues ifGlobalGraph.instanceis accessed from multiple threads.Consider implementing a thread-safe singleton pattern or synchronizing access to
GlobalGraph.instanceto prevent potential race conditions.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (6)
core/metadata/src/model/ProtoDescriptorHelper.ts (4)
60-74: Consider improving error handling increateByDynamicMultiInstanceClazzThe method correctly handles the creation of ProtoDescriptors for dynamic multi-instance classes. However, consider the following improvements:
- Similar to the previous method, replace the assertion with a more graceful error handling approach.
- Handle the case where
instancePropertyis not found more explicitly.Here's a suggestion for improved error handling:
static createByDynamicMultiInstanceClazz(clazz: EggProtoImplClass, options: { // ... options ... }): ProtoDescriptor[] { if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { console.warn(`Class ${clazz.name} is not a MultiInstancePrototype`); return []; } const instanceProperty = PrototypeUtil.getDynamicMultiInstanceProperty(clazz, { moduleName: options.instanceModuleName, unitPath: options.instanceDefineUnitPath, }); if (!instanceProperty) { console.warn(`PrototypeInfo not found for class ${clazz.name}`); return []; } return ProtoDescriptorHelper.#createByMultiInstanceClazz(clazz, instanceProperty, options); }This approach provides more informative feedback and handles error cases more gracefully.
Also, note that this method delegates to a private method
#createByMultiInstanceClazzfor the actual creation of ProtoDescriptors, which is a good practice for code reuse between dynamic and static multi-instance classes.
76-88: Consider improving error handling and reducing duplicationThe
createByStaticMultiInstanceClazzmethod is very similar tocreateByDynamicMultiInstanceClazz. Consider the following improvements:
- Implement similar error handling improvements as suggested for
createByDynamicMultiInstanceClazz.- Given the similarity between these methods, consider refactoring to reduce duplication.
Here's a suggestion for improved error handling:
static createByStaticMultiInstanceClazz(clazz: EggProtoImplClass, options: { // ... options ... }): ProtoDescriptor[] { if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { console.warn(`Class ${clazz.name} is not a MultiInstancePrototype`); return []; } const instanceProperty = PrototypeUtil.getStaticMultiInstanceProperty(clazz); if (!instanceProperty) { console.warn(`PrototypeInfo not found for class ${clazz.name}`); return []; } return ProtoDescriptorHelper.#createByMultiInstanceClazz(clazz, instanceProperty, options); }To reduce duplication, you could create a helper method that encapsulates the common logic between
createByDynamicMultiInstanceClazzandcreateByStaticMultiInstanceClazz, parameterizing the differences.
134-164: Consider improving error handling increateByInstanceClazzThe
createByInstanceClazzmethod correctly creates a single ProtoDescriptor for a given instance class. However, consider the following improvements:
- Replace assertions with more graceful error handling, similar to the suggestions for previous methods.
- Handle the case where
propertyis not found more explicitly.Here's a suggestion for improved error handling:
static createByInstanceClazz(clazz: EggProtoImplClass, ctx: MultiInstancePrototypeGetObjectsContext): ProtoDescriptor | null { if (!PrototypeUtil.isEggPrototype(clazz)) { console.warn(`Class ${clazz.name} is not an EggPrototype`); return null; } if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) { console.warn(`Class ${clazz.name} is a MultiInstancePrototype, not a regular Prototype`); return null; } const property = PrototypeUtil.getProperty(clazz); if (!property) { console.warn(`PrototypeInfo not found for class ${clazz.name}`); return null; } // ... rest of the method ... }This approach provides more informative feedback and handles error cases more gracefully.
The construction of the ClassProtoDescriptor is correct and comprehensive, including all necessary properties.
166-180: LGTM: Well-structuredselectProtomethodThe
selectProtomethod is well-implemented, with clear steps for checking each selection criteria (name, access level, and qualifiers). The logic is correct and easy to follow.For improved readability, consider using early returns:
static selectProto(proto: ProtoDescriptor, ctx: ProtoSelectorContext): boolean { // 1. name match if (proto.name !== ctx.name) { return false; } // 2. access level match if (proto.accessLevel !== AccessLevel.PUBLIC && proto.instanceModuleName !== ctx.moduleName) { return false; } // 3. qualifier match return QualifierUtil.matchQualifiers(proto.qualifiers, ctx.qualifiers); }This structure makes the logic even more clear and reduces nesting.
core/metadata/test/LoadUnit.test.ts (2)
3-9: LGTM! Consider cleaning up commented imports.The changes to imports and the addition of the
beforeEachhook are good improvements:
- The new imports align with the PR objective of using
GlobalGraph.- The
beforeEachhook ensures a clean state for each test, which is a good practice.Consider removing the commented-out imports (lines 13-16) if they are no longer needed, to keep the code clean.
Also applies to: 13-17, 21-23
Line range hint
1-203: Overall, excellent changes that align with PR objectives.The modifications to this test file consistently implement the use of
GlobalGraphandbuildGlobalGraph, aligning well with the PR's goal of transitioning complexity into the Loader and Graph components. The error handling improvements and expanded multi-instance test setup enhance the overall quality and coverage of the tests.Consider the following suggestions for further improvement:
- Remove commented-out imports if they're no longer needed.
- Extract repetitive setup code in multi-instance tests into helper functions.
- Ensure that these changes are reflected in the documentation, as mentioned in the PR checklist.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
- core/metadata/src/impl/EggPrototypeBuilder.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptorHelper.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1 hunks)
- core/metadata/test/LoadUnit.test.ts (8 hunks)
- core/metadata/test/fixtures/LoaderUtil.ts (2 hunks)
- core/types/metadata/model/ProtoDescriptor.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- core/metadata/src/impl/EggPrototypeBuilder.ts
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts
- core/types/metadata/model/ProtoDescriptor.ts
🧰 Additional context used
🪛 Biome
core/metadata/src/model/ProtoDescriptorHelper.ts
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
🔇 Additional comments (16)
core/metadata/test/fixtures/LoaderUtil.ts (2)
3-6: LGTM: New imports are correctly added.The new import statements for
GlobalModuleNodeBuilder,GlobalGraph,Loader, andModuleConfigUtilare correctly placed and necessary for the newly added functions.
Line range hint
1-87: Overall implementation looks good, but some objectives are pendingThe new functions
buildModuleNodeandbuildGlobalGraphsuccessfully implement the core functionality for building a global graph with module nodes, which aligns with the PR objective of transitioning complexity into the Loader and Graph components.However, some of the future implementations mentioned in the PR objectives are not yet addressed:
- Loading protocol descriptions for the loader instead of adding protocols at runtime.
- Enforcing a strict mode to verify the existence of protocols before booting.
- Implementing dump/restore functionality for GlobalGraph.
Consider creating separate issues or follow-up PRs to address these pending objectives. Would you like me to help draft these issues?
core/metadata/src/model/ProtoDescriptorHelper.ts (2)
22-39: LGTM: Efficient implementation ofaddDefaultQualifierThe
addDefaultQualifiermethod is well-implemented. It efficiently adds default qualifiers while avoiding duplicates. The use of the spread operator andfindmethod for checking existing qualifiers is a good approach.
1-181: Overall assessment: Well-implemented with room for improvementsThe
ProtoDescriptorHelperclass provides a comprehensive set of utility methods for managing prototype descriptors in a multi-instance context. The implementation is generally correct and well-structured. However, there are several areas where improvements can be made:
- Consider refactoring the class into a module with exported functions for better adherence to TypeScript/JavaScript best practices.
- Implement more robust error handling throughout the methods, replacing assertions with more graceful error handling approaches.
- Look for opportunities to reduce code duplication, especially between similar methods like
createByDynamicMultiInstanceClazzandcreateByStaticMultiInstanceClazz.- Consider using early returns in methods like
selectProtofor improved readability.These changes would enhance the maintainability, robustness, and readability of the code while preserving its current functionality.
🧰 Tools
🪛 Biome
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/src/impl/ModuleLoadUnit.ts (6)
212-212: Simplified constructor aligns with PR objectivesThe removal of the
loaderparameter from the constructor aligns with the PR objective of shifting complexity to the Loader and Graph components. This simplification is a good step towards the stated goals.
Line range hint
1-304: Summary of changes and recommendationsThe changes in this file align well with the PR objectives of shifting complexity to the Loader and Graph components. The removal of the
loaderdependency and the introduction ofGlobalGraphare positive steps towards this goal.Key points and recommendations:
- Fix typos in variable and method names (e.g.,
globalGrahptoglobalGraph).- Remove unused imports and unnecessary class properties.
- Add error handling to asynchronous operations, particularly in the
initmethod.- Consider adding more explanatory comments, especially for filtering logic in
doLoadClazz.Overall, the changes are moving in the right direction, but addressing these points will improve code quality and maintainability.
23-23:⚠️ Potential issueFix typo in import statement
There's a typo in the import statement for
FrameworkErrorFormater. It should beFrameworkErrorFormatter.Please apply this diff to fix the typo:
-import { FrameworkErrorFormater } from 'egg-errors'; +import { FrameworkErrorFormatter } from 'egg-errors';Likely invalid or redundant comment.
67-67:⚠️ Potential issueFix usage of PrototypeUtil in toString method
The
toStringmethod is incorrectly usingthis.PrototypeUtil.PrototypeUtilis an imported module, not a class property.Please apply this diff to fix the usage:
- return `${this.clazz.name}@${this.PrototypeUtil.getFilePath(this.clazz)}`; + return `${this.clazz.name}@${PrototypeUtil.getFilePath(this.clazz)}`;Likely invalid or redundant comment.
248-250: 🛠️ Refactor suggestionAdd error handling to init method
The
initmethod now uses asynchronous operations to create prototypes. Consider adding error handling to prevent unexpected crashes and improve robustness.Consider wrapping the prototype creation in a try-catch block:
async init() { this.loadClazz(); for (const protoDescriptor of this.protos) { + try { const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this); EggPrototypeFactory.instance.registerPrototype(proto, this); + } catch (error) { + console.error(`Failed to create prototype for ${protoDescriptor.name}:`, error); + // Consider whether to throw or continue processing other prototypes + } } }Likely invalid or redundant comment.
5-7: Verify usage of newly imported typesThe newly imported types
GraphNodeObjandInitTypeQualifierAttributefrom '@eggjs/tegg-types' don't appear to be used in the visible changes. Please ensure these imports are necessary for the changes made or remove them if unused.core/metadata/test/LoadUnit.test.ts (6)
53-54: LGTM! Good integration ofbuildGlobalGraph.The addition of
buildGlobalGraphbefore creating the load unit aligns well with the PR objective. It ensures that the global graph is properly set up, which should provide a more realistic test environment.
72-74: LGTM! Improved error handling in test.The use of
assert.rejectssimplifies the error handling and makes the test more concise. It also directly tests thebuildGlobalGraphfunction, which aligns well with the PR objectives.
82-87: LGTM! Enhanced error checking in test.The use of
assert.rejectswith specific error message checks improves the test's precision. It ensures thatbuildGlobalGraphis failing for the expected reasons, which aligns well with the PR objectives and provides better test coverage.
93-97: LGTM! Improved error validation in multi-proto test.The use of
assert.rejectswith a specific error message check enhances the test's accuracy. It ensures thatbuildGlobalGraphis failing due to duplicate proto, which is the expected behavior for this test case.
105-105: LGTM! Consistent use ofbuildGlobalGraph.The addition of
buildGlobalGraphcalls in these tests maintains consistency with the PR's objective. It ensures that the global graph is properly initialized before load unit creation, providing a more accurate test environment.Also applies to: 114-114
174-189: LGTM! Comprehensive setup for multi-instance test.The expanded test setup with multiple modules and loaders provides a more thorough test for multi-instance scenarios, aligning well with the PR objectives. The use of
buildGlobalGraphwith multiple inputs is consistent with earlier changes.As noted in a previous review, consider extracting the repetitive setup code into a helper function to improve readability and maintainability. This suggestion is still applicable to this expanded setup.
killagu
force-pushed
the
chore/global_graph
branch
from
f1ee808 to
459f09a
Compare
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (13)
plugin/tegg/lib/EggModuleLoader.ts (2)
25-36: LGTM: Refactored buildAppGraph methodThe refactoring of the
buildAppGraphmethod aligns well with the PR objective. It simplifies the graph-building process by leveragingLoaderFactory.loadAppandGlobalGraph.create. This approach effectively shifts complexity to the Loader and Graph components.Consider adding error handling to this method:
private buildAppGraph() { try { // ... existing code ... } catch (error) { console.error('Error building app graph:', error); throw error; // or handle it as appropriate for your use case } }This will help with debugging if issues arise during graph building.
Line range hint
1-55: Overall assessment: Changes align well with PR objectivesThe modifications to the
EggModuleLoaderclass successfully transition the complexity of MultiInstanceProto into the Loader and Graph components, as intended. The introduction ofGlobalGraphand the refactoring of methods likebuildAppGraphandloadModuleeffectively minimize changes to other parts of the codebase while implementing the desired functionality.Some suggestions for further improvement:
- Consider addressing the direct assignment to
GlobalGraph.instancein the constructor.- Implement error handling in critical methods like
buildAppGraph.- Optimize the loader creation process in the
loadModulemethod.These changes provide a solid foundation for future implementations mentioned in the PR description, such as loading protocol descriptions for the loader and implementing dump/restore functionality for GlobalGraph.
standalone/standalone/src/EggModuleLoader.ts (1)
23-24: LGTM:loadmethod updated correctly with minor optimization opportunityThe
loadmethod has been appropriately updated to use theglobalGraphfor sorting and retrieving themoduleConfigList. This change is consistent with the new graph generation approach and aligns with the PR objectives.A minor optimization suggestion:
- this.globalGraph.sort(); - const moduleConfigList = GlobalGraph.instance!.moduleConfigList; + this.globalGraph.sort(); + const moduleConfigList = this.globalGraph.moduleConfigList;This change avoids using the global instance and instead uses the class property directly, which is slightly more efficient and maintains better encapsulation.
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
86-104: Approve implementation with minor suggestions.The new
createProtoByDescriptormethod is well-implemented and consistent with the existingcreateProtomethod. It correctly handles the creation of a single prototype using aClassProtoDescriptor.Consider the following minor improvements:
- Replace
thiswith the class name to avoid potential confusion in a static context:- const creator = this.getPrototypeCreator(protoDescriptor.protoImplType); + const creator = EggPrototypeCreatorFactory.getPrototypeCreator(protoDescriptor.protoImplType);
- Use template literals for the error message to improve readability:
- throw new Error(`not found proto creator for type: ${protoDescriptor.protoImplType}`); + throw new Error(`Proto creator not found for type: ${protoDescriptor.protoImplType}`);For future consideration: Unlike
createProto, this method doesn't handle multi-instance prototypes. If this functionality is needed in the future, you may want to add similar logic or consider refactoring both methods to share common code.🧰 Tools
🪛 Biome
[error] 87-87: 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/common-util/src/Graph.ts (1)
111-119: LGTM with suggestion: New findToNode method added.The new
findToNodemethod is a good addition, allowing for finding a specific connected node based on both ID and metadata. The implementation correctly iterates through thetoNodeMapand checks for metadata equality.Consider handling the case where no metadata is provided to the method. Currently, it will always return
undefinedif theedgeMetais falsy. You might want to return the node if no metadata is provided, or add a parameter to control this behavior.core/metadata/src/model/ProtoDescriptorHelper.ts (1)
166-181: LGTM: Well-implementedselectProtomethod with a minor suggestionThe
selectProtomethod is well-implemented:
- It follows a clear logical flow for selecting a prototype based on given criteria.
- The checks for name, access level, and qualifiers are concise and effective.
To slightly improve readability, consider using early returns:
static selectProto(proto: ProtoDescriptor, ctx: ProtoSelectorContext): boolean { // 1. name match if (proto.name !== ctx.name) { return false; } // 2. access level match if (proto.accessLevel !== AccessLevel.PUBLIC && proto.instanceModuleName !== ctx.moduleName) { return false; } // 3. qualifier match return QualifierUtil.matchQualifiers(proto.qualifiers, ctx.qualifiers); }This minor change eliminates the need for the final
return truestatement, making the function's logic slightly more explicit.🧰 Tools
🪛 Biome
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/test/LoadUnit.test.ts (3)
13-16: Consider removing commented out importsThe imports for
App,App2,BizManager, andSecretare commented out. If these are no longer needed, it's best to remove them entirely to keep the codebase clean.
71-74: Improved error handling in recursive deps testThe use of
assert.rejectsimproves the test by checking for the error during graph building. This is more accurate and consistent with the new architecture.Consider adding a more specific error message check:
await assert.rejects(async () => { return buildGlobalGraph([ repoModulePath ], [ loader ]); }, (error: Error) => { assert(error.message.includes('proto has recursive deps')); return true; });This ensures that the exact expected error is thrown.
177-192: Comprehensive setup for multi-instance inject testThe expanded setup with multiple modules and loaders provides a more thorough test for complex scenarios. The use of
buildGlobalGraphis consistent with other tests and aligns with the new architecture.To improve readability and maintainability, consider extracting the module paths and loader creation into a separate setup function:
function setupMultiInstanceTest() { const modules = [ path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app'), path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/app2'), path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/foo'), path.join(__dirname, './fixtures/modules/app-multi-inject-multi/app/modules/bar'), ]; const loaders = modules.map(module => new TestLoader(module)); return { modules, loaders }; } // In the test: const { modules, loaders } = setupMultiInstanceTest(); buildGlobalGraph(modules, loaders);This would make the test setup more concise and easier to maintain.
core/loader/src/LoaderFactory.ts (1)
31-31: Consider removing the unusedprotosproperty if not necessaryThe
protosarray in theModuleDescriptorobject is initialized but not populated within this method. If it's not required, consider removing it to simplify the code. If it is intended to be used, ensure that it is properly populated.core/core-decorator/src/util/PrototypeUtil.ts (3)
149-152: Add @returns to method documentationThe method
getStaticMultiInstancePropertyis missing the@returnsdocumentation. Adding this will clarify what the method returns and improve code readability.Apply this diff to update the documentation:
/** * Get instance property of Static multi-instance prototype. * @param {EggProtoImplClass} clazz - + * @returns {EggMultiInstancePrototypeInfo | undefined} - The static multi-instance prototype information, or undefined if not available. */
160-164: Add @returns to method documentationThe method
getDynamicMultiInstancePropertylacks a@returnstag in its documentation. Including it will enhance the clarity of the method's purpose and output.Apply this diff to update the documentation:
/** * Get instance property of Dynamic multi-instance prototype. * @param {EggProtoImplClass} clazz - * @param {MultiInstancePrototypeGetObjectsContext} ctx - + * @returns {EggMultiInstancePrototypeInfo | undefined} - The dynamic multi-instance prototype information, or undefined if not available. */
189-189: Address the TODO commentThere is a TODO comment indicating that this code block should be deleted in the next major version:
// TODO delete in next major version, default qualifier be added in ProtoDescriptorHelper.addDefaultQualifierConsider tracking this task to ensure it's addressed appropriately in future releases.
Would you like me to open a new GitHub issue to track this TODO, or assist in implementing the required changes?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (34)
- core/common-util/src/Graph.ts (6 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (4 hunks)
- core/core-decorator/src/util/QualifierUtil.ts (1 hunks)
- core/loader/src/LoaderFactory.ts (2 hunks)
- core/metadata/index.ts (1 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/ModuleDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptorHelper.ts (1 hunks)
- core/metadata/src/model/graph/GlobalGraph.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNode.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1 hunks)
- core/metadata/src/model/graph/ProtoNode.ts (1 hunks)
- core/metadata/src/model/graph/ProtoSelector.ts (1 hunks)
- core/metadata/test/GlobalGraph.test.ts (1 hunks)
- core/metadata/test/LoadUnit.test.ts (8 hunks)
- core/metadata/test/fixtures/LoaderUtil.ts (2 hunks)
- core/runtime/test/LoadUnitInstance.test.ts (2 hunks)
- core/runtime/test/util.ts (1 hunks)
- core/test-util/CoreTestHelper.ts (2 hunks)
- core/test-util/LoaderUtil.ts (2 hunks)
- core/types/common/ModuleConfig.ts (1 hunks)
- core/types/core-decorator/enum/MultiInstanceType.ts (1 hunks)
- core/types/core-decorator/index.ts (1 hunks)
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/types/metadata/enum/ProtoDescriptorType.ts (1 hunks)
- core/types/metadata/index.ts (1 hunks)
- core/types/metadata/model/Loader.ts (1 hunks)
- core/types/metadata/model/ProtoDescriptor.ts (1 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (2 hunks)
- standalone/standalone/src/EggModuleLoader.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (23)
- core/core-decorator/src/util/QualifierUtil.ts
- core/metadata/index.ts
- core/metadata/src/impl/EggPrototypeBuilder.ts
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts
- core/metadata/src/model/graph/GlobalModuleNode.ts
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts
- core/metadata/src/model/graph/ProtoNode.ts
- core/metadata/src/model/graph/ProtoSelector.ts
- core/metadata/test/GlobalGraph.test.ts
- core/metadata/test/fixtures/LoaderUtil.ts
- core/runtime/test/LoadUnitInstance.test.ts
- core/runtime/test/util.ts
- core/test-util/CoreTestHelper.ts
- core/test-util/LoaderUtil.ts
- core/types/common/ModuleConfig.ts
- core/types/core-decorator/enum/MultiInstanceType.ts
- core/types/core-decorator/index.ts
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts
- core/types/metadata/enum/ProtoDescriptorType.ts
- core/types/metadata/index.ts
- core/types/metadata/model/Loader.ts
- core/types/metadata/model/ProtoDescriptor.ts
🧰 Additional context used
🪛 Biome
core/metadata/src/factory/EggPrototypeCreatorFactory.ts
[error] 87-87: 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/metadata/src/model/ModuleDescriptor.ts
[error] 16-27: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/src/model/ProtoDescriptorHelper.ts
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
standalone/standalone/src/EggModuleLoader.ts
[error] 37-37: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (36)
core/metadata/src/model/ModuleDescriptor.ts (2)
1-5: LGTM: Imports and constants are well-defined.The imports are appropriate for the file's purpose, and using an environment variable for
DUMP_PATHprovides flexibility for different environments.
7-14: LGTM: ModuleDescriptor interface is well-defined.The
ModuleDescriptorinterface is comprehensive and well-structured. It provides a clear template for module descriptor objects, including all necessary properties. The use of optional properties and imported types enhances flexibility and type consistency.plugin/tegg/lib/EggModuleLoader.ts (3)
4-4: LGTM: Import and property addition for GlobalGraphThe addition of the
GlobalGraphimport and theglobalGraphproperty to theEggModuleLoaderclass aligns well with the PR objective of transitioning to a global graph structure. These changes lay the foundation for the refactoring work in the rest of the file.Also applies to: 12-12
16-16:⚠️ Potential issueReconsider direct assignment to GlobalGraph.instance
While initializing
this.globalGraphis correct, directly assigning toGlobalGraph.instancein the constructor may lead to tight coupling and potential side effects, as noted in a previous review. Consider encapsulating instance management within theGlobalGraphclass or providing a dedicated method for setting the instance.
40-45: 🛠️ Refactor suggestion
⚠️ Potential issueAddress potential issues in loadModule method
Update globalGraph after rebuilding:
The call tothis.buildAppGraph()doesn't updatethis.globalGraph. This may lead to operating on an outdated graph. Consider updating the code as follows:this.globalGraph = this.buildAppGraph(); this.globalGraph.sort();Optimize loader creation:
Creating a new loader for each module usingLoaderFactory.createLoader()might be inefficient. If possible, consider caching or reusing loaders to improve performance.standalone/standalone/src/EggModuleLoader.ts (3)
1-1: LGTM: Imports and new property look goodThe new imports and the addition of the
globalGraphproperty are appropriate for the changes made in the class. The property is correctly typed and marked as private, which is good for encapsulation.Also applies to: 7-7
11-11: Split multiple assignments for better readabilityThe multiple assignment in a single expression can reduce code readability. This issue was previously flagged in a review comment. To improve clarity, consider splitting the assignments:
- GlobalGraph.instance = this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences); + this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences); + GlobalGraph.instance = this.globalGraph;This change will make the code easier to understand and maintain.
14-18: LGTM: RefactoredgenerateAppGraphaligns with PR objectivesThe refactoring of the
generateAppGraphmethod effectively simplifies the graph generation process. It now utilizesLoaderFactory.loadAppto obtain module descriptors and creates aGlobalGraphinstance, which aligns well with the PR objective of shifting complexity to the Loader and Graph components. The method being static is appropriate for its functionality.This change should help in minimizing modifications to other parts of the codebase, as intended.
core/common-util/src/Graph.ts (7)
5-8: LGTM: Well-defined EdgeMeta interface.The new
EdgeMetainterface is a good addition. It provides a clear contract for edge metadata, allowing for comparison and string representation. This interface will support the integration of metadata into the graph structure.
10-13: LGTM: GraphNode class updated to support metadata.The changes to the
GraphNodeclass effectively incorporate metadata support. The class signature,toNodeMap,fromNodeMap, and theaddToVertexandaddFromVertexmethods have been appropriately updated to handle the new metadata parameter.Note: The previous comment about supporting multiple edges between the same nodes with different metadata is still applicable. Consider addressing this in future iterations if it aligns with the intended use case.
Also applies to: 23-36
56-56: LGTM: GraphPath class successfully updated to support metadata.The
GraphPathclass has been effectively modified to incorporate metadata support. The changes to the class signature,nodesarray,pushVertex,popVertex, andtoStringmethods are consistent and well-implemented. The newtoStringmethod provides a more detailed representation of the path, including metadata when present.Also applies to: 58-58, 60-71, 75-87
95-96: LGTM: Graph class signature and addVertex method updated correctly.The
Graphclass signature has been appropriately updated to include the generic parameter for metadata. Thenodesmap andaddVertexmethod now correctly use the updated generic types. These changes maintain consistency with the updates made to other classes in the file.Also applies to: 98-104
106-109: LGTM: addEdge method updated to support metadata.The
addEdgemethod has been successfully modified to include metadata support. The changes ensure that metadata is correctly associated with both incoming and outgoing connections when creating an edge.
122-127: LGTM: appendVertexToPath method updated to support metadata.The
appendVertexToPathmethod has been successfully modified to include metadata support. The changes ensure that metadata is correctly preserved when appending vertices to the path.Note: The previous comment about potential stack overflow due to deep recursion in this method is still applicable. Consider addressing this in future iterations to improve performance and prevent potential stack issues.
135-136: LGTM: loopPath, accessNode, and sort methods updated correctly.The
loopPath,accessNode, andsortmethods have been appropriately updated to use the new generic types. These changes maintain consistency with the updates made to other parts of theGraphclass. The core logic of these methods remains unchanged, which is correct as they don't directly deal with metadata.Also applies to: 146-157, 171-172
core/metadata/src/model/ProtoDescriptorHelper.ts (3)
22-39: LGTM: Well-implementedaddDefaultQualifiermethodThe
addDefaultQualifiermethod is well-implemented:
- It maintains immutability by creating a new array.
- It checks for existing qualifiers before adding defaults, avoiding duplicates.
- The logic is clear and concise.
Good job on this implementation!
21-181: 🛠️ Refactor suggestionConsider refactoring the class into a module with exported functions
The
ProtoDescriptorHelperclass contains only static methods, which goes against best practices in TypeScript/JavaScript. It would be more idiomatic and maintainable to refactor this into a module with exported functions.Here's an example of how you could start refactoring this:
// ProtoDescriptorHelper.ts export function addDefaultQualifier(qualifiers: QualifierInfo[], initType: ObjectInitTypeLike, loadUnitName: string): QualifierInfo[] { // ... implementation ... } export function createByMultiInstanceClazz(clazz: EggProtoImplClass, options: { defineModuleName: string; defineUnitPath: string; instanceModuleName: string; instanceDefineUnitPath: string; }): ProtoDescriptor[] { // ... implementation ... } // ... other functions ...This approach would make the code more modular, easier to test, and align with TypeScript/JavaScript best practices.
🧰 Tools
🪛 Biome
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
41-58: 🛠️ Refactor suggestionImprove error handling and type checking in
createByMultiInstanceClazzWhile the method correctly distinguishes between dynamic and static multi-instance classes, consider improving the error handling and type checking:
- Replace the assertion with a more graceful error handling approach.
- Add handling for unknown
MultiInstanceTypevalues.Here's a suggestion for improved implementation:
static createByMultiInstanceClazz(clazz: EggProtoImplClass, options: { // ... options ... }): ProtoDescriptor[] { if (!PrototypeUtil.isEggMultiInstancePrototype(clazz)) { console.warn(`Class ${clazz.name} is not a MultiInstancePrototype`); return []; } const type = PrototypeUtil.getEggMultiInstancePrototypeType(clazz); switch (type) { case MultiInstanceType.DYNAMIC: return ProtoDescriptorHelper.createByDynamicMultiInstanceClazz(clazz, options); case MultiInstanceType.STATIC: if (options.defineModuleName === options.instanceModuleName) { return ProtoDescriptorHelper.createByStaticMultiInstanceClazz(clazz, options); } return []; default: console.warn(`Unknown MultiInstanceType: ${type}`); return []; } }This approach provides more informative feedback and handles all possible cases.
core/metadata/src/model/graph/GlobalGraph.ts (6)
17-19: Reminder: Address TODO comments regarding strict modeThere's a TODO comment indicating plans to enforce strict mode and ensure all protos are loaded before building the global graph. Consider prioritizing this change in the next major version to enhance the reliability of the graph-building process.
72-79: Improve error messages for duplicate modules and protosThe error messages for duplicate modules and protos could be more informative. Consider including specific identifiers like module names or IDs to make troubleshooting easier.
1-248: Overall assessment of GlobalGraph implementationThe
GlobalGraphclass provides a comprehensive solution for managing module and proto dependencies. While the core functionality appears sound, there are opportunities for optimization and improvement:
- Address the TODO comments, particularly regarding strict mode enforcement.
- Optimize the
#findDependencyProtoWithDefaultQualifiersmethod to improve performance.- Consider using a
Mapin#findModuleNodefor faster lookups.- Improve error messages for better debugging.
- Remove or utilize the unused static
instanceproperty.These enhancements will contribute to a more robust and efficient implementation.
62-63:⚠️ Potential issueUnused static property 'instance'
The static property
instance?is declared but never assigned or used within the class. If you intend to implement a singleton pattern, consider initializing and utilizing this property accordingly. Otherwise, it can be safely removed to clean up the code.
118-135:⚠️ Potential issuePotential performance issue in
#findDependencyProtoWithDefaultQualifiersThe method
#findDependencyProtoWithDefaultQualifiersmay have performance issues due to its O(n2) complexity, as noted in the TODO comment. Consider optimizing this method by indexing protos by name and qualifiers to reduce lookup time and improve performance.
180-186: 🛠️ Refactor suggestionOptimize
#findModuleNodefor better performanceThe method
#findModuleNodeperforms a linear search overthis.moduleGraph.nodes.values(). To improve performance, especially with a large number of modules, consider using aMapto store module nodes keyed by module name, allowing constant-time lookups.core/metadata/src/impl/ModuleLoadUnit.ts (5)
Line range hint
218-252: Review changes in ModuleLoadUnit methodsSignificant changes have been made to several methods in ModuleLoadUnit:
doLoadClazznow usesglobalGrahp(should beglobalGraph) instead of a loader.loadClazzhas been simplified to only calldoLoadClazz.initnow iterates overthis.protosinstead ofthis.clazzList.These changes reflect a shift from using a loader to using a global graph for managing prototypes. While this approach might be more centralized, consider the following:
- Performance: Ensure that using the global graph doesn't introduce performance issues for large projects.
- Flexibility: Verify that this change doesn't limit the flexibility of the module loading process.
- Error Handling: The current implementation lacks robust error handling.
Add error handling to the
initmethod:async init() { this.loadClazz(); for (const protoDescriptor of this.protos) { + try { const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this); EggPrototypeFactory.instance.registerPrototype(proto, this); + } catch (error) { + console.error(`Failed to initialize prototype: ${protoDescriptor.name}`, error); + // Consider whether to throw or continue with other prototypes + } } }To understand the impact of these changes, run:
#!/bin/bash # Check usage of globalGraph in the entire codebase rg --type typescript "globalGraph" --glob !core/metadata/src/impl/ModuleLoadUnit.ts # Check for any remaining uses of the old loader rg --type typescript "this\.loader" core/metadata/src/impl/ModuleLoadUnit.tsConsider adding comments explaining the rationale behind these changes and their implications for the overall system architecture.
304-304: Review change in ModuleLoadUnit.createModule methodThe
createModulestatic method has been updated to reflect the changes in the ModuleLoadUnit constructor. Theloaderparameter is no longer passed when creating a new ModuleLoadUnit instance.This change is consistent with the earlier modification to the ModuleLoadUnit constructor. However, ensure that this doesn't break any existing code that relies on the loader being passed or used within the ModuleLoadUnit.
To understand the impact of this change, run:
#!/bin/bash # Check usage of ModuleLoadUnit.createModule rg --type typescript "ModuleLoadUnit\.createModule" --glob !core/metadata/src/impl/ModuleLoadUnit.ts # Check for any remaining references to loader in the file rg --type typescript "loader" core/metadata/src/impl/ModuleLoadUnit.tsConsider adding a comment explaining the rationale behind removing the loader and how module loading is now handled without it.
212-212: Review change in ModuleLoadUnit constructorThe
loaderparameter has been removed from the ModuleLoadUnit constructor. This change aligns with the commented-out loader property, indicating a shift in the loading mechanism.To understand the impact of this change, please run:
#!/bin/bash # Check usage of ModuleLoadUnit constructor rg --type typescript "new ModuleLoadUnit\(" --glob !core/metadata/src/impl/ModuleLoadUnit.tsEnsure that all places where ModuleLoadUnit is instantiated have been updated to reflect this change in the constructor signature. Also, consider adding a comment explaining the rationale behind removing the loader parameter.
Line range hint
1-307: Summary of changes and recommendationsThis review has identified several significant changes to the ModuleLoadUnit class and its related components:
- The shift from using a loader to a global graph for managing prototypes.
- Removal of the
loaderproperty and parameter from various methods.- Introduction of new properties (
protosandclazzList) and a getter (globalGraph).- Updates to methods like
doLoadClazz,loadClazz, andinitto reflect the new approach.While these changes seem to aim for a more centralized approach to module loading, there are a few areas that require attention:
- Ensure all typos are corrected (e.g.,
globalGrahptoglobalGraph).- Improve error handling, especially in methods like
init.- Verify that the removal of the loader doesn't break existing functionality or negatively impact performance.
- Add comments explaining the rationale behind these architectural changes.
- Update any external code that might be affected by the changes to the ModuleLoadUnit constructor and createModule method.
To ensure a comprehensive review, run the following script to check for any remaining issues:
#!/bin/bash # Check for potential issues echo "Checking for typos..." rg --type typescript "grahp" core/metadata/src/impl/ModuleLoadUnit.ts echo "Checking for TODO comments..." rg --type typescript "TODO" core/metadata/src/impl/ModuleLoadUnit.ts echo "Checking for any remaining loader references..." rg --type typescript "loader" core/metadata/src/impl/ModuleLoadUnit.tsPlease address these points to ensure the changes are robust and well-documented.
5-8: Verify usage of newly added importsThe imports
GraphNodeObj,InitTypeQualifierAttribute, andObjectInitTypeLikehave been added. Please ensure these are actually used in the updated code. If they remain unused, consider removing them to maintain clean and efficient imports.To check the usage of these imports, run the following script:
✅ Verification successful
Import usage verified successfully
The imports
GraphNodeObj,InitTypeQualifierAttribute, andObjectInitTypeLikeare actively used incore/metadata/src/impl/ModuleLoadUnit.ts. No unused imports were found.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check usage of newly added imports rg --type typescript "GraphNodeObj|InitTypeQualifierAttribute|ObjectInitTypeLike" core/metadata/src/impl/ModuleLoadUnit.tsLength of output: 541
Script:
#!/bin/bash # Corrected script to check usage of newly added imports rg "GraphNodeObj|InitTypeQualifierAttribute|ObjectInitTypeLike" --glob "*.ts" core/metadata/src/impl/ModuleLoadUnit.tsLength of output: 502
core/metadata/test/LoadUnit.test.ts (6)
21-23: Good addition of beforeEach hookThe new beforeEach hook ensures a clean state for GlobalGraph.instance before each test, which is a good practice for test isolation.
29-31: Appropriate use of buildGlobalGraphThe addition of
buildGlobalGraphbefore creating the load unit ensures proper setup of the global graph, which is consistent with the new architecture.
53-54: Consistent use of buildGlobalGraphThe addition of
buildGlobalGraphbefore creating the load unit is consistent with the new architecture and ensures proper test setup.
82-89: Robust error handling in init failed testThe use of
assert.rejectswith specific error message checks improves the test's robustness. It ensures that the correct error is thrown and includes the expected error messages.
95-100: Simplified and focused multi proto init failure testThe test has been simplified to focus solely on the
buildGlobalGraphstep, which is appropriate for checking duplicate proto errors. The use ofassert.rejectswith a specific error message check ensures that the correct error is thrown.
Line range hint
1-205: Summary of changes to LoadUnit.test.tsThe changes in this file consistently introduce the use of
GlobalGraphandbuildGlobalGraph, reflecting a shift in the underlying architecture. Key improvements include:
- Better test isolation with the new
beforeEachhook.- More robust error handling using
assert.rejects.- Expanded test coverage for complex multi-instance scenarios.
These changes enhance the overall quality and reliability of the test suite. The refactoring suggestions provided in the individual comments can further improve code readability and maintainability.
core/core-decorator/src/util/PrototypeUtil.ts (1)
62-65: Update method documentation to accurately describe the methodThe previous review comment regarding the documentation of
getEggMultiInstancePrototypeTypeis still applicable. The method returns aMultiInstanceTypeorundefined, but the documentation is missing the@returnstag, and the@paramtag's type should be updated toEggProtoImplClass. Please update the method's documentation accordingly.
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (8)
core/metadata/src/model/ModuleDescriptor.ts (3)
8-19: LGTM: Enhanced interfaces for better module descriptor handling.The updates to the
ModuleDescriptorinterface and the addition ofModuleDumpOptionsare well-aligned with the PR objectives:
- The new properties in
ModuleDescriptor(multiInstanceClazzList and protos) support the transition of complexity into the Loader and Graph components.- The
ModuleDumpOptionsinterface provides flexibility for dump operations.Consider adding JSDoc comments to these interfaces to improve documentation. For example:
/** * Represents a module descriptor with associated classes and prototypes. */ export interface ModuleDescriptor { // ... existing properties ... } /** * Options for dumping module descriptors. */ export interface ModuleDumpOptions { /** * Optional custom path for dumping the module descriptor. */ path?: string; }
46-55: LGTM: Improved dump methods with async operations.The changes to
dumpPathanddumpmethods are excellent improvements:
- The
dumpPathmethod now properly handles the newModuleDumpOptions, using optional chaining for safe access.- The
dumpmethod has been updated to use asynchronous file system operations, addressing the previous review comments and improving performance.These changes align well with the PR objectives and best practices.
Consider adding error handling to the
dumpmethod to make it more robust:static async dump(desc: ModuleDescriptor, options?: ModuleDumpOptions) { try { const dumpPath = ModuleDescriptorDumper.dumpPath(desc, options); await fs.mkdir(path.dirname(dumpPath), { recursive: true }); await fs.writeFile(dumpPath, ModuleDescriptorDumper.stringifyDescriptor(desc)); } catch (error) { console.error(`Failed to dump module descriptor: ${error.message}`); throw error; // Re-throw if you want calling code to handle the error } }This addition will provide better visibility into any issues that might occur during the dump process.
1-56: Overall assessment: Good improvements with some refactoring opportunities.The changes to
ModuleDescriptor.tsalign well with the PR objectives and address previous review comments. The introduction of asynchronous operations and the enhanced flexibility in module descriptor handling are particularly noteworthy improvements.However, there are a few areas where the code could be further improved:
As noted by the static analysis tool, the
ModuleDescriptorDumperclass contains only static methods. Consider refactoring this into separate functions to align with best practices and improve maintainability.The JSON stringification process could be more efficient and maintainable by leveraging
JSON.stringifymore effectively.Error handling could be enhanced in the
dumpmethod to provide better visibility into potential issues.These refactoring opportunities, if addressed, would further improve the code quality and maintainability of this module.
To address the static analysis hint and improve the overall structure, consider reorganizing the code into separate functions instead of a class:
export function getDumpPath(desc: ModuleDescriptor, options?: ModuleDumpOptions): string { const dumpDir = DUMP_PATH ?? options?.path ?? desc.unitPath; return path.join(dumpDir, '.egg', `${desc.name}_module_desc.json`); } export async function dumpModuleDescriptor(desc: ModuleDescriptor, options?: ModuleDumpOptions): Promise<void> { try { const dumpPath = getDumpPath(desc, options); await fs.mkdir(path.dirname(dumpPath), { recursive: true }); await fs.writeFile(dumpPath, stringifyDescriptor(desc)); } catch (error) { console.error(`Failed to dump module descriptor: ${error.message}`); throw error; } } // Include the refactored stringifyDescriptor and stringifyClazz functions hereThis structure maintains the same functionality while addressing the static analysis concern and potentially improving code organization.
🧰 Tools
🪛 Biome
[error] 21-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
standalone/standalone/src/EggModuleLoader.ts (1)
27-39: LGTM:generateAppGraphmethod refactored with improved error handling.The new implementation streamlines the graph generation process and adds error handling for dumping module descriptors. This is a good improvement.
Consider using template literals for the error message for better readability:
- e.message = 'dump module descriptor failed: ' + e.message; + e.message = `dump module descriptor failed: ${e.message}`;core/metadata/src/model/graph/GlobalGraph.ts (2)
16-20: Consider planning for strict mode enforcementThe TODO comment indicates plans to enforce strict mode in the next major version. This change could have significant implications for the module's behavior and compatibility.
To prepare for this change:
- Document the planned strict mode behavior in the module's documentation.
- Consider adding a deprecation warning when
strictis set tofalse.- Plan for a migration guide to help users transition to strict mode.
1-247: Overall assessment of GlobalGraph implementationThe
GlobalGraphclass provides a robust implementation for managing module and proto dependencies. However, there are several areas where improvements could be made:
- Performance optimizations in methods like
#findDependencyProtoWithDefaultQualifiersand#findModuleNode.- Enhanced error messages for better debugging.
- Potential refactoring of complex methods like
buildfor improved readability.- Addressing TODO comments and unused properties.
These improvements would enhance the maintainability and efficiency of the
GlobalGraphclass, especially when dealing with large-scale applications.Consider creating a separate issue to track these improvements and prioritize them for future development iterations.
core/metadata/src/impl/ModuleLoadUnit.ts (2)
218-232: LGTM: Updated module loading mechanism with a suggestionThe changes to
doLoadClazzandloadClazzmethods are appropriate and align with the new approach of usingGlobalGraphfor module loading. The simplification ofloadClazzimproves code readability.Suggestion for improvement:
Enhance the TODO comment to provide more context:- // TODO ModuleLoadUnit should support all proto descriptor + // TODO: Extend ModuleLoadUnit to support all proto descriptor types, not just ClassProtoDescriptorThis change provides clearer guidance for future development.
250-252: LGTM with suggestion: Updated init methodThe changes to the
initmethod align well with the new approach of working with proto descriptors and usingEggPrototypeCreatorFactory. This approach is more flexible and potentially more extensible.Suggestion for improvement:
Add error handling to prevent potential unhandled promise rejections:async init() { this.loadClazz(); for (const protoDescriptor of this.protos) { - const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this); - EggPrototypeFactory.instance.registerPrototype(proto, this); + try { + const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this); + EggPrototypeFactory.instance.registerPrototype(proto, this); + } catch (error) { + console.error(`Failed to create or register prototype for ${protoDescriptor.name}:`, error); + // Consider whether to throw the error or handle it in another way + } } }This change ensures that any errors during prototype creation or registration are caught and logged, preventing unhandled promise rejections.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
- .gitignore (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/ModuleDescriptor.ts (1 hunks)
- core/metadata/src/model/graph/GlobalGraph.ts (1 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (2 hunks)
- standalone/standalone/src/EggModuleLoader.ts (1 hunks)
- standalone/standalone/src/Runner.ts (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- .gitignore
🧰 Additional context used
🪛 Biome
core/metadata/src/model/ModuleDescriptor.ts
[error] 21-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
standalone/standalone/src/EggModuleLoader.ts
[error] 58-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
standalone/standalone/src/Runner.ts
[error] 143-143: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (16)
plugin/tegg/lib/EggModuleLoader.ts (6)
4-4: LGTM: Import changes are consistent with class modificationsThe addition of
GlobalGraphandModuleDescriptorDumperimports aligns well with the changes made in theEggModuleLoaderclass.
12-12: LGTM: New property aligns with class modificationsThe addition of the
globalGraphproperty of typeGlobalGraphis consistent with the changes made in theEggModuleLoaderclass.
16-16: Avoid directly settingGlobalGraph.instancein the constructorDirectly assigning to
GlobalGraph.instancecan lead to unintended side effects and tight coupling between components. It's better practice to encapsulate instance management within theGlobalGraphclass or provide a method to set the instance.Consider updating the code to avoid external manipulation of
GlobalGraph.instance. For example, you might modifyGlobalGraphto have a static method for setting its instance.
25-44: LGTM: ImprovedbuildAppGraphmethodThe refactoring of the
buildAppGraphmethod is a significant improvement:
- It simplifies the process by using
LoaderFactory.loadAppfor module descriptors.- Implements error handling for descriptor dumping.
- Creates and builds a
GlobalGraphinstance.- Returns the created graph, which is then used to initialize the
globalGraphproperty.These changes align well with the PR objectives of shifting complexity into the Loader and Graph components.
48-49: Assign the new graph tothis.globalGraphafter rebuildingIn the
loadModule()method, you're callingthis.buildAppGraph()to rebuild the graph but not updatingthis.globalGraph. As a result,this.globalGraph.sort()may be operating on an outdated graph.Assign the new graph returned by
this.buildAppGraph()tothis.globalGraphto ensure you're using the updated graph.- this.buildAppGraph(); this.globalGraph.sort(); + this.globalGraph = this.buildAppGraph(); + this.globalGraph.sort();
53-53: Consider reusing loaders or optimizing loader creationIn the loop within
loadModule(), a new loader is being created for each module usingLoaderFactory.createLoader(). If loader creation is resource-intensive, consider whether loaders can be reused or optimized for better performance.Investigate whether loaders can be cached or whether the loader creation process can be optimized to improve efficiency.
core/metadata/src/model/ModuleDescriptor.ts (1)
1-6: LGTM: Improved imports and added flexibility for dump path.The changes to the import statements and the addition of the
DUMP_PATHconstant are good improvements:
- Using
fs/promisesaligns with the recommendation to use asynchronous file system operations.- The
DUMP_PATHconstant adds flexibility by allowing the dump location to be specified via an environment variable.These changes contribute to better performance and configurability.
standalone/standalone/src/EggModuleLoader.ts (5)
3-3: LGTM: Import statements updated correctly.The new imports for
GlobalGraph,ModuleDescriptorDumper, andLoggerare consistent with the changes in the class implementation.Also applies to: 7-7, 11-11
13-16: LGTM: New interfaceEggModuleLoaderOptionsadded.The new interface is well-defined and includes the necessary properties (
loggerandbaseDir) for the loader options.
44-45: LGTM:loadmethod updated to useGlobalGraphandLoaderFactory.The changes to the
loadmethod are consistent with the newGlobalGraphusage and improve the loading process by usingLoaderFactory.createLoader.Also applies to: 48-48
20-24:⚠️ Potential issueRefactor the multiple assignment in the constructor.
The changes to the constructor are consistent with the new
GlobalGraphusage. However, there's a multiple assignment in a single expression, which can reduce code readability.Apply the following change to improve clarity:
- GlobalGraph.instance = this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options); + this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options); + GlobalGraph.instance = this.globalGraph;
55-60:⚠️ Potential issueRefactor the
preLoadmethod for consistency and remove unused variables.The
preLoadmethod has been updated to useGlobalGraph, which is consistent with the changes in other parts of the class. However, there are a few issues to address:
- Multiple assignment in a single expression:
- const globalGraph = GlobalGraph.instance = EggModuleLoader.generateAppGraph(moduleReferences, options); + const globalGraph = EggModuleLoader.generateAppGraph(moduleReferences, options); + GlobalGraph.instance = globalGraph;
- Remove unused
loaderCache:- const loaderCache = new Map<string, Loader>();These changes will improve code clarity and remove unused variables.
🧰 Tools
🪛 Biome
[error] 58-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
core/metadata/src/model/graph/GlobalGraph.ts (1)
71-79:⚠️ Potential issueEnhance error messages for duplicate modules and protos
The current error messages for duplicate modules and protos lack specific information, which could make debugging difficult.
Consider improving the error messages by including more details:
-throw new Error(`duplicate module: ${moduleNode}`); +throw new Error(`Duplicate module detected: ${moduleNode.val.name}`); -throw new Error(`duplicate proto: ${protoNode.val}`); +throw new Error(`Duplicate proto detected: ${protoNode.val.proto.name}`);This change will provide more context about which specific module or proto is causing the duplication error.
Likely invalid or redundant comment.
core/metadata/src/impl/ModuleLoadUnit.ts (3)
5-8: LGTM: New imports added to support type definitionsThe addition of
GraphNodeObj,InitTypeQualifierAttribute, andObjectInitTypeLikeimports from '@eggjs/tegg-types' is appropriate. These new imports provide necessary type definitions for the changes made in theProtoNodeclass and elsewhere in the file.
212-212: LGTM: Simplified ModuleLoadUnit constructorThe removal of the
loaderparameter from the constructor is appropriate and consistent with the changes in the module loading approach. This simplification focuses the constructor on essential properties and aligns with the overall refactoring of theModuleLoadUnitclass.
198-211:⚠️ Potential issueApprove changes to ModuleLoadUnit properties and fix getter typo
The changes to the
ModuleLoadUnitclass properties and the addition of theglobalGraphgetter are appropriate and align with the new module loading mechanism. However, there's a typo in the getter name:
- Fix the typo in the getter name:
- get globalGrahp(): GlobalGraph { + get globalGraph(): GlobalGraph { return GlobalGraph.instance!; }
The new properties
protosandclazzListare correctly typed and support the updated functionality.Commenting out the
loaderproperty is consistent with the changes in the module loading approach.Likely invalid or redundant comment.
Let MultiInstanceProto be first class. In future: - impl loadProtoDesc for loader, not add proto in runtime - impl strict mode, check proto exists before boot - impl GlobalGraph dump/restore
killagu
force-pushed
the
chore/global_graph
branch
from
5c42012 to
11a7256
Compare
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (15)
core/metadata/src/model/ModuleDescriptor.ts (2)
8-19: LGTM: Interface updates enhance module descriptor functionality.The additions to the
ModuleDescriptorinterface and the newModuleDumpOptionsinterface improve the overall functionality and flexibility of the module descriptor system.Consider adding JSDoc comments to these interfaces to improve code documentation and provide better context for developers using these types.
46-55: LGTM: Improved dump functionality with async operations.The updates to
dumpPathanddumpmethods are good improvements:
- The use of
ModuleDumpOptionsprovides flexibility in configuring the dump path.- The
dumpmethod now uses asynchronous file system operations, which aligns with best practices for Node.js.Consider adding error handling to the
dumpmethod to gracefully handle potential file system errors:static async dump(desc: ModuleDescriptor, options?: ModuleDumpOptions) { const dumpPath = ModuleDescriptorDumper.dumpPath(desc, options); try { await fs.mkdir(path.dirname(dumpPath), { recursive: true }); await fs.writeFile(dumpPath, ModuleDescriptorDumper.stringifyDescriptor(desc)); } catch (error) { console.error(`Failed to dump module descriptor: ${error}`); throw error; // or handle it as appropriate for your use case } }This addition will make the method more robust and easier to debug in case of issues.
standalone/standalone/src/EggModuleLoader.ts (1)
27-39: LGTM: ImprovedgenerateAppGraphmethod with enhanced error handling.The refactored
generateAppGraphmethod aligns well with the PR objectives. It effectively usesLoaderFactory,ModuleDescriptorDumper, andGlobalGraphto streamline the graph generation process.Consider adding more context to the error message in the catch block:
- e.message = 'dump module descriptor failed: ' + e.message; + e.message = `Failed to dump module descriptor for ${moduleDescriptor.name}: ${e.message}`;This change would provide more specific information about which module descriptor failed to dump, aiding in debugging.
core/common-util/src/Graph.ts (1)
Line range hint
1-184: Overall assessment: Well-implemented metadata support for graph structures.The changes in this file successfully introduce metadata support to the graph implementation. The modifications are consistent across
GraphNode,GraphPath, andGraphclasses, enhancing the flexibility and power of the graph structure. The newEdgeMetainterface andfindToNodemethod are valuable additions that complement the metadata support.The implementation aligns well with the PR objective of enhancing the graph structure and minimizing modifications to other parts of the codebase. The changes are well-thought-out and maintain the existing functionality while adding new capabilities.
One area for future improvement is the potential stack overflow issue in the
appendVertexToPathmethod, as noted in a previous comment. Consider addressing this in a future iteration to enhance the robustness of the implementation.core/metadata/src/model/ProtoDescriptorHelper.ts (1)
21-181: Consider converting the class to a module with exported functionsWhile the implementation of individual methods is generally good, the overall structure of using a class with only static methods goes against TypeScript/JavaScript best practices. Consider refactoring this into a module with exported functions for better maintainability and adherence to language idioms.
Here's a high-level suggestion for refactoring:
- Remove the
class ProtoDescriptorHelperwrapper.- Export each method as a standalone function.
- For private methods (like
#createByMultiInstanceClazz), you can keep them as non-exported functions in the module.This approach would make the code more modular and easier to test and maintain. It would also address the static analysis hint about avoiding classes with only static members.
Would you like me to provide a more detailed refactoring suggestion for this architectural change?
🧰 Tools
🪛 Biome
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/src/model/graph/GlobalGraph.ts (1)
16-20: Consider documenting strict mode behaviorThe TODO comment suggests plans to enforce strict mode in a future version. It would be beneficial to document the current behavior of strict mode and how it affects the graph building process. This documentation will help users understand the implications of enabling or disabling strict mode.
Consider adding a comment explaining the current behavior of strict mode and its impact on the graph building process. This will provide clarity for developers using this class.
core/metadata/src/impl/ModuleLoadUnit.ts (2)
198-200: Clarify changes in module loading mechanismThe changes to the
ModuleLoadUnitclass properties suggest a significant shift in the module loading mechanism:
- The
loaderproperty has been commented out, indicating it's no longer used.- New properties
protos: ClassProtoDescriptor[]andclazzList: EggProtoImplClass[]have been added.Please provide comments explaining:
- The reason for removing the
loaderproperty.- The purpose and usage of the new
protosandclazzListproperties.- How these changes affect the overall module loading process.
This information will help future maintainers understand the new design and its implications.
212-212: Approve constructor change and update documentationThe removal of the
loaderparameter from the constructor is consistent with the earlier changes and simplifies the class initialization. This change is approved.However, please ensure that:
- All calls to the
ModuleLoadUnitconstructor throughout the codebase are updated to reflect this change.- Any documentation (including JSDoc comments, if present) for the
ModuleLoadUnitclass is updated to reflect the new constructor signature.core/metadata/test/LoadUnit.test.ts (3)
30-31: Approve: Addition of buildGlobalGraph callThe introduction of
buildGlobalGraphcall before creating the load unit aligns with the new approach of using GlobalGraph. This ensures proper setup of the global context.Consider adding an assertion to verify that the GlobalGraph is properly set up:
assert(GlobalGraph.instance, 'GlobalGraph should be initialized');
82-89: Approve: Enhanced error checking in initialization failure testThe test case has been improved to use
assert.rejectsandbuildGlobalGraph, which enhances error handling and aligns with the new approach. The specific error message checks add precision to the test.Consider extracting the error messages into constants for better maintainability:
const ERROR_MSG_1 = 'Object persistenceService not found'; const ERROR_MSG_2 = 'faq/TEGG_EGG_PROTO_NOT_FOUND'; assert(e.message.includes(ERROR_MSG_1)); assert(e.message.includes(ERROR_MSG_2));
177-192: Approve: Comprehensive setup for multi-instance testsThe changes introduce a more comprehensive setup for multi-instance tests, including multiple modules and loaders. This aligns well with the new approach using
buildGlobalGraph.However, the setup code is repetitive. Consider refactoring it into a helper function for better maintainability:
function setupMultiInstanceTest(modules: string[]): TestLoader[] { const loaders = modules.map(module => new TestLoader(module)); buildGlobalGraph(modules, loaders); return loaders; } // Usage const modules = [ appInstanceModule, app2InstanceModule, fooInstanceModule, barInstanceModule, ]; const loaders = setupMultiInstanceTest(modules); const [loader, loader2, fooLoader, barLoader] = loaders;standalone/standalone/src/Runner.ts (2)
142-145: Approve changes with a suggestion for simplificationThe initialization of
loadUnitLoaderwith additional configuration options (loggerandbaseDir) is a good improvement, enhancing its flexibility. However, theloggeroption can be simplified for better readability.Consider simplifying the
loggeroption using optional chaining:- logger: ((this.innerObjects.logger && this.innerObjects.logger[0])?.obj as Logger) || console, + logger: (this.innerObjects.logger?.[0]?.obj as Logger) || console,This change maintains the same functionality while improving code readability.
🧰 Tools
🪛 Biome
[error] 143-143: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
201-204: Approve changes with a suggestion for logging flexibilityThe update to
EggModuleLoader.preLoadwithbaseDirandloggerparameters is consistent with the changes in the constructor, which is good for maintaining consistency across the class.While using
consoleas the default logger is straightforward, consider enhancing flexibility by allowing the caller to provide a custom logger:static async preLoad(cwd: string, dependencies?: RunnerOptions['dependencies'], options?: { logger?: Logger }) { const moduleReferences = Runner.getModuleReferences(cwd, dependencies); await EggModuleLoader.preLoad(moduleReferences, { baseDir: cwd, logger: options?.logger || console, }); }This approach would allow users to inject their preferred logging mechanism while maintaining the current default behavior.
core/core-decorator/src/util/PrototypeUtil.ts (2)
149-174: LGTM: New methods added correctly with minor suggestionsThe new methods
getStaticMultiInstancePropertyandgetDynamicMultiInstancePropertyare well-implemented and follow the existing patterns in the class. However, consider the following suggestions:
- Enhance the documentation comments to provide more details about the return values and their structure.
- Consider adding error handling for potential undefined values, especially in
getDynamicMultiInstancePropertywhen accessingcallBackMetadata.getObjects(ctx).Example of improved documentation for
getDynamicMultiInstanceProperty:/** * Get instance property of Dynamic multi-instance prototype. * @param {EggProtoImplClass} clazz - The class to retrieve the property from. * @param {MultiInstancePrototypeGetObjectsContext} ctx - The context for getting objects. * @returns {EggMultiInstancePrototypeInfo | undefined} The multi-instance prototype info with dynamically generated objects, or undefined if not found. */
Line range hint
189-205: Acknowledge TODO for future removal of default qualifier logicThe added TODO comment indicates that the default qualifier logic will be removed in the next major version. This is good for maintaining backwards compatibility while planning for future changes.
To ensure this change is not forgotten, consider creating a tracking issue for removing this logic in the next major version. Would you like me to create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (36)
- .gitignore (1 hunks)
- core/common-util/src/Graph.ts (6 hunks)
- core/core-decorator/src/util/PrototypeUtil.ts (4 hunks)
- core/core-decorator/src/util/QualifierUtil.ts (1 hunks)
- core/loader/src/LoaderFactory.ts (2 hunks)
- core/metadata/index.ts (1 hunks)
- core/metadata/src/factory/EggPrototypeCreatorFactory.ts (2 hunks)
- core/metadata/src/impl/EggPrototypeBuilder.ts (1 hunks)
- core/metadata/src/impl/ModuleLoadUnit.ts (5 hunks)
- core/metadata/src/model/ModuleDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts (1 hunks)
- core/metadata/src/model/ProtoDescriptorHelper.ts (1 hunks)
- core/metadata/src/model/graph/GlobalGraph.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNode.ts (1 hunks)
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts (1 hunks)
- core/metadata/src/model/graph/ProtoNode.ts (1 hunks)
- core/metadata/src/model/graph/ProtoSelector.ts (1 hunks)
- core/metadata/test/GlobalGraph.test.ts (1 hunks)
- core/metadata/test/LoadUnit.test.ts (8 hunks)
- core/metadata/test/fixtures/LoaderUtil.ts (2 hunks)
- core/runtime/test/LoadUnitInstance.test.ts (2 hunks)
- core/runtime/test/util.ts (1 hunks)
- core/test-util/CoreTestHelper.ts (2 hunks)
- core/test-util/LoaderUtil.ts (2 hunks)
- core/types/common/ModuleConfig.ts (1 hunks)
- core/types/core-decorator/enum/MultiInstanceType.ts (1 hunks)
- core/types/core-decorator/index.ts (1 hunks)
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts (1 hunks)
- core/types/metadata/enum/ProtoDescriptorType.ts (1 hunks)
- core/types/metadata/index.ts (1 hunks)
- core/types/metadata/model/Loader.ts (1 hunks)
- core/types/metadata/model/ProtoDescriptor.ts (1 hunks)
- plugin/tegg/lib/EggModuleLoader.ts (2 hunks)
- standalone/standalone/src/EggModuleLoader.ts (1 hunks)
- standalone/standalone/src/Runner.ts (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (25)
- .gitignore
- core/core-decorator/src/util/QualifierUtil.ts
- core/metadata/index.ts
- core/metadata/src/impl/EggPrototypeBuilder.ts
- core/metadata/src/model/ProtoDescriptor/AbstractProtoDescriptor.ts
- core/metadata/src/model/ProtoDescriptor/ClassProtoDescriptor.ts
- core/metadata/src/model/graph/GlobalModuleNode.ts
- core/metadata/src/model/graph/GlobalModuleNodeBuilder.ts
- core/metadata/src/model/graph/ProtoNode.ts
- core/metadata/src/model/graph/ProtoSelector.ts
- core/metadata/test/GlobalGraph.test.ts
- core/metadata/test/fixtures/LoaderUtil.ts
- core/runtime/test/LoadUnitInstance.test.ts
- core/runtime/test/util.ts
- core/test-util/CoreTestHelper.ts
- core/test-util/LoaderUtil.ts
- core/types/common/ModuleConfig.ts
- core/types/core-decorator/enum/MultiInstanceType.ts
- core/types/core-decorator/index.ts
- core/types/core-decorator/model/EggMultiInstancePrototypeInfo.ts
- core/types/metadata/enum/ProtoDescriptorType.ts
- core/types/metadata/index.ts
- core/types/metadata/model/Loader.ts
- core/types/metadata/model/ProtoDescriptor.ts
- plugin/tegg/lib/EggModuleLoader.ts
🧰 Additional context used
🪛 Biome
core/metadata/src/factory/EggPrototypeCreatorFactory.ts
[error] 87-87: 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/metadata/src/model/ModuleDescriptor.ts
[error] 21-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
core/metadata/src/model/ProtoDescriptorHelper.ts
[error] 21-181: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
standalone/standalone/src/EggModuleLoader.ts
[error] 58-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
standalone/standalone/src/Runner.ts
[error] 143-143: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (31)
core/loader/src/LoaderFactory.ts (2)
1-3: LGTM: New imports are appropriate for the added functionality.The new imports for
ModuleDescriptor,PrototypeUtil, andEggProtoImplClassare necessary and correctly added to support the newloadAppmethod.
22-46: LGTM: Good integration with existing code.The
loadAppmethod is well-integrated into theLoaderFactoryclass. It appropriately uses the existingcreateLoadermethod, maintaining consistency with the current design and extending the class's functionality as intended by the PR objectives.core/metadata/src/model/ModuleDescriptor.ts (2)
1-6: LGTM: Import statements and constant declaration look good.The updated import statements and the addition of the
DUMP_PATHconstant using an environment variable are appropriate for the file's functionality and provide flexibility in configuration.
1-56: Overall assessment: Good improvements with some refactoring opportunities.The changes to
ModuleDescriptor.tsalign well with the PR objectives of transitioning complexity into the Loader and Graph components. The additions and modifications enhance the module descriptor functionality and provide more flexibility in handling module metadata.Key points:
- The new interfaces and properties improve the structure of module descriptors.
- The use of asynchronous operations in the
dumpmethod is a good practice.- There are opportunities for refactoring the
ModuleDescriptorDumperclass into separate functions for better maintainability and to align with static analysis suggestions.- JSON stringification can be improved for better performance and reliability.
Consider implementing the suggested refactoring and improvements to further enhance the code quality and maintainability of this module.
🧰 Tools
🪛 Biome
[error] 21-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
standalone/standalone/src/EggModuleLoader.ts (4)
3-3: LGTM: Import statements updated correctly.The new imports for
GlobalGraph,ModuleDescriptorDumper, andLoggerare consistent with the changes made in the file and support the new functionality.Also applies to: 7-7, 11-11
13-16: LGTM: New interfaceEggModuleLoaderOptionsadded.The new interface provides a clear structure for the options passed to the
EggModuleLoader, includingloggerandbaseDirproperties. This enhances type safety and improves code readability.
44-45: LGTM:loadmethod updated to useGlobalGraph.The
loadmethod has been successfully updated to use theglobalGraphfor sorting and retrieving themoduleConfigList. The removal ofloaderCacheand the use ofLoaderFactory.createLoaderare consistent with the PR objectives and the new implementation.Also applies to: 48-48
Line range hint
1-72: Overall improvements align well with PR objectives.The changes in this file successfully transition the complexity of MultiInstanceProto into the Loader and Graph components, as intended in the PR objectives. The introduction of
GlobalGraph, the refactoring ofgenerateAppGraph, and the updates toloadandpreLoadmethods all contribute to this goal.The new
EggModuleLoaderOptionsinterface and the updated constructor improve the configuration process. The error handling ingenerateAppGraphenhances the robustness of the module loading process.These changes lay a good foundation for future implementations mentioned in the PR description, such as loading protocol descriptions for the loader and implementing dump/restore functionality for GlobalGraph.
🧰 Tools
🪛 Biome
[error] 58-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
core/metadata/src/factory/EggPrototypeCreatorFactory.ts (1)
86-104: LGTM with minor suggestions.The implementation of
createProtoByDescriptorlooks good and follows a similar structure to the existingcreateProtomethod. However, there are a couple of suggestions for improvement:
Consider addressing the static analysis hint by replacing
thiswith the class nameEggPrototypeCreatorFactoryto avoid potential confusion in a static context.It would be helpful to add a comment explaining the difference between this method and
createProto, particularly highlighting that this method deals with a single prototype based on a descriptor, whilecreateProtocan handle multiple prototypes.Here's a suggested implementation addressing these points:
/** * Creates a single EggPrototype based on a ClassProtoDescriptor. * Unlike createProto, this method always returns a single prototype. */ static async createProtoByDescriptor(protoDescriptor: ClassProtoDescriptor, loadUnit: LoadUnit): Promise<EggPrototype> { const creator = EggPrototypeCreatorFactory.getPrototypeCreator(protoDescriptor.protoImplType); if (!creator) { throw new Error(`not found proto creator for type: ${protoDescriptor.protoImplType}`); } // ... rest of the method remains the same }Let's verify the usage of this new method in the codebase:
🧰 Tools
🪛 Biome
[error] 87-87: 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/common-util/src/Graph.ts (8)
5-8: LGTM: New EdgeMeta interface.The new
EdgeMetainterface is well-defined and provides a clear contract for edge metadata. It includes methods for equality comparison and string representation, which are essential for proper metadata handling in the graph structure.
10-10: LGTM: GraphNode class updated to support metadata.The
GraphNodeclass has been successfully updated to include metadata support. The changes to the class signature and thetoNodeMapandfromNodeMapproperties are well-implemented, allowing for the storage of both connected nodes and their associated metadata.Also applies to: 12-13
23-27: LGTM: addToVertex and addFromVertex methods updated.The
addToVertexandaddFromVertexmethods have been correctly updated to accept and store metadata along with the node connections. The changes are consistent with the updates to the class properties and maintain the existing behavior of preventing duplicate connections.Also applies to: 31-35
56-56: LGTM: GraphPath class updated to support metadata.The
GraphPathclass has been successfully updated to include metadata support. The changes to the class signature,nodesproperty, and thepushVertexandpopVertexmethods are well-implemented. ThetoStringmethod has been appropriately updated to include metadata representation.Also applies to: 58-58, 60-63, 68-71, 77-84, 87-87
95-96: LGTM: Graph class signature and addEdge method updated.The
Graphclass signature has been correctly updated to include the metadata generic type. TheaddEdgemethod now properly accepts and propagates metadata when establishing connections between nodes. These changes are consistent with the updates made to theGraphNodeandGraphPathclasses.Also applies to: 106-109
111-119: LGTM: New findToNode method added.The new
findToNodemethod is a valuable addition to theGraphclass. It correctly implements the logic to find a specific 'to' node based on both ID and metadata, enhancing the graph's functionality by allowing metadata-based node retrieval.
122-127: LGTM with a note: appendVertexToPath method updated.The
appendVertexToPathmethod has been correctly updated to handle and propagate metadata during graph traversal. These changes are consistent with the updates to other parts of the class.However, as noted in a previous review comment, there's still a potential issue with deep recursion that could lead to a stack overflow. Consider addressing this in a future iteration by refactoring to an iterative approach.
135-136: LGTM: Type annotations updated in loopPath, accessNode, and sort methods.The
loopPath,accessNode, andsortmethods have been correctly updated to use the new generic typeM extends EdgeMeta. These changes ensure type consistency with the rest of the class while maintaining the existing functionality of these methods.Also applies to: 146-146, 157-157, 171-172
core/metadata/src/model/ProtoDescriptorHelper.ts (1)
1-20: LGTM: Imports and dependencies are well-organizedThe imports are properly structured and grouped, which enhances readability. The use of specific imports from '@eggjs/core-decorator' and '@eggjs/tegg-types' suggests good modularization of the codebase.
core/metadata/src/model/graph/GlobalGraph.ts (5)
62-63: Remove or implement unused static propertyThe static property
instance?is declared but never used within the class. This could lead to confusion about the intended usage of theGlobalGraphclass.Consider one of the following actions:
- If a singleton pattern is intended, implement it fully.
- If not needed, remove the property to improve code clarity.
-static instance?: GlobalGraph;
72-79: Improve error messages for duplicate modules and protosThe error messages for duplicate modules and protos may not provide sufficient information if
moduleNodeandprotoNode.valdo not have meaningful string representations. Including specific identifiers like module names or IDs will make the error messages more informative.Apply the following diff to enhance the error messages:
-throw new Error(`duplicate module: ${moduleNode}`); +throw new Error(`Duplicate module detected: ${moduleNode.val.name}`); -throw new Error(`duplicate proto: ${protoNode.val}`); +throw new Error(`Duplicate proto detected: ${protoNode.val.proto.name}`);
82-109: Consider refactoring thebuildmethod for improved readabilityThe
buildmethod contains complex nested logic, which may make it difficult to understand and maintain.Consider breaking down the method into smaller, more focused helper methods. For example:
- Create a method to handle finding and connecting proto dependencies.
- Create a method to handle module dependency connections.
- Extract the error handling logic into separate methods.
This refactoring would improve the method's readability and make it easier to test individual components of the build process.
118-135: Optimize#findDependencyProtoWithDefaultQualifiersfor better performanceThe current implementation has a time complexity of O(n^2), which could lead to performance issues with large numbers of protos.
Consider optimizing this method:
- Use a more efficient data structure for storing and querying protos, such as a Map or Set indexed by relevant attributes.
- Implement caching mechanisms to store frequently accessed results.
- If possible, precompute and store compatibility information during the graph building phase.
Example optimization using a Map:
private protoMap: Map<string, GraphNode<ProtoNode, ProtoDependencyMeta>[]> = new Map(); // In the build method or when adding protos this.protoMap.set(protoKey, compatibleProtos); // In #findDependencyProtoWithDefaultQualifiers return this.protoMap.get(protoKey) || [];This change could significantly improve performance for large graphs.
180-186: Optimize#findModuleNodefor better performanceThe current implementation of
#findModuleNodeuses a linear search, which can be inefficient for a large number of modules.Consider using a
Mapto store module nodes, keyed by module name. This would allow for constant-time lookups:private moduleMap: Map<string, GraphNode<GlobalModuleNode, ModuleDependencyMeta>> = new Map(); // In the addModuleNode method this.moduleMap.set(moduleNode.val.name, moduleNode); // Replace the #findModuleNode method #findModuleNode(moduleName: string) { return this.moduleMap.get(moduleName); }This change would improve the performance of module lookups, especially in larger graphs.
core/metadata/src/impl/ModuleLoadUnit.ts (1)
67-67:⚠️ Potential issueUse imported PrototypeUtil instead of instance property
The
toStringmethod now usesthis.PrototypeUtilinstead of the importedPrototypeUtil. This change introduces inconsistency and may lead to maintenance issues. Consider reverting to using the importedPrototypeUtil:- return `${this.clazz.name}@${this.PrototypeUtil.getFilePath(this.clazz)}`; + return `${this.clazz.name}@${PrototypeUtil.getFilePath(this.clazz)}`;This change ensures consistency with how
PrototypeUtilis used elsewhere in the codebase and eliminates the need for the privatePrototypeUtilproperty.Likely invalid or redundant comment.
core/metadata/test/LoadUnit.test.ts (5)
21-23: Approve: GlobalGraph instance reset before each testThe addition of a
beforeEachhook to resetGlobalGraph.instancetoundefinedensures a clean state for each test, which is a good practice for test isolation.
53-54: Approve: Consistent use of buildGlobalGraphThe addition of
buildGlobalGraphcall before creating the load unit is consistent with the new approach and ensures proper setup of the global context.
72-74: Approve: Improved error handling in recursive deps testThe test case has been rewritten to use
assert.rejectsandbuildGlobalGraph, which improves error handling and more accurately tests for recursive dependencies at the graph building stage. This change enhances the test's effectiveness and readability.
95-100: Approve: Consistent improvement in multi-proto initialization failure testThe test case has been updated to use
assert.rejectsandbuildGlobalGraph, which is consistent with the changes in other tests. This modification improves error handling and more accurately tests for duplicate proto issues at the graph building stage.
3-9: Significant changes in import statementsThe import statements have been updated to include
GlobalGraphandbuildGlobalGraph, while removing imports related toApp,App2,BizManager, andSecret. This change suggests a shift in the test's focus towards global graph management.To ensure these changes don't introduce unused imports, run the following command:
Also applies to: 13-17
core/core-decorator/src/util/PrototypeUtil.ts (2)
10-12: LGTM: Import statements updated correctlyThe new imports
MultiInstancePrototypeGetObjectsContextandMultiInstanceTypehave been added, which are necessary for the new methods in the class. The change is appropriate and doesn't introduce any issues.
62-75:⚠️ Potential issueUpdate method documentation to match implementation
The current documentation for
getEggMultiInstancePrototypeTypeis incomplete and doesn't accurately reflect its functionality. Please update the documentation to match the implementation.Apply this diff to improve the documentation:
/** - * Get the type of the egg multi-instance prototype. - * @param {Function} clazz - + * Get the type of the egg multi-instance prototype. + * @param {EggProtoImplClass} clazz - The class to check. + * @returns {MultiInstanceType | undefined} The type of the multi-instance prototype (STATIC or DYNAMIC), or undefined if not a multi-instance prototype. */
| static loadApp(moduleReferences: readonly ModuleReference[]): ModuleDescriptor[] { | ||
| const result: ModuleDescriptor[] = []; | ||
| const multiInstanceClazzList: EggProtoImplClass[] = []; | ||
| for (const moduleReference of moduleReferences) { | ||
| const loader = LoaderFactory.createLoader(moduleReference.path, moduleReference.loaderType || EggLoadUnitType.MODULE); | ||
| const res: ModuleDescriptor = { | ||
| name: moduleReference.name, | ||
| unitPath: moduleReference.path, | ||
| clazzList: [], | ||
| protos: [], | ||
| multiInstanceClazzList, | ||
| optional: moduleReference.optional, | ||
| }; | ||
| result.push(res); | ||
| const clazzList = loader.load(); | ||
| for (const clazz of clazzList) { | ||
| if (PrototypeUtil.isEggPrototype(clazz)) { | ||
| res.clazzList.push(clazz); | ||
| } else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) { | ||
| res.multiInstanceClazzList.push(clazz); | ||
| } | ||
| } | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor the loadApp method to address potential issues and improve maintainability.
While the method implements the core functionality described in the PR objectives, there are several areas for improvement:
- Initialize
multiInstanceClazzListinside the loop to avoid shared state issues. - Add error handling around
loader.load()to gracefully handle potential exceptions. - Add JSDoc comments to improve documentation and maintainability.
- Consider handling classes that are neither single-instance nor multi-instance prototypes.
Here's a suggested refactor addressing these points:
/**
* Load application modules and return their descriptors.
* @param moduleReferences - An array of module references to load.
* @returns An array of ModuleDescriptor objects representing loaded modules.
*/
static loadApp(moduleReferences: readonly ModuleReference[]): ModuleDescriptor[] {
const result: ModuleDescriptor[] = [];
for (const moduleReference of moduleReferences) {
const multiInstanceClazzList: EggProtoImplClass[] = [];
const loader = LoaderFactory.createLoader(moduleReference.path, moduleReference.loaderType || EggLoadUnitType.MODULE);
const res: ModuleDescriptor = {
name: moduleReference.name,
unitPath: moduleReference.path,
clazzList: [],
protos: [],
multiInstanceClazzList,
optional: moduleReference.optional,
};
result.push(res);
try {
const clazzList = loader.load();
for (const clazz of clazzList) {
if (PrototypeUtil.isEggPrototype(clazz)) {
res.clazzList.push(clazz);
} else if (PrototypeUtil.isEggMultiInstancePrototype(clazz)) {
res.multiInstanceClazzList.push(clazz);
} else {
console.warn(`Unhandled class type: ${clazz.name}`);
}
}
} catch (error) {
console.error(`Failed to load module at ${moduleReference.path}:`, error);
// Decide whether to continue or throw the error based on your error handling strategy
}
}
return result;
}This refactored version addresses the identified issues and improves the overall robustness of the method.
| export class ModuleDescriptorDumper { | ||
| static stringifyDescriptor(moduleDescriptor: ModuleDescriptor): string { | ||
| return '{' | ||
| + `"name": "${moduleDescriptor.name}",` | ||
| + `"unitPath": "${moduleDescriptor.unitPath}",` | ||
| + (typeof moduleDescriptor.optional !== 'undefined' ? `"optional": ${moduleDescriptor.optional},` : '') | ||
| + `"clazzList": [${moduleDescriptor.clazzList.map(t => { | ||
| return ModuleDescriptorDumper.stringifyClazz(t, moduleDescriptor); | ||
| }).join(',')}],` | ||
| + `"multiInstanceClazzList": [${moduleDescriptor.multiInstanceClazzList.map(t => { | ||
| return ModuleDescriptorDumper.stringifyClazz(t, moduleDescriptor); | ||
| }).join(',')}],` | ||
| + `"protos": [${moduleDescriptor.protos.map(t => { | ||
| return JSON.stringify(t); | ||
| }).join(',')}]` | ||
| + '}'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor class and improve JSON stringification.
While the ModuleDescriptorDumper class provides useful functionality, consider the following improvements:
- Refactor the class into separate functions to align with the static analysis suggestion and improve maintainability.
- Use
JSON.stringifywith a replacer function instead of string concatenation for better performance and reliability.
Here's a suggested refactoring:
export function stringifyDescriptor(moduleDescriptor: ModuleDescriptor): string {
return JSON.stringify(moduleDescriptor, (key, value) => {
if (key === 'clazzList' || key === 'multiInstanceClazzList') {
return value.map(clazz => stringifyClazz(clazz, moduleDescriptor));
}
return value;
}, 2);
}This refactoring simplifies the code, makes it more maintainable, and potentially improves performance by leveraging JSON.stringify.
🧰 Tools
🪛 Biome
[error] 21-56: Avoid classes that contain only static members.
Prefer using simple functions instead of classes with only static members.
(lint/complexity/noStaticOnlyClass)
| static stringifyClazz(clazz: EggProtoImplClass, moduleDescriptor: ModuleDescriptor): string { | ||
| return '{' | ||
| + `"name": "${clazz.name}",` | ||
| + (PrototypeUtil.getFilePath(clazz) ? `"filePath": "${path.relative(moduleDescriptor.unitPath, PrototypeUtil.getFilePath(clazz)!)}"` : '') | ||
| + '}'; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Align stringifyClazz with the refactoring suggestion.
To maintain consistency with the previous refactoring suggestion and to improve efficiency, consider modifying this method as follows:
export function stringifyClazz(clazz: EggProtoImplClass, moduleDescriptor: ModuleDescriptor): object {
const result: any = { name: clazz.name };
const filePath = PrototypeUtil.getFilePath(clazz);
if (filePath) {
result.filePath = path.relative(moduleDescriptor.unitPath, filePath);
}
return result;
}This change returns an object instead of a string, allowing it to be used more flexibly in conjunction with JSON.stringify in the stringifyDescriptor function. This approach is more efficient and less prone to errors in JSON formatting.
| constructor(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions) { | ||
| this.moduleReferences = moduleReferences; | ||
| GlobalGraph.instance = this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options); |
There was a problem hiding this comment.
Simplify the assignment in the constructor.
The constructor has been updated to accept the new options parameter, which is good. However, there's still a multiple assignment in a single expression, which can reduce readability.
Apply this change to improve clarity:
- GlobalGraph.instance = this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options);
+ this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options);
+ GlobalGraph.instance = this.globalGraph;This change separates the assignments, making the code easier to understand and maintain.
📝 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.
| constructor(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions) { | |
| this.moduleReferences = moduleReferences; | |
| GlobalGraph.instance = this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options); | |
| constructor(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions) { | |
| this.moduleReferences = moduleReferences; | |
| this.globalGraph = EggModuleLoader.generateAppGraph(this.moduleReferences, options); | |
| GlobalGraph.instance = this.globalGraph; |
| static async preLoad(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions): Promise<void> { | ||
| const loadUnits: LoadUnit[] = []; | ||
| const loaderCache = new Map<string, Loader>(); | ||
| const appGraph = EggModuleLoader.generateAppGraph(loaderCache, moduleReferences); | ||
| appGraph.sort(); | ||
| const moduleConfigList = appGraph.moduleConfigList; | ||
| const globalGraph = GlobalGraph.instance = EggModuleLoader.generateAppGraph(moduleReferences, options); | ||
| globalGraph.sort(); | ||
| const moduleConfigList = globalGraph.moduleConfigList; | ||
| for (const moduleConfig of moduleConfigList) { | ||
| const modulePath = moduleConfig.path; | ||
| const loader = loaderCache.get(modulePath)!; |
There was a problem hiding this comment.
Update preLoad method for consistency and remove unused variables.
The preLoad method has been updated to use GlobalGraph, which is good. However, there are a few issues to address:
- Remove the unused
loaderCache:
- const loaderCache = new Map<string, Loader>();- Simplify the assignment:
- const globalGraph = GlobalGraph.instance = EggModuleLoader.generateAppGraph(moduleReferences, options);
+ const globalGraph = EggModuleLoader.generateAppGraph(moduleReferences, options);
+ GlobalGraph.instance = globalGraph;- Update the loader retrieval to use
LoaderFactory:
- const loader = loaderCache.get(modulePath)!;
+ const loader = LoaderFactory.createLoader(modulePath, EggLoadUnitType.MODULE);These changes will improve code consistency, remove unused variables, and align with the new implementation using GlobalGraph and LoaderFactory.
📝 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 async preLoad(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions): Promise<void> { | |
| const loadUnits: LoadUnit[] = []; | |
| const loaderCache = new Map<string, Loader>(); | |
| const appGraph = EggModuleLoader.generateAppGraph(loaderCache, moduleReferences); | |
| appGraph.sort(); | |
| const moduleConfigList = appGraph.moduleConfigList; | |
| const globalGraph = GlobalGraph.instance = EggModuleLoader.generateAppGraph(moduleReferences, options); | |
| globalGraph.sort(); | |
| const moduleConfigList = globalGraph.moduleConfigList; | |
| for (const moduleConfig of moduleConfigList) { | |
| const modulePath = moduleConfig.path; | |
| const loader = loaderCache.get(modulePath)!; | |
| static async preLoad(moduleReferences: readonly ModuleReference[], options: EggModuleLoaderOptions): Promise<void> { | |
| const loadUnits: LoadUnit[] = []; | |
| const globalGraph = EggModuleLoader.generateAppGraph(moduleReferences, options); | |
| GlobalGraph.instance = globalGraph; | |
| globalGraph.sort(); | |
| const moduleConfigList = globalGraph.moduleConfigList; | |
| for (const moduleConfig of moduleConfigList) { | |
| const modulePath = moduleConfig.path; | |
| const loader = LoaderFactory.createLoader(modulePath, EggLoadUnitType.MODULE); |
🧰 Tools
🪛 Biome
[error] 58-58: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
| GraphNodeObj, | ||
| InitTypeQualifierAttribute, | ||
| LoadUnitNameQualifierAttribute, | ||
| ObjectInitTypeLike, | ||
| } from '@eggjs/tegg-types'; |
There was a problem hiding this comment.
Fix typo in import and approve new imports
The new imports are necessary for the changes in the ModuleLoadUnit class. However, there's a typo in one of the imports:
- Fix the typo in
FrameworkErrorFormater:
-import { FrameworkErrorFormater } from 'egg-errors';
+import { FrameworkErrorFormatter } from 'egg-errors';- The other new imports (
GraphNodeObj,ObjectInitTypeLike,PrototypeUtil,QualifierUtil,ClassProtoDescriptor, andGlobalGraph) are appropriate and support the new functionality in the file.
Also applies to: 23-26
| readonly id: string; | ||
| readonly qualifiers: QualifierInfo[]; | ||
| readonly initType: ObjectInitTypeLike; | ||
| private PrototypeUtil: any; |
There was a problem hiding this comment.
Reconsider the addition of private PrototypeUtil property
The addition of private PrototypeUtil: any; raises concerns:
- Using
anytype bypasses TypeScript's type checking, which is generally discouraged. PrototypeUtilis already imported at the top of the file, so it's unclear why this property is needed.
Consider one of the following options:
a) If the property is necessary, provide a more specific type than any.
b) If it's not needed, remove the property and use the imported PrototypeUtil directly.
| get globalGraph(): GlobalGraph { | ||
| return GlobalGraph.instance!; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider safer access to GlobalGraph instance
The new globalGraph getter uses a non-null assertion (!) when accessing GlobalGraph.instance. This approach might lead to runtime errors if the instance is not initialized when accessed.
Consider using a safer alternative:
get globalGraph(): GlobalGraph {
const instance = GlobalGraph.instance;
if (!instance) {
throw new Error('GlobalGraph instance is not initialized');
}
return instance;
}This change provides a more explicit error message if the instance is not available, making debugging easier.
| for (const protoDescriptor of this.protos) { | ||
| const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this); | ||
| EggPrototypeFactory.instance.registerPrototype(proto, this); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling to init method
The init method has been updated to use this.protos and EggPrototypeCreatorFactory. While this change is consistent with the new proto-based approach, it lacks error handling for the asynchronous operations.
Consider adding try-catch blocks to handle potential errors during prototype creation:
async init() {
this.loadClazz();
for (const protoDescriptor of this.protos) {
try {
const proto = await EggPrototypeCreatorFactory.createProtoByDescriptor(protoDescriptor, this);
EggPrototypeFactory.instance.registerPrototype(proto, this);
} catch (error) {
console.error(`Failed to create prototype for ${protoDescriptor.name}:`, error);
// Consider whether to throw the error or continue with the next prototype
}
}
}This change will make the initialization process more robust and easier to debug.
| private doLoadClazz() { | ||
| const protos = this.globalGraph.moduleProtoDescriptorMap.get(this.name); | ||
| if (protos) { | ||
| // TODO ModuleLoadUnit should support all proto descriptor | ||
| this.protos = protos!.filter(t => ClassProtoDescriptor.isClassProtoDescriptor(t)); | ||
| this.clazzList = this.protos.map(t => t.clazz); | ||
| } else { | ||
| this.protos = []; | ||
| this.clazzList = []; | ||
| } | ||
| return result; | ||
| } | ||
|
|
||
| private loadClazz() { | ||
| if (!this.clazzList) { | ||
| const clazzList = this.doLoadClazz(); | ||
| const protoGraph = new ModuleGraph(clazzList, this.unitPath, this.name); | ||
| protoGraph.sort(); | ||
| this.clazzList = protoGraph.clazzList; | ||
| this.doLoadClazz(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review changes in class loading mechanism
The doLoadClazz and loadClazz methods have been significantly refactored:
doLoadClazznow usesglobalGraphto retrieve module proto descriptors.- It filters for
ClassProtoDescriptortypes, suggesting limited support for other descriptor types. loadClazzhas been simplified to just calldoLoadClazz.
Suggestions:
- Add a comment explaining why only
ClassProtoDescriptortypes are being filtered. - Consider handling cases where
protosmight be undefined more explicitly. - The TODO comment suggests future work. Consider creating a GitHub issue to track this enhancement.
private doLoadClazz() {
const protos = this.globalGraph.moduleProtoDescriptorMap.get(this.name);
if (protos) {
// TODO ModuleLoadUnit should support all proto descriptor
+ // Currently, we only support ClassProtoDescriptor types
this.protos = protos.filter(t => ClassProtoDescriptor.isClassProtoDescriptor(t));
this.clazzList = this.protos.map(t => t.clazz);
} else {
+ console.warn(`No proto descriptors found for module: ${this.name}`);
this.protos = [];
this.clazzList = [];
}
}Committable suggestion was skipped due to low confidence.
|
Let MultiInstanceProto be first class. In future:
Checklist
npm testpassesAffected core subsystem(s)
Description of change
Summary by CodeRabbit
Release Notes
New Features
ModuleDumpOptionsandEggModuleLoaderOptions, for enhanced configuration.Improvements
GlobalGraph.Runnerclass to support better logging and directory management.LoaderUtilwith new functions for building module nodes and global graphs.Bug Fixes
Documentation