refactor(enhanced): export getAllReferencedModules as a util #2937
Conversation
|
![squadronai[bot]](https://avatars.githubusercontent.com/in/901096?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary
This pull request refactors the getAllReferencedModules function by moving it from a class method to a standalone utility function. The primary goal of this change is to improve the reusability and maintainability of this functionality across the codebase.
The key changes include:
- Extracting the
getAllReferencedModulesfunction from theHoistContainerReferencesPluginclass and making it a standalone utility function. - Updating the
FederationRuntimePluginclass to use the new utility function instead of the previous class method implementation.
These changes will make the getAllReferencedModules functionality more easily accessible and usable in other parts of the codebase, enhancing the overall code organization and flexibility.
File Summaries
| File | Summary |
|---|---|
| packages/enhanced/src/lib/container/HoistContainerReferencesPlugin.ts | The code changes refactor the getAllReferencedModules function by moving it from a class method to a standalone utility function. This change allows the function to be reused more easily across different parts of the codebase. |
| packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts | The changes in this file involve refactoring the getAllReferencedModules functionality into a utility function, making it more reusable and easier to maintain. The primary purpose of this change is to improve the overall code structure and organization within the FederationRuntimePlugin class. |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Incremental Review
Comments posted: 3
Configuration
Squadron Mode: essential
Commits Reviewed
760cc35c53dd9beb7ebba20b729108b2c61b0966...57409e5aa0e23a624e9635ffc858458790522f55
Files Reviewed
- packages/enhanced/src/lib/container/HoistContainerReferencesPlugin.ts
| // Helper method to collect all referenced modules recursively | ||
| export function getAllReferencedModules( | ||
| compilation: Compilation, | ||
| module: Module, | ||
| type?: 'all' | 'initial', | ||
| ): Set<Module> { | ||
| const collectedModules = new Set<Module>([module]); | ||
| const stack = [module]; | ||
|
|
||
| while (stack.length > 0) { | ||
| const currentModule = stack.pop(); | ||
| if (!currentModule) continue; | ||
| const mgm = compilation.moduleGraph._getModuleGraphModule(currentModule); | ||
| if (mgm && mgm.outgoingConnections) { | ||
| for (const connection of mgm.outgoingConnections) { | ||
| if (type === 'initial') { | ||
| const parentBlock = compilation.moduleGraph.getParentBlock( | ||
| connection.dependency, | ||
| ); | ||
| if (parentBlock instanceof AsyncDependenciesBlock) { | ||
| continue; | ||
| } | ||
| } | ||
| if (connection.module && !collectedModules.has(connection.module)) { | ||
| collectedModules.add(connection.module); | ||
| stack.push(connection.module); | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| return collectedModules; | ||
| } |
There was a problem hiding this comment.
The getAllReferencedModules function has been extracted as a standalone utility function, which is a good refactoring. However, there are a few improvements that could be made:
-
Consider adding type annotations for the
compilation,module, andtypeparameters to improve readability and type safety. -
The function uses a depth-first search algorithm to collect modules. For large module graphs, this could potentially lead to stack overflow issues. Consider using an iterative approach instead of a recursive one.
-
The function modifies the
collectedModulesset while iterating over it, which could lead to unexpected behavior. Consider using a separate set for newly discovered modules. -
The
typeparameter is only used for the 'initial' case. Consider renaming it toinitialOnlyor similar to make its purpose clearer.
Here's an example of how you could refactor this function:
| // Helper method to collect all referenced modules recursively | |
| export function getAllReferencedModules( | |
| compilation: Compilation, | |
| module: Module, | |
| type?: 'all' | 'initial', | |
| ): Set<Module> { | |
| const collectedModules = new Set<Module>([module]); | |
| const stack = [module]; | |
| while (stack.length > 0) { | |
| const currentModule = stack.pop(); | |
| if (!currentModule) continue; | |
| const mgm = compilation.moduleGraph._getModuleGraphModule(currentModule); | |
| if (mgm && mgm.outgoingConnections) { | |
| for (const connection of mgm.outgoingConnections) { | |
| if (type === 'initial') { | |
| const parentBlock = compilation.moduleGraph.getParentBlock( | |
| connection.dependency, | |
| ); | |
| if (parentBlock instanceof AsyncDependenciesBlock) { | |
| continue; | |
| } | |
| } | |
| if (connection.module && !collectedModules.has(connection.module)) { | |
| collectedModules.add(connection.module); | |
| stack.push(connection.module); | |
| } | |
| } | |
| } | |
| } | |
| return collectedModules; | |
| } | |
| export function getAllReferencedModules( | |
| compilation: Compilation, | |
| module: Module, | |
| initialOnly: boolean = false | |
| ): Set<Module> { | |
| const collectedModules = new Set<Module>([module]); | |
| const stack: Module[] = [module]; | |
| while (stack.length > 0) { | |
| const currentModule = stack.pop()!; | |
| const mgm = compilation.moduleGraph._getModuleGraphModule(currentModule); | |
| if (mgm && mgm.outgoingConnections) { | |
| for (const connection of mgm.outgoingConnections) { | |
| if (initialOnly) { | |
| const parentBlock = compilation.moduleGraph.getParentBlock(connection.dependency); | |
| if (parentBlock instanceof AsyncDependenciesBlock) { | |
| continue; | |
| } | |
| } | |
| if (connection.module && !collectedModules.has(connection.module)) { | |
| collectedModules.add(connection.module); | |
| stack.push(connection.module); | |
| } | |
| } | |
| } | |
| } | |
| return collectedModules; | |
| } |
This refactored version uses an iterative approach, which is less likely to cause stack overflow issues. It also clarifies the purpose of the type parameter by renaming it to initialOnly.
| module instanceof NormalModule && | ||
| module.resource === this.bundlerRuntimeDep | ||
| ) { | ||
| const allRefs = this.getAllReferencedModules( | ||
| const allRefs = getAllReferencedModules( | ||
| compilation, | ||
| module, | ||
| 'initial', |
There was a problem hiding this comment.
The usage of getAllReferencedModules here looks correct. However, it's worth noting that the function call is incomplete in the provided snippet. Make sure the function call is completed correctly in the actual code. Also, consider adding a comment explaining why you're using 'initial' as the third argument, as it might not be immediately clear to other developers.
| return; | ||
| } | ||
|
|
||
| const allReferencedModules = this.getAllReferencedModules( | ||
| const allReferencedModules = getAllReferencedModules( | ||
| compilation, | ||
| runtimeModule, | ||
| 'initial', |
There was a problem hiding this comment.
Similar to the previous comment, the usage of getAllReferencedModules here is correct. However, make sure the function call is completed properly in the actual code. It might be helpful to add a brief comment explaining the purpose of this call and why you're using 'initial' as the third argument.
There was a problem hiding this comment.
Incremental Review
Comments posted: 1
Configuration
Squadron Mode: essential
Commits Reviewed
57409e5aa0e23a624e9635ffc858458790522f55...88aca7017071000f0546a218e9b20fb959d2c97c
Files Reviewed
- packages/enhanced/src/lib/container/HoistContainerReferencesPlugin.ts
- packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
| // hoist all modules of federation entry | ||
| this.getFilePath(), | ||
| this.bundlerRuntimePath, | ||
| this.options.experiments, | ||
| ).apply(compiler); | ||
|
|
||
| new compiler.webpack.NormalModuleReplacementPlugin( |
There was a problem hiding this comment.
The code snippet shows the usage of getAllReferencedModules, which is now likely a utility function. This change aligns with the commit message to export it as a utility. It's a good practice to modularize and separate concerns, making the code more maintainable and reusable.
If there are other parts of the codebase that used getAllReferencedModules as a class method, ensure they've been updated to use the new utility function to maintain consistency across the project.
ScriptedAlchemy
changed the title
Description
Export getAllReferencedModules as a utility and not a class method
Related Issue
Types of changes
Checklist