feat: shareable runtime #3018
feat: shareable runtime #3018
Conversation
…lugin.ts Co-authored-by: squadronai[bot] <170149692+squadronai[bot]@users.noreply.github.com>
…/federation-hooks # Conflicts: # .github/workflows/devtools.yml
🦋 Changeset detectedLatest commit: 37ed459 The changes in this PR will be included in the next version bump. This PR includes changesets to release 39 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. |
…ntime is used in chunk
…ial module in calculations
|
@squadronai review |
![squadronai[bot]](https://avatars.githubusercontent.com/in/901096?s=60&v=4){kind=small}
There was a problem hiding this comment.
Incremental Review
Comments posted: 16
Configuration
Squadron Mode: essential
Commits Reviewed
5abd1ba0b482f118bfef681c1d3bb102f7c9e70f...37ed459ded9f14e5b500bd271f27cfd96ce82211
Files Reviewed
- packages/nextjs-mf/src/plugins/NextFederationPlugin/index.ts
- packages/runtime/src/global.ts
- packages/runtime/src/index.ts
- packages/enhanced/src/lib/container/HoistContainerReferencesPlugin.ts
Files Ignored
These files were ignored due to the filter in the squadron.yaml file.
- .changeset/ai-eager-cat.md
- .changeset/ai-eager-tiger.md
- .changeset/ai-gentle-eagle.md
- .changeset/ai-noisy-fox.md
- .changeset/ai-sleepy-bear.md
- .changeset/ai-sleepy-fox.md
- .changeset/real-baboons-complain.md
- apps/3000-home/next-env.d.ts
- apps/3001-shop/next-env.d.ts
- apps/3002-checkout/next-env.d.ts
- packages/runtime/tests/snapshots/preload-remote.spec.ts.snap
- packages/runtime/tests/globa.spec.ts
- packages/runtime/tests/global.spec.ts
- packages/runtime/tests/sync.spec.ts
- packages/webpack-bundler-runtime/package.json
- packages/webpack-bundler-runtime/project.json
- packages/webpack-bundler-runtime/src/embedded.ts
| federationRuntime: | ||
| this._options.experiments?.federationRuntime || 'hoisted', |
There was a problem hiding this comment.
The default value for federationRuntime is set to 'hoisted'. Consider making this configurable or documenting the reasoning behind this default choice. If 'hoisted' is the recommended setting, it might be helpful to add a comment explaining why.
| federationRuntime: | |
| this._options.experiments?.federationRuntime || 'hoisted', | |
| experiments: { | |
| federationRuntime: | |
| this._options.experiments?.federationRuntime || 'hoisted', // Default to 'hoisted' for better performance and compatibility | |
| }, |
| @@ -213,7 +213,8 @@ | |||
| dts: this._options.dts ?? false, | |||
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', | |||
There was a problem hiding this comment.
The shareStrategy is set to 'loaded-first' by default. It would be beneficial to add a comment explaining the implications of this strategy and why it's chosen as the default.
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', | |
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', // Use 'loaded-first' strategy for efficient module sharing |
| dts: this._options.dts ?? false, | ||
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', | ||
| experiments: { | ||
| federationRuntime: 'hoisted', | ||
| federationRuntime: | ||
| this._options.experiments?.federationRuntime || 'hoisted', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Consider grouping related options together and adding type annotations for better code readability and maintainability. This can help developers understand the structure of the options object more easily.
| dts: this._options.dts ?? false, | |
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', | |
| experiments: { | |
| federationRuntime: 'hoisted', | |
| federationRuntime: | |
| this._options.experiments?.federationRuntime || 'hoisted', | |
| }, | |
| }; | |
| const pluginOptions: NextFederationPluginOptions = { | |
| dts: this._options.dts ?? false, | |
| shareStrategy: this._options.shareStrategy ?? 'loaded-first', | |
| experiments: { | |
| federationRuntime: this._options.experiments?.federationRuntime || 'hoisted', | |
| }, | |
| }; |
| export function setGlobalShareableRuntime( | ||
| runtimeExports: ShareableRuntime, | ||
| ): void { | ||
| if (nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__) { | ||
| return; | ||
| } | ||
| nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__ = runtimeExports; | ||
| } |
There was a problem hiding this comment.
The setGlobalShareableRuntime function could benefit from additional error handling and logging. Consider adding a warning if the runtime is already set, and potentially a type check for the runtimeExports parameter. Here's a suggested implementation:
| export function setGlobalShareableRuntime( | |
| runtimeExports: ShareableRuntime, | |
| ): void { | |
| if (nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__) { | |
| return; | |
| } | |
| nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__ = runtimeExports; | |
| } | |
| export function setGlobalShareableRuntime( | |
| runtimeExports: ShareableRuntime, | |
| ): void { | |
| if (nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__) { | |
| warn('Shareable runtime is already set. Ignoring new runtime.'); | |
| return; | |
| } | |
| if (typeof runtimeExports !== 'object' || runtimeExports === null) { | |
| throw new Error('Invalid shareable runtime provided.'); | |
| } | |
| nativeGlobal.__FEDERATION__.__SHAREABLE_RUNTIME__ = runtimeExports; | |
| } |
This change improves error handling, provides more informative warnings, and ensures type safety.
| type ShareableRuntime = { | ||
| FederationManager: typeof import('./index').FederationManager; | ||
| FederationHost: typeof import('./index').FederationHost; | ||
| loadScript: typeof import('./index').loadScript; | ||
| loadScriptNode: typeof import('./index').loadScriptNode; | ||
| registerGlobalPlugins: typeof import('./index').registerGlobalPlugins; | ||
| getRemoteInfo: typeof import('./index').getRemoteInfo; | ||
| getRemoteEntry: typeof import('./index').getRemoteEntry; | ||
| Module: typeof import('./index').Module; | ||
| }; |
There was a problem hiding this comment.
The ShareableRuntime type is currently using typeof import('./index') for each property. This approach might lead to circular dependencies and make the code harder to maintain. Consider defining these types explicitly or importing them from a separate types file. For example:
| type ShareableRuntime = { | |
| FederationManager: typeof import('./index').FederationManager; | |
| FederationHost: typeof import('./index').FederationHost; | |
| loadScript: typeof import('./index').loadScript; | |
| loadScriptNode: typeof import('./index').loadScriptNode; | |
| registerGlobalPlugins: typeof import('./index').registerGlobalPlugins; | |
| getRemoteInfo: typeof import('./index').getRemoteInfo; | |
| getRemoteEntry: typeof import('./index').getRemoteEntry; | |
| Module: typeof import('./index').Module; | |
| }; | |
| import { FederationManager, FederationHost, loadScript, loadScriptNode, registerGlobalPlugins, getRemoteInfo, getRemoteEntry, Module } from './types'; | |
| type ShareableRuntime = { | |
| FederationManager: FederationManager; | |
| FederationHost: FederationHost; | |
| loadScript: loadScript; | |
| loadScriptNode: loadScriptNode; | |
| registerGlobalPlugins: registerGlobalPlugins; | |
| getRemoteInfo: getRemoteInfo; | |
| getRemoteEntry: getRemoteEntry; | |
| Module: Module; | |
| }; |
This change would improve code maintainability and reduce the risk of circular dependencies.
| }, | ||
| (chunks: Iterable<Chunk>) => { | ||
| const runtimeChunks = this.getRuntimeChunks(compilation); | ||
| this.hoistModulesInChunks( | ||
| compilation, | ||
| runtimeChunks, | ||
| chunks, | ||
| logger, | ||
| containerEntryDependencies, | ||
| ); | ||
| this.hoistModulesInChunks(compilation, containerEntryDependencies); | ||
| }, | ||
| ); | ||
| }, |
There was a problem hiding this comment.
The hoistModulesInChunks method is called inside the optimizeChunks hook, which runs after the SplitChunksPlugin. Consider adding a comment explaining why this specific stage is chosen and how it interacts with other optimization plugins. This would improve the code's maintainability and help future developers understand the plugin's behavior in the context of Webpack's compilation process.
| private hoistModulesInChunks( | ||
| compilation: Compilation, | ||
| runtimeChunks: Set<Chunk>, | ||
| chunks: Iterable<Chunk>, | ||
| logger: ReturnType<Compilation['getLogger']>, | ||
| containerEntryDependencies: Set<Dependency>, | ||
| ): void { | ||
| const { chunkGraph, moduleGraph } = compilation; | ||
| // when runtimeChunk: single is set - ContainerPlugin will create a "partial" chunk we can use to | ||
| // move modules into the runtime chunk | ||
|
|
||
| // First, handle the minimal check and remove included modules from the chunk | ||
| this.handleMinimalCheck( | ||
| compilation, | ||
| containerEntryDependencies, | ||
| chunkGraph, | ||
| moduleGraph, | ||
| ); | ||
|
|
||
| // Now, perform the global hoist over all chunks | ||
| for (const dep of containerEntryDependencies) { | ||
| const containerEntryModule = moduleGraph.getModule(dep); | ||
| if (!containerEntryModule) continue; |
There was a problem hiding this comment.
The hoistModulesInChunks method seems to be doing multiple operations. Consider splitting it into smaller, more focused methods to improve readability and maintainability. For example, you could create separate methods for handling the minimal check and performing the global hoist. This would make the code easier to understand and test.
| compilation, | ||
| containerEntryModule, | ||
| 'initial', | ||
| true, | ||
| ); | ||
|
|
||
| const allRemoteReferences = getAllReferencedModules( | ||
| compilation, | ||
| containerEntryModule, | ||
| 'external', | ||
| true, | ||
| ); | ||
|
|
||
| for (const remote of allRemoteReferences) { |
There was a problem hiding this comment.
The getAllReferencedModules function is called twice with different parameters. Consider combining these calls into a single operation to reduce redundancy and improve performance. You could create a helper method that handles both 'initial' and 'external' types in one pass.
| private handleMinimalCheck( | ||
| compilation: Compilation, | ||
| containerEntryDependencies: Set<Dependency>, | ||
| chunkGraph: Compilation['chunkGraph'], | ||
| moduleGraph: Compilation['moduleGraph'], | ||
| ): void { | ||
| let minimal; | ||
| for (const dep of containerEntryDependencies as Set<FederationRuntimeDependency>) { | ||
| if (dep.minimal) { | ||
| minimal = moduleGraph.getModule(dep); | ||
| } | ||
| } | ||
| if (minimal) { | ||
| for (const dep of containerEntryDependencies as Set<FederationRuntimeDependency>) { | ||
| if (dep.minimal) continue; | ||
| const containerEntryModule = moduleGraph.getModule(dep); | ||
| if (!containerEntryModule) continue; | ||
| const allReferencedModules = getAllReferencedModules( | ||
| compilation, | ||
| containerEntryModule, | ||
| 'initial', | ||
| ); | ||
|
|
||
| const containerRuntimes = | ||
| chunkGraph.getModuleRuntimes(containerEntryModule); | ||
| const runtimes = new Set<string>(); | ||
|
|
||
| for (const runtimeSpec of containerRuntimes) { | ||
| compilation.compiler.webpack.util.runtime.forEachRuntime( | ||
| runtimeSpec, | ||
| (runtimeKey) => { | ||
| if (runtimeKey) { | ||
| runtimes.add(runtimeKey); | ||
| } | ||
| }, | ||
| ); | ||
| } | ||
|
|
||
| for (const runtime of runtimes) { | ||
| const runtimeChunk = compilation.namedChunks.get(runtime); | ||
| if (!runtimeChunk) continue; | ||
| // if there is no minimal chunk in the runtime module, skip it. | ||
| if (!chunkGraph.isModuleInChunk(minimal, runtimeChunk)) continue; | ||
|
|
||
| for (const module of allReferencedModules) { | ||
| if (chunkGraph.isModuleInChunk(module, runtimeChunk)) { | ||
| chunkGraph.disconnectChunkAndModule(runtimeChunk, module); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The handleMinimalCheck method is quite long and complex. Consider breaking it down into smaller, more focused methods. This would improve readability and make the code easier to maintain. Additionally, consider adding comments to explain the purpose of each major step in the process, especially for complex operations like the runtime handling.
| compilation: Compilation, | ||
| module: Module, | ||
| type?: 'all' | 'initial' | 'external', | ||
| withInitialModule?: boolean, | ||
| ): Set<Module> { | ||
| const collectedModules = new Set<Module>([module]); | ||
| const visitedModules = new WeakSet<Module>([module]); | ||
| const collectedModules = new Set<Module>(withInitialModule ? [module] : []); | ||
| const visitedModules = new WeakSet<Module>(withInitialModule ? [module] : []); | ||
| const stack = [module]; | ||
|
|
||
| while (stack.length > 0) { |
There was a problem hiding this comment.
The getAllReferencedModules function uses a recursive approach with a stack. While this works, for very large module graphs, it could potentially lead to stack overflow issues. Consider implementing an iterative approach instead, which would be more memory-efficient for large projects. Also, add a comment explaining the algorithm used for future maintainers.
Description
Related Issue
Types of changes
Checklist