Skip to content

refactor(enhanced): export getAllReferencedModules as a util #2937

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Sep 9, 2024
Merged

refactor(enhanced): export getAllReferencedModules as a util #2937

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
57409e5
refactor: export getAllReferencedModules as a util
ScriptedAlchemy Sep 9, 2024
88aca70
fix: pass experiments to hoist plugin
ScriptedAlchemy Sep 9, 2024
55902f5
fix: pass experiments to hoist plugin
ScriptedAlchemy Sep 9, 2024
bee69ee
ci: use wait on in ci for devtools
ScriptedAlchemy Sep 9, 2024
94ca9af
ci: use wait on in ci for devtools
ScriptedAlchemy Sep 9, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion .github/workflows/devtools.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ jobs:
run: sudo apt-get update && sudo apt-get install xvfb

- name: E2E Chrome Devtools
run: pnpm run app:manifest:dev & echo "done" && sleep 50 && npx nx e2e:devtools chrome-devtools
run: pnpm run app:manifest:dev & echo "done" && npx wait-on tcp:3009 tcp:3010 tcp:3011 tcp:3012 && sleep 10 && npx nx e2e:devtools chrome-devtools

- name: kill port
run: lsof -ti tcp:3008,3009,3010,3011,3012 | xargs kill
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@
"app:node:dev": "nx run-many --target=serve --parallel=10 --configuration=development -p node-host,node-local-remote,node-remote,node-dynamic-remote-new-version,node-dynamic-remote",
"app:runtime:dev": "nx run-many --target=serve -p 3005-runtime-host,3006-runtime-remote,3007-runtime-remote",
"app:router:dev": "nx run-many --target=serve --parallel=10 --projects='router-*'",
"app:manifest:dev": "kill-port 3009 3010 3011 3012 3013 && nx run-many --target=serve --parallel=100 -p manifest-webpack-host,3009-webpack-provider,3010-rspack-provider,3011-rspack-manifest-provider,3012-rspack-js-entry-provider",
"app:manifest:dev": "nx run-many --target=serve --parallel=100 -p manifest-webpack-host,3009-webpack-provider,3010-rspack-provider,3011-rspack-manifest-provider,3012-rspack-js-entry-provider",
"app:ts:dev": "nx run-many --target=serve -p react_ts_host,react_ts_nested_remote,react_ts_remote",
"app:modern:dev": "nx run-many --target=serve --parallel=10 --configuration=development -p modernjs-ssr-dynamic-nested-remote,modernjs-ssr-dynamic-remote,modernjs-ssr-dynamic-remote-new-version,modernjs-ssr-host,modernjs-ssr-nested-remote,modernjs-ssr-remote,modernjs-ssr-remote-new-version",
"commitlint": "commitlint --edit",
Expand Down
76 changes: 40 additions & 36 deletions packages/enhanced/src/lib/container/HoistContainerReferencesPlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ import { normalizeWebpackPath } from '@module-federation/sdk/normalize-webpack-p
import type { RuntimeSpec } from 'webpack/lib/util/runtime';
import type ExportsInfo from 'webpack/lib/ExportsInfo';
import ContainerEntryModule from './ContainerEntryModule';
import { moduleFederationPlugin } from '@module-federation/sdk';

const { NormalModule, AsyncDependenciesBlock } = require(
normalizeWebpackPath('webpack'),
Expand All @@ -29,15 +30,18 @@ export class HoistContainerReferences implements WebpackPluginInstance {
private readonly entryFilePath?: string;
private readonly bundlerRuntimeDep?: string;
private readonly explanation: string;
private readonly experiments: moduleFederationPlugin.ModuleFederationPluginOptions['experiments'];

constructor(
name?: string,
entryFilePath?: string,
bundlerRuntimeDep?: string,
experiments?: moduleFederationPlugin.ModuleFederationPluginOptions['experiments'],
) {
this.containerName = name || 'no known chunk name';
this.entryFilePath = entryFilePath;
this.bundlerRuntimeDep = bundlerRuntimeDep;
this.experiments = experiments;
this.explanation =
'Bundler runtime path module is required for proper functioning';
}
Expand Down Expand Up @@ -92,7 +96,7 @@ export class HoistContainerReferences implements WebpackPluginInstance {
module instanceof NormalModule &&
module.resource === this.bundlerRuntimeDep
) {
const allRefs = this.getAllReferencedModules(
const allRefs = getAllReferencedModules(
compilation,
module,
'initial',
Comment on lines 96 to 102
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand All @@ -119,40 +123,6 @@ export class HoistContainerReferences implements WebpackPluginInstance {
);
}

// Helper method to collect all referenced modules recursively
private 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;
}

// Helper method to find a specific module in a chunk
private findModule(
compilation: Compilation,
Expand Down Expand Up @@ -226,7 +196,7 @@ export class HoistContainerReferences implements WebpackPluginInstance {
return;
}

const allReferencedModules = this.getAllReferencedModules(
const allReferencedModules = getAllReferencedModules(
compilation,
runtimeModule,
'initial',
Comment on lines 196 to 202
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down Expand Up @@ -291,4 +261,38 @@ export class HoistContainerReferences implements WebpackPluginInstance {
}
}

// 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;
}
Comment on lines +264 to +296
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Consider adding type annotations for the compilation, module, and type parameters to improve readability and type safety.

  2. 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.

  3. The function modifies the collectedModules set while iterating over it, which could lead to unexpected behavior. Consider using a separate set for newly discovered modules.

  4. The type parameter is only used for the 'initial' case. Consider renaming it to initialOnly or similar to make its purpose clearer.

Here's an example of how you could refactor this function:

Suggested change
// 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.


export default HoistContainerReferences;
1 change: 1 addition & 0 deletions packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,7 @@ class FederationRuntimePlugin {
// hoist all modules of federation entry
this.getFilePath(),
this.bundlerRuntimePath,
this.options.experiments,
).apply(compiler);

new compiler.webpack.NormalModuleReplacementPlugin(
Comment on lines 382 to 388
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Expand Down