-
-
Notifications
You must be signed in to change notification settings - Fork 238
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.
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
feat: support interface data prefetch #2543
Conversation
🦋 Changeset detectedLatest commit: 62b029d The changes in this PR will be included in the next version bump. This PR includes changesets to release 38 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
✅ Deploy Preview for module-federation-docs ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
if (!this.recordOutdate[id]) { | ||
this.recordOutdate[id] = {}; | ||
} | ||
this.recordOutdate[id][functionId] = isOutdate; |
Check warning
Code scanning / CodeQL
Prototype-polluting assignment Medium
library input
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
This assignment may alter Object.prototype if a malicious '__proto__' string is injected from
library input
mr Description information needs to let users know the corresponding information |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incremental Review
Comments posted: 0
Configuration
Squadron Mode: essential
Commits Reviewed
d7551fe1a4b5520d96ee3a3522ad14df7d354370...575c4548125f054cb448b706ed0390222c7aadf5
Files Reviewed
- packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/light-items-battle.md
- .github/workflows/e2e-manifest.yml
- apps/manifest-demo/3009-webpack-provider/tsconfig.app.json
- apps/manifest-demo/3009-webpack-provider/tsconfig.json
- apps/manifest-demo/3010-rspack-provider/package.json
- apps/manifest-demo/3010-rspack-provider/rspack.config.js
- apps/manifest-demo/3010-rspack-provider/src/App.tsx
- apps/manifest-demo/webpack-host/src/App.tsx
- apps/manifest-demo/webpack-host/webpack.config.js
- apps/modernjs/cypress/e2e/app.cy.ts
- apps/modernjs/modern.config.ts
- apps/modernjs/src/components/react-component.prefetch.ts
- apps/modernjs/src/components/react-component.tsx
- apps/modernjs/src/routes/page.tsx
- apps/website-new/docs/en/guide/_meta.json
- apps/website-new/docs/en/guide/performance/_meta.json
- apps/website-new/docs/en/guide/performance/prefetch.mdx
- apps/website-new/docs/public/guide/performance/data-prefetch/common.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/log.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/prefetch.jpg
- apps/website-new/docs/zh/guide/_meta.json
- apps/website-new/docs/zh/guide/performance/_meta.json
- apps/website-new/docs/zh/guide/performance/prefetch.mdx
- package.json
- packages/data-prefetch/CHANGELOG.md
- packages/data-prefetch/README.md
- packages/data-prefetch/tests/babel.spec.ts
- packages/data-prefetch/tests/prefetch.spec.ts
- packages/data-prefetch/tests/react.spec.ts
- packages/data-prefetch/jest.config.js
- packages/data-prefetch/package.json
- packages/data-prefetch/project.json
- packages/data-prefetch/src/cli/babel.ts
- packages/data-prefetch/src/cli/index.ts
- packages/data-prefetch/src/common/constant.ts
- packages/data-prefetch/src/common/index.ts
- packages/data-prefetch/src/common/node-utils.ts
- packages/data-prefetch/src/common/runtime-utils.ts
- packages/data-prefetch/src/index.ts
- packages/data-prefetch/src/logger/index.ts
- packages/data-prefetch/src/plugin.ts
- packages/data-prefetch/src/prefetch.ts
- packages/data-prefetch/src/react/hooks.ts
- packages/data-prefetch/src/react/index.ts
- packages/data-prefetch/src/react/utils.ts
- packages/data-prefetch/src/shared/index.ts
- packages/data-prefetch/src/universal/index.ts
- packages/data-prefetch/tsconfig.json
- packages/data-prefetch/tsup.config.ts
- packages/enhanced/package.json
- packages/manifest/tsconfig.json
- packages/webpack-bundler-runtime/src/initContainerEntry.ts
- packages/webpack-bundler-runtime/src/types.ts
- pnpm-lock.yaml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incremental Review
Comments posted: 11
Configuration
Squadron Mode: essential
Commits Reviewed
e25a957ca31629d6d1b9a8a56b521b998a87b1a3...b5d033c3a81cd935dda7faa3d72dce8fe2d45f3a
Files Reviewed
- packages/enhanced/src/lib/container/ContainerEntryModule.ts
- packages/enhanced/src/lib/container/ModuleFederationPlugin.ts
- packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
- packages/enhanced/src/prefetch.ts
- packages/manifest/src/ManifestManager.ts
- packages/runtime/src/core.ts
- packages/runtime/src/index.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/plugins/generate-preload-assets.ts
- packages/runtime/src/remote/index.ts
- packages/sdk/src/constant.ts
- packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/light-items-battle.md
- .github/workflows/e2e-manifest.yml
- apps/manifest-demo/3009-webpack-provider/tsconfig.app.json
- apps/manifest-demo/3009-webpack-provider/tsconfig.json
- apps/manifest-demo/3010-rspack-provider/package.json
- apps/manifest-demo/3010-rspack-provider/rspack.config.js
- apps/manifest-demo/3010-rspack-provider/src/App.tsx
- apps/manifest-demo/webpack-host/src/App.tsx
- apps/manifest-demo/webpack-host/webpack.config.js
- apps/modernjs/cypress/e2e/app.cy.ts
- apps/modernjs/modern.config.ts
- apps/modernjs/src/components/react-component.prefetch.ts
- apps/modernjs/src/components/react-component.tsx
- apps/modernjs/src/routes/page.tsx
- apps/website-new/docs/en/guide/_meta.json
- apps/website-new/docs/en/guide/performance/_meta.json
- apps/website-new/docs/en/guide/performance/prefetch.mdx
- apps/website-new/docs/public/guide/performance/data-prefetch/common.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/log.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/prefetch.jpg
- apps/website-new/docs/zh/guide/_meta.json
- apps/website-new/docs/zh/guide/performance/_meta.json
- apps/website-new/docs/zh/guide/performance/prefetch.mdx
- package.json
- packages/data-prefetch/CHANGELOG.md
- packages/data-prefetch/README.md
- packages/data-prefetch/tests/babel.spec.ts
- packages/data-prefetch/tests/prefetch.spec.ts
- packages/data-prefetch/tests/react.spec.ts
- packages/data-prefetch/jest.config.js
- packages/data-prefetch/package.json
- packages/data-prefetch/project.json
- packages/data-prefetch/src/cli/babel.ts
- packages/data-prefetch/src/cli/index.ts
- packages/data-prefetch/src/common/constant.ts
- packages/data-prefetch/src/common/index.ts
- packages/data-prefetch/src/common/node-utils.ts
- packages/data-prefetch/src/common/runtime-utils.ts
- packages/data-prefetch/src/index.ts
- packages/data-prefetch/src/logger/index.ts
- packages/data-prefetch/src/plugin.ts
- packages/data-prefetch/src/prefetch.ts
- packages/data-prefetch/src/react/hooks.ts
- packages/data-prefetch/src/react/index.ts
- packages/data-prefetch/src/react/utils.ts
- packages/data-prefetch/src/shared/index.ts
- packages/data-prefetch/src/universal/index.ts
- packages/data-prefetch/tsconfig.json
- packages/data-prefetch/tsup.config.ts
- packages/enhanced/package.json
- packages/manifest/tsconfig.json
- packages/webpack-bundler-runtime/src/initContainerEntry.ts
- packages/webpack-bundler-runtime/src/types.ts
- pnpm-lock.yaml
@@ -293,7 +294,9 @@ class ContainerEntryModule extends Module { | |||
'})', | |||
], | |||
)};`, | |||
PrefetchPlugin.setRemoteIdentifier(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PrefetchPlugin.setRemoteIdentifier()
call is inserted directly into the generated code. This might lead to issues if the plugin is not properly initialized or if its API changes. Consider wrapping this call in a try-catch block to handle potential errors gracefully:
PrefetchPlugin.setRemoteIdentifier(), | |
try { | |
PrefetchPlugin.setRemoteIdentifier(); | |
} catch (error) { | |
console.warn('Failed to set remote identifier for prefetching:', error); | |
} |
This change ensures that any errors during the prefetch setup won't break the entire module initialization process.
`${initRuntimeModuleGetter}`, | ||
PrefetchPlugin.removeRemoteIdentifier(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar to the setRemoteIdentifier
call, the removeRemoteIdentifier
call should also be wrapped in a try-catch block for error handling:
PrefetchPlugin.removeRemoteIdentifier(), | |
try { | |
PrefetchPlugin.removeRemoteIdentifier(); | |
} catch (error) { | |
console.warn('Failed to remove remote identifier for prefetching:', error); | |
} |
This ensures consistent error handling for both prefetch-related operations.
@@ -16,6 +16,7 @@ import type { | |||
ResolverWithOptions, | |||
WebpackOptions, | |||
} from 'webpack/lib/Module'; | |||
import { PrefetchPlugin } from '@module-federation/data-prefetch/cli'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import statement for the PrefetchPlugin suggests that it's coming from a CLI package. It's unusual to import runtime functionality from a CLI package. Consider moving the PrefetchPlugin to a runtime package or creating a separate runtime package for data prefetching functionality. This would improve the separation of concerns and potentially reduce bundle size by not including unnecessary CLI code in the runtime.
import { PrefetchPlugin } from '@module-federation/data-prefetch/cli'; | |
import { PrefetchPlugin } from '@module-federation/data-prefetch/runtime'; |
This change assumes you've refactored the project structure to separate CLI and runtime concerns.
let prefetchInterface = false; | ||
const prefetchFilePath = path.resolve( | ||
compiler.options.context || process.cwd(), | ||
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | ||
); | ||
const existPrefetch = fs.existsSync(prefetchFilePath); | ||
if (existPrefetch) { | ||
const content = fs.readFileSync(prefetchFilePath).toString(); | ||
if (content) { | ||
prefetchInterface = true; | ||
} | ||
} | ||
stats.metaData.prefetchInterface = prefetchInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the prefetch interface check into a separate method for better readability and reusability. This will also make the generateManifest
method less cluttered. Here's a suggestion:
let prefetchInterface = false; | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | |
); | |
const existPrefetch = fs.existsSync(prefetchFilePath); | |
if (existPrefetch) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
if (content) { | |
prefetchInterface = true; | |
} | |
} | |
stats.metaData.prefetchInterface = prefetchInterface; | |
private checkPrefetchInterface(compiler: Compiler, stats: Stats): boolean { | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}` | |
); | |
if (fs.existsSync(prefetchFilePath)) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
return !!content; | |
} | |
return false; | |
} | |
// In generateManifest method: | |
stats.metaData.prefetchInterface = this.checkPrefetchInterface(compiler, stats); |
This change improves code organization and makes the prefetch interface check more modular.
return sum; | ||
}, [] as ManifestRemote[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for processing manifest.exposes
, manifest.shared
, and manifest.remotes
is repetitive. Consider creating a helper function to reduce duplication. Here's a suggestion:
return sum; | |
}, [] as ManifestRemote[]); | |
private processManifestArray<T, U>( | |
items: T[], | |
transformer: (item: T) => U | |
): U[] { | |
return items.reduce((sum, cur) => { | |
sum.push(transformer(cur)); | |
return sum; | |
}, [] as U[]); | |
} | |
// Usage in generateManifest: | |
manifest.exposes = this.processManifestArray(stats.exposes, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
assets: cur.assets, | |
path: cur.path, | |
})); | |
manifest.shared = this.processManifestArray(stats.shared, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
version: cur.version, | |
singleton: cur.singleton, | |
requiredVersion: cur.requiredVersion, | |
hash: cur.hash, | |
assets: cur.assets, | |
})); | |
manifest.remotes = this.processManifestArray(stats.remotes, (cur) => { | |
const remote: ManifestRemote = { | |
federationContainerName: cur.federationContainerName, | |
moduleName: cur.moduleName, | |
alias: cur.alias, | |
}; | |
if ('entry' in cur) { | |
remote.entry = cur.entry; | |
} else if ('version' in cur) { | |
remote.entry = cur.version; | |
} | |
return remote; | |
}); |
This change reduces code duplication and improves maintainability.
expose: string, | ||
options?: { loadFactory?: boolean }, | ||
remoteSnapshot?: ModuleInfo, | ||
) { | ||
const { loadFactory = true } = options || { loadFactory: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using object destructuring with default values for cleaner option handling:
const { loadFactory = true } = options || { loadFactory: true }; | |
const { loadFactory = true } = options ?? {}; |
This approach eliminates the need for the fallback object and makes the default value more explicit.
name: remoteInfo.name, | ||
remoteSnapshot: moduleInfoSnapshot, | ||
preloadConfig, | ||
remote: remoteInfo, | ||
remote: remoteInfo as Remote, | ||
origin, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a type annotation to the preloadInfo
object for better clarity and type safety. This can help prevent potential type-related issues in the future and improve code readability. Here's a suggestion:
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo, | |
remote: remoteInfo as Remote, | |
origin, | |
}); | |
const preloadInfo: PreloadInfo = { | |
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo as Remote, | |
origin, | |
}; |
You would need to define the PreloadInfo
interface in the appropriate location, possibly in the types file. This change enhances type safety and makes the code more self-documenting.
@@ -228,6 +232,7 @@ export interface ModuleFederationPluginOptions { | |||
dev?: boolean | PluginDevOptions; | |||
dts?: boolean | PluginDtsOptions; | |||
async?: boolean | AsyncBoundaryOptions; | |||
dataPrefetch?: DataPrefetch; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to explain the purpose and usage of the dataPrefetch
option. For example:
dataPrefetch?: DataPrefetch; | |
/** | |
* When enabled, allows remote modules to send their interface requests | |
* along with the module static resource in advance, improving load times. | |
*/ | |
dataPrefetch?: DataPrefetch; |
@@ -228,6 +232,7 @@ export interface ModuleFederationPluginOptions { | |||
dev?: boolean | PluginDevOptions; | |||
dts?: boolean | PluginDtsOptions; | |||
async?: boolean | AsyncBoundaryOptions; | |||
dataPrefetch?: DataPrefetch; | |||
virtualRuntimeEntry?: boolean; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be helpful to add a comment explaining the purpose and impact of the virtualRuntimeEntry
option. For example:
virtualRuntimeEntry?: boolean; | |
/** | |
* When set to true, creates a virtual entry point for runtime, | |
* which can be useful for certain module federation setups. | |
*/ | |
virtualRuntimeEntry?: boolean; |
experiments?: { | ||
federationRuntime?: false | 'hoisted'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to explain the purpose of the experiments
object and specifically the federationRun
property. For example:
experiments?: { | |
federationRuntime?: false | 'hoisted'; | |
/** | |
* Experimental features for the Module Federation Plugin. | |
* @property {boolean} federationRun - Enables experimental federation runtime features. | |
*/ | |
experiments?: { | |
federationRun?: boolean; | |
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incremental Review
Comments posted: 9
Configuration
Squadron Mode: essential
Commits Reviewed
81201b8b5170f7d0475049d016bad44a13318f7f...0c6250298fc62ae75445282d2876de9f47c4083f
Files Reviewed
- packages/enhanced/src/lib/container/ContainerEntryModule.ts
- packages/enhanced/src/lib/container/ModuleFederationPlugin.ts
- packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
- packages/enhanced/src/prefetch.ts
- packages/manifest/src/ManifestManager.ts
- packages/runtime/src/core.ts
- packages/runtime/src/index.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/plugins/generate-preload-assets.ts
- packages/runtime/src/remote/index.ts
- packages/sdk/src/constant.ts
- packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/light-items-battle.md
- .github/workflows/e2e-manifest.yml
- apps/manifest-demo/3009-webpack-provider/tsconfig.app.json
- apps/manifest-demo/3009-webpack-provider/tsconfig.json
- apps/manifest-demo/3010-rspack-provider/package.json
- apps/manifest-demo/3010-rspack-provider/rspack.config.js
- apps/manifest-demo/3010-rspack-provider/src/App.tsx
- apps/manifest-demo/webpack-host/src/App.tsx
- apps/manifest-demo/webpack-host/webpack.config.js
- apps/modernjs/cypress/e2e/app.cy.ts
- apps/modernjs/modern.config.ts
- apps/modernjs/src/components/react-component.prefetch.ts
- apps/modernjs/src/components/react-component.tsx
- apps/modernjs/src/routes/page.tsx
- apps/website-new/docs/en/guide/_meta.json
- apps/website-new/docs/en/guide/performance/_meta.json
- apps/website-new/docs/en/guide/performance/prefetch.mdx
- apps/website-new/docs/public/guide/performance/data-prefetch/common.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/log.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/prefetch.jpg
- apps/website-new/docs/zh/guide/_meta.json
- apps/website-new/docs/zh/guide/performance/_meta.json
- apps/website-new/docs/zh/guide/performance/prefetch.mdx
- package.json
- packages/data-prefetch/CHANGELOG.md
- packages/data-prefetch/README.md
- packages/data-prefetch/tests/babel.spec.ts
- packages/data-prefetch/tests/prefetch.spec.ts
- packages/data-prefetch/tests/react.spec.ts
- packages/data-prefetch/jest.config.js
- packages/data-prefetch/package.json
- packages/data-prefetch/project.json
- packages/data-prefetch/src/cli/babel.ts
- packages/data-prefetch/src/cli/index.ts
- packages/data-prefetch/src/common/constant.ts
- packages/data-prefetch/src/common/index.ts
- packages/data-prefetch/src/common/node-utils.ts
- packages/data-prefetch/src/common/runtime-utils.ts
- packages/data-prefetch/src/index.ts
- packages/data-prefetch/src/logger/index.ts
- packages/data-prefetch/src/plugin.ts
- packages/data-prefetch/src/prefetch.ts
- packages/data-prefetch/src/react/hooks.ts
- packages/data-prefetch/src/react/index.ts
- packages/data-prefetch/src/react/utils.ts
- packages/data-prefetch/src/shared/index.ts
- packages/data-prefetch/src/universal/index.ts
- packages/data-prefetch/tsconfig.json
- packages/data-prefetch/tsup.config.ts
- packages/enhanced/package.json
- packages/manifest/tsconfig.json
- packages/webpack-bundler-runtime/src/initContainerEntry.ts
- packages/webpack-bundler-runtime/src/types.ts
- pnpm-lock.yaml
@@ -16,6 +16,7 @@ import type { | |||
ResolverWithOptions, | |||
WebpackOptions, | |||
} from 'webpack/lib/Module'; | |||
import { PrefetchPlugin } from '@module-federation/data-prefetch/cli'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import of PrefetchPlugin
from '@module-federation/data-prefetch/cli' is a new addition that supports the interface data prefetch feature. Ensure that this dependency is properly documented in the project's README or relevant documentation to inform other developers about this new functionality.
PrefetchPlugin.setRemoteIdentifier(), | ||
`${initRuntimeModuleGetter}`, | ||
PrefetchPlugin.removeRemoteIdentifier(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of PrefetchPlugin.setRemoteIdentifier()
and PrefetchPlugin.removeRemoteIdentifier()
suggests that you're setting up a context for prefetching data. Consider adding a brief comment explaining the purpose of these calls and how they relate to the data prefetching feature. For example:
PrefetchPlugin.setRemoteIdentifier(), | |
`${initRuntimeModuleGetter}`, | |
PrefetchPlugin.removeRemoteIdentifier(), | |
// Set up context for data prefetching | |
PrefetchPlugin.setRemoteIdentifier(), | |
`${initRuntimeModuleGetter}`, | |
// Clean up prefetching context | |
PrefetchPlugin.removeRemoteIdentifier(), |
This will improve code readability and help other developers understand the purpose of these method calls.
return sum; | ||
}, [] as ManifestRemote[]); | ||
|
||
let prefetchInterface = false; | ||
const prefetchFilePath = path.resolve( | ||
compiler.options.context || process.cwd(), | ||
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | ||
); | ||
const existPrefetch = fs.existsSync(prefetchFilePath); | ||
if (existPrefetch) { | ||
const content = fs.readFileSync(prefetchFilePath).toString(); | ||
if (content) { | ||
prefetchInterface = true; | ||
} | ||
} | ||
stats.metaData.prefetchInterface = prefetchInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the prefetch interface check into a separate method for better readability and reusability. This will also make the generateManifest
method less cluttered. Here's a suggestion:
return sum; | |
}, [] as ManifestRemote[]); | |
let prefetchInterface = false; | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | |
); | |
const existPrefetch = fs.existsSync(prefetchFilePath); | |
if (existPrefetch) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
if (content) { | |
prefetchInterface = true; | |
} | |
} | |
stats.metaData.prefetchInterface = prefetchInterface; | |
private checkPrefetchInterface(compiler: Compiler, stats: Stats): boolean { | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}` | |
); | |
if (fs.existsSync(prefetchFilePath)) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
return !!content; | |
} | |
return false; | |
} | |
// In generateManifest method: | |
stats.metaData.prefetchInterface = this.checkPrefetchInterface(compiler, stats); |
This change improves code organization and makes the prefetch interface check more modular.
@@ -101,6 +105,20 @@ class ManifestManager { | |||
return sum; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for processing manifest.exposes
, manifest.shared
, and manifest.remotes
is repetitive. Consider creating a helper function to reduce duplication. Here's a suggestion:
return sum; | |
private processManifestArray<T, U>( | |
items: T[], | |
transformer: (item: T) => U | |
): U[] { | |
return items.reduce((sum, cur) => { | |
sum.push(transformer(cur)); | |
return sum; | |
}, [] as U[]); | |
} | |
// Usage in generateManifest: | |
manifest.exposes = this.processManifestArray(stats.exposes, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
assets: cur.assets, | |
path: cur.path, | |
})); | |
manifest.shared = this.processManifestArray(stats.shared, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
version: cur.version, | |
singleton: cur.singleton, | |
requiredVersion: cur.requiredVersion, | |
hash: cur.hash, | |
assets: cur.assets, | |
})); | |
manifest.remotes = this.processManifestArray(stats.remotes, (cur) => { | |
const remote: ManifestRemote = { | |
federationContainerName: cur.federationContainerName, | |
moduleName: cur.moduleName, | |
alias: cur.alias, | |
}; | |
if ('entry' in cur) { | |
remote.entry = cur.entry; | |
} else if ('version' in cur) { | |
remote.entry = cur.version; | |
} | |
return remote; | |
}); |
This change reduces code duplication and improves maintainability.
} | ||
} | ||
stats.metaData.prefetchInterface = prefetchInterface; | ||
|
||
this._manifest = manifest; | ||
|
||
const manifestFileName = this.fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the case where additionalData
is provided but fails. This will make the code more robust. Here's a suggestion:
const manifestFileName = this.fileName; | |
if (additionalData) { | |
try { | |
const ret = await additionalData({ | |
manifest: this._manifest, | |
stats, | |
pluginOptions: this._options, | |
compiler, | |
compilation, | |
bundler, | |
}); | |
this._manifest = ret || this._manifest; | |
} catch (error) { | |
console.error('Error processing additional data:', error); | |
// Optionally, you can add the error to the compilation warnings or errors | |
compilation.warnings.push(new Error('Failed to process additional manifest data')); | |
} | |
} |
This change adds error handling to prevent the entire process from failing if additionalData
throws an error.
expose: string, | ||
options?: { loadFactory?: boolean }, | ||
remoteSnapshot?: ModuleInfo, | ||
) { | ||
const { loadFactory = true } = options || { loadFactory: true }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider using object destructuring with default values for better consistency and readability:
const { loadFactory = true } = options || { loadFactory: true }; | |
const { loadFactory = true } = options ?? {}; |
This approach is more consistent with modern JavaScript practices and clearly shows the default value for loadFactory
.
name: remoteInfo.name, | ||
remoteSnapshot: moduleInfoSnapshot, | ||
preloadConfig, | ||
remote: remoteInfo, | ||
remote: remoteInfo as Remote, | ||
origin, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a type annotation to the preloadInfo
object for better clarity and type safety. This can help prevent potential type-related issues in the future and improve code readability. Here's a suggestion:
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo, | |
remote: remoteInfo as Remote, | |
origin, | |
}); | |
const preloadInfo: PreloadInfo = { | |
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo as Remote, | |
origin, | |
}; |
You would need to define the PreloadInfo
interface in the appropriate location, possibly in the types file. This change enhances type safety and makes the code more self-documenting.
export interface AdditionalDataOptions { | ||
stats: Stats; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to explain the purpose and contents of the AdditionalDataOptions
interface. For example:
export interface AdditionalDataOptions { | |
stats: Stats; | |
/** | |
* Additional data options for the Module Federation Plugin. | |
* This interface includes statistics about the module. | |
*/ | |
export interface AdditionalDataOptions { | |
stats: Stats; |
This helps developers understand the purpose and contents of this interface at a glance.
dev?: boolean | PluginDevOptions; | ||
dts?: boolean | PluginDtsOptions; | ||
async?: boolean | AsyncBoundaryOptions; | ||
dataPrefetch?: DataPrefetch; | ||
virtualRuntimeEntry?: boolean; | ||
experiments?: { | ||
federationRuntime?: false | 'hoisted'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding comments to explain the purpose of each option in this interface. For example:
dev?: boolean | PluginDevOptions; | |
dts?: boolean | PluginDtsOptions; | |
async?: boolean | AsyncBoundaryOptions; | |
dataPrefetch?: DataPrefetch; | |
virtualRuntimeEntry?: boolean; | |
experiments?: { | |
federationRuntime?: false | 'hoisted'; | |
/** | |
* Enable development mode or specify development options. | |
*/ | |
dev?: boolean | PluginDevOptions; | |
/** | |
* Enable TypeScript declaration file generation or specify options. | |
*/ | |
dts?: boolean | PluginDtsOptions; | |
/** | |
* Enable asynchronous loading or specify async boundary options. | |
*/ | |
async?: boolean | AsyncBoundaryOptions; | |
/** | |
* Enable data prefetching for remote modules. | |
*/ | |
dataPrefetch?: DataPrefetch; | |
/** | |
* Enable virtual runtime entry. | |
*/ | |
virtualRuntimeEntry?: boolean; | |
/** | |
* Experimental features. | |
*/ | |
experiments?: { | |
federationRun |
These comments provide more context about each option, making the code more self-documenting and easier to understand.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Incremental Review
Comments posted: 9
Configuration
Squadron Mode: essential
Commits Reviewed
4a79d27662c70232dac3dc5ee651dadb16d833be...319e4c5b7520710c2a44f69a82bc0f331a9699b4
Files Reviewed
- packages/enhanced/src/lib/container/ContainerEntryModule.ts
- packages/enhanced/src/lib/container/ModuleFederationPlugin.ts
- packages/enhanced/src/lib/container/runtime/FederationRuntimePlugin.ts
- packages/enhanced/src/prefetch.ts
- packages/manifest/src/ManifestManager.ts
- packages/runtime/src/core.ts
- packages/runtime/src/index.ts
- packages/runtime/src/module/index.ts
- packages/runtime/src/plugins/generate-preload-assets.ts
- packages/runtime/src/remote/index.ts
- packages/sdk/src/constant.ts
- packages/sdk/src/types/plugins/ModuleFederationPlugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/light-items-battle.md
- apps/manifest-demo/3009-webpack-provider/tsconfig.app.json
- apps/manifest-demo/3009-webpack-provider/tsconfig.json
- apps/manifest-demo/3010-rspack-provider/package.json
- apps/manifest-demo/3010-rspack-provider/rspack.config.js
- apps/manifest-demo/3010-rspack-provider/src/App.tsx
- apps/manifest-demo/webpack-host/src/App.tsx
- apps/manifest-demo/webpack-host/webpack.config.js
- apps/modernjs/cypress/e2e/app.cy.ts
- apps/modernjs/modern.config.ts
- apps/modernjs/src/components/react-component.prefetch.ts
- apps/modernjs/src/components/react-component.tsx
- apps/modernjs/src/routes/page.tsx
- apps/website-new/docs/en/guide/_meta.json
- apps/website-new/docs/en/guide/performance/_meta.json
- apps/website-new/docs/en/guide/performance/prefetch.mdx
- apps/website-new/docs/public/guide/performance/data-prefetch/common.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/log.jpg
- apps/website-new/docs/public/guide/performance/data-prefetch/prefetch.jpg
- apps/website-new/docs/zh/guide/_meta.json
- apps/website-new/docs/zh/guide/performance/_meta.json
- apps/website-new/docs/zh/guide/performance/prefetch.mdx
- package.json
- packages/data-prefetch/CHANGELOG.md
- packages/data-prefetch/README.md
- packages/data-prefetch/tests/babel.spec.ts
- packages/data-prefetch/tests/prefetch.spec.ts
- packages/data-prefetch/tests/react.spec.ts
- packages/data-prefetch/jest.config.js
- packages/data-prefetch/package.json
- packages/data-prefetch/project.json
- packages/data-prefetch/src/cli/babel.ts
- packages/data-prefetch/src/cli/index.ts
- packages/data-prefetch/src/common/constant.ts
- packages/data-prefetch/src/common/index.ts
- packages/data-prefetch/src/common/node-utils.ts
- packages/data-prefetch/src/common/runtime-utils.ts
- packages/data-prefetch/src/index.ts
- packages/data-prefetch/src/logger/index.ts
- packages/data-prefetch/src/plugin.ts
- packages/data-prefetch/src/prefetch.ts
- packages/data-prefetch/src/react/hooks.ts
- packages/data-prefetch/src/react/index.ts
- packages/data-prefetch/src/react/utils.ts
- packages/data-prefetch/src/shared/index.ts
- packages/data-prefetch/src/universal/index.ts
- packages/data-prefetch/tsconfig.json
- packages/data-prefetch/tsup.config.ts
- packages/enhanced/package.json
- packages/manifest/tsconfig.json
- packages/webpack-bundler-runtime/src/initContainerEntry.ts
- packages/webpack-bundler-runtime/src/types.ts
- pnpm-lock.yaml
@@ -16,6 +16,7 @@ import type { | |||
ResolverWithOptions, | |||
WebpackOptions, | |||
} from 'webpack/lib/Module'; | |||
import { PrefetchPlugin } from '@module-federation/data-prefetch/cli'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The import of PrefetchPlugin
from '@module-federation/data-prefetch/cli' is a new addition that supports the interface data prefetch feature. Ensure that this dependency is properly documented in the project's README or relevant documentation to inform other developers about this new functionality.
PrefetchPlugin.setRemoteIdentifier(), | ||
`${initRuntimeModuleGetter}`, | ||
PrefetchPlugin.removeRemoteIdentifier(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The usage of PrefetchPlugin.setRemoteIdentifier()
and PrefetchPlugin.removeRemoteIdentifier()
suggests that you're setting up a context for prefetching data. Consider adding a brief comment explaining the purpose of these calls and how they relate to the data prefetching feature. For example:
PrefetchPlugin.setRemoteIdentifier(), | |
`${initRuntimeModuleGetter}`, | |
PrefetchPlugin.removeRemoteIdentifier(), | |
// Set up context for data prefetching | |
PrefetchPlugin.setRemoteIdentifier(), | |
`${initRuntimeModuleGetter}`, | |
// Clean up prefetching context | |
PrefetchPlugin.removeRemoteIdentifier(), |
This will improve code readability and make it easier for other developers to understand the purpose of these method calls.
let prefetchInterface = false; | ||
const prefetchFilePath = path.resolve( | ||
compiler.options.context || process.cwd(), | ||
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | ||
); | ||
const existPrefetch = fs.existsSync(prefetchFilePath); | ||
if (existPrefetch) { | ||
const content = fs.readFileSync(prefetchFilePath).toString(); | ||
if (content) { | ||
prefetchInterface = true; | ||
} | ||
} | ||
stats.metaData.prefetchInterface = prefetchInterface; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting the prefetch interface check into a separate method for better readability and reusability. This will also make the generateManifest
method less cluttered. Here's a suggestion:
let prefetchInterface = false; | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}`, | |
); | |
const existPrefetch = fs.existsSync(prefetchFilePath); | |
if (existPrefetch) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
if (content) { | |
prefetchInterface = true; | |
} | |
} | |
stats.metaData.prefetchInterface = prefetchInterface; | |
private checkPrefetchInterface(compiler: Compiler, stats: Stats): boolean { | |
const prefetchFilePath = path.resolve( | |
compiler.options.context || process.cwd(), | |
`node_modules/.mf/${encodeName(stats.name)}/${MFPrefetchCommon.fileName}` | |
); | |
if (fs.existsSync(prefetchFilePath)) { | |
const content = fs.readFileSync(prefetchFilePath).toString(); | |
return !!content; | |
} | |
return false; | |
} | |
// In generateManifest method: | |
stats.metaData.prefetchInterface = this.checkPrefetchInterface(compiler, stats); |
This change improves code organization and makes the prefetch interface check more modular.
return sum; | ||
}, [] as ManifestRemote[]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for processing manifest.exposes
, manifest.shared
, and manifest.remotes
is repetitive. Consider creating a helper function to reduce duplication. Here's a suggestion:
return sum; | |
}, [] as ManifestRemote[]); | |
private processManifestArray<T, U>( | |
items: T[], | |
transformer: (item: T) => U | |
): U[] { | |
return items.reduce((sum, cur) => { | |
sum.push(transformer(cur)); | |
return sum; | |
}, [] as U[]); | |
} | |
// Usage in generateManifest: | |
manifest.exposes = this.processManifestArray(stats.exposes, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
assets: cur.assets, | |
path: cur.path, | |
})); | |
manifest.shared = this.processManifestArray(stats.shared, (cur) => ({ | |
id: cur.id, | |
name: cur.name, | |
version: cur.version, | |
singleton: cur.singleton, | |
requiredVersion: cur.requiredVersion, | |
hash: cur.hash, | |
assets: cur.assets, | |
})); | |
manifest.remotes = this.processManifestArray(stats.remotes, (cur) => { | |
const remote: ManifestRemote = { | |
federationContainerName: cur.federationContainerName, | |
moduleName: cur.moduleName, | |
alias: cur.alias, | |
}; | |
if ('entry' in cur) { | |
remote.entry = cur.entry; | |
} else if ('version' in cur) { | |
remote.entry = cur.version; | |
} | |
return remote; | |
}); |
This change reduces code duplication and improves maintainability.
} | ||
} | ||
stats.metaData.prefetchInterface = prefetchInterface; | ||
|
||
this._manifest = manifest; | ||
|
||
const manifestFileName = this.fileName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding error handling for the case where additionalData
is provided but fails. This will make the code more robust. Here's a suggestion:
const manifestFileName = this.fileName; | |
if (additionalData) { | |
try { | |
const ret = await additionalData({ | |
manifest: this._manifest, | |
stats, | |
pluginOptions: this._options, | |
compiler, | |
compilation, | |
bundler, | |
}); | |
this._manifest = ret || this._manifest; | |
} catch (error) { | |
console.error('Error processing additional data:', error); | |
// Optionally, you can add the error to the compilation warnings or errors | |
compilation.warnings.push(new Error('Failed to process additional manifest data')); | |
} | |
} |
This change adds error handling to prevent the entire process from failing if additionalData
throws an error.
@@ -100,6 +105,8 @@ class Module { | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding more specific error handling for the remoteEntryExports.init
call. This could help in diagnosing issues more easily during the initialization process.
try { | |
await remoteEntryExports.init( | |
initContainerOptions.shareScope, | |
initContainerOptions.initScope, | |
initContainerOptions.remoteEntryInitOptions, | |
); | |
} catch (error) { | |
console.error(`Failed to initialize remote entry for ${this.remoteInfo.name}:`, error); | |
throw error; | |
} |
This change adds a try-catch block to provide more context in case of an initialization error, which could be helpful for debugging in complex federated module setups.
name: remoteInfo.name, | ||
remoteSnapshot: moduleInfoSnapshot, | ||
preloadConfig, | ||
remote: remoteInfo, | ||
remote: remoteInfo as Remote, | ||
origin, | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a type annotation to the preloadInfo
object for better clarity and type safety. This can help prevent potential type-related issues in the future and improve code readability. Here's a suggestion:
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo, | |
remote: remoteInfo as Remote, | |
origin, | |
}); | |
const preloadInfo: PreloadInfo = { | |
name: remoteInfo.name, | |
remoteSnapshot: moduleInfoSnapshot, | |
preloadConfig, | |
remote: remoteInfo as Remote, | |
origin, | |
}; |
You would need to define the PreloadInfo
interface in the appropriate location, possibly in the types file. This change enhances type safety and makes the code more self-documenting.
export interface AdditionalDataOptions { | ||
stats: Stats; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding a comment to explain the purpose and usage of the AdditionalDataOptions
interface. For example:
export interface AdditionalDataOptions { | |
stats: Stats; | |
/** | |
* Additional options for data-related configurations. | |
*/ | |
export interface AdditionalDataOptions { | |
stats: Stats; |
This helps developers understand the context and purpose of this interface.
dev?: boolean | PluginDevOptions; | ||
dts?: boolean | PluginDtsOptions; | ||
async?: boolean | AsyncBoundaryOptions; | ||
dataPrefetch?: DataPrefetch; | ||
virtualRuntimeEntry?: boolean; | ||
experiments?: { | ||
federationRuntime?: false | 'hoisted'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding comments to explain the purpose of each option in this interface. For example:
dev?: boolean | PluginDevOptions; | |
dts?: boolean | PluginDtsOptions; | |
async?: boolean | AsyncBoundaryOptions; | |
dataPrefetch?: DataPrefetch; | |
virtualRuntimeEntry?: boolean; | |
experiments?: { | |
federationRuntime?: false | 'hoisted'; | |
/** | |
* Enable development mode or specify development options. | |
*/ | |
dev?: boolean | PluginDevOptions; | |
/** | |
* Enable TypeScript declaration file generation or specify DTS options. | |
*/ | |
dts?: boolean | PluginDtsOptions; | |
/** | |
* Enable asynchronous loading or specify async boundary options. | |
*/ | |
async?: boolean | AsyncBoundaryOptions; | |
/** | |
* Enable data prefetching for remote modules. | |
*/ | |
dataPrefetch?: DataPrefetch; | |
/** | |
* Enable virtual runtime entry. | |
*/ | |
virtualRuntimeEntry?: boolean; | |
/** | |
* Experimental features configuration. | |
*/ | |
experiments?: { | |
federationRun |
These comments provide more context about each option, making the code more self-documenting and easier to understand.
Description
Support remote module send their interface request along with the module static resource in advance, rather than render to call request.
This will be useful when remote module load for a long time, their request will be parallel with the module itself.
And it also useful for secondary screen, the module can show directly with it's data
Related Issue
Types of changes
Checklist