fix(runtime): keep top-level singleton on first init - #5028
fix(runtime): keep top-level singleton on first init#5028ScriptedAlchemy wants to merge 2 commits into
Conversation
init() unconditionally re-pointed the module-level FederationInstance that
backs the top-level loadRemote/registerRemotes/registerPlugins/loadShare/
preloadRemote/registerShared/getInstance exports whenever it created a new
instance. When a remote container resolves @module-federation/runtime to the
same copy as the host (typical for Node SSR builds that externalize
node_modules), executing the remote entry calls init({ name: '<remote>' })
and silently hijacks the host's top-level API: getInstance() returns the
remote and registerRemotes() writes into the remote's empty options.remotes.
Make the singleton first-wins in the create branch, mirroring the existing
else branch, while still returning the newly created instance. Add a
regression test and a changeset.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 8c3df75 The changes in this PR will be included in the next version bump. This PR includes changesets to release 48 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b23b5730d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // The top-level API singleton is first-wins: a later init() call (for | ||
| // example from a remote container that shares this runtime copy) must | ||
| // not re-point loadRemote/registerRemotes/getInstance to its instance. | ||
| if (!FederationInstance) { |
There was a problem hiding this comment.
When the exported resetFederationGlobalInfo() clears CurrentGlobal.__FEDERATION__.__INSTANCES__ (packages/runtime-core/src/global.ts:114-117) and the same runtime module is subsequently reinitialized, init() creates and globally registers a replacement but this condition preserves the detached old FederationInstance. Consequently, getInstance() and top-level APIs such as loadRemote and registerRemotes continue operating on stale state rather than the instance just returned by init(); before this change, the creation branch rebound the singleton. Treat the singleton as unset when it is no longer present in the global instance registry.
Useful? React with 👍 / 👎.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
@module-federation/devtools
@module-federation/cli
create-module-federation
@module-federation/dts-plugin
@module-federation/enhanced
@module-federation/error-codes
@module-federation/esbuild
@module-federation/managers
@module-federation/manifest
@module-federation/metro
@module-federation/metro-plugin-rnc-cli
@module-federation/metro-plugin-rnef
@module-federation/metro-plugin-rock
@module-federation/modern-js
@module-federation/modern-js-v3
@module-federation/native-federation-tests
@module-federation/native-federation-typescript
@module-federation/nextjs-mf
@module-federation/node
@module-federation/observability-plugin
@module-federation/playground
@module-federation/retry-plugin
@module-federation/rsbuild-plugin
@module-federation/rspack
@module-federation/rspress-plugin
@module-federation/rstest
@module-federation/runtime
@module-federation/runtime-core
@module-federation/runtime-tools
@module-federation/sdk
@module-federation/storybook-addon
@module-federation/third-party-dts-extractor
@module-federation/treeshake-frontend
@module-federation/treeshake-server
@module-federation/typescript
@module-federation/utilities
@module-federation/webpack-bundler-runtime
@module-federation/bridge-react
@module-federation/bridge-react-webpack-plugin
@module-federation/bridge-shared
@module-federation/bridge-vue3
@module-federation/inject-external-runtime-core-plugin
commit: |
Bundle Size Report8 package(s) changed, 35 unchanged. Package dist + ESM entry
Bundle targets
Tree-shakable entrypoints
Consumer scenarios
Total dist (raw): 36.16 MB (+3.0 kB (+0.0%)) Bundle sizes are generated with rslib (Rspack). Package-root metrics preserve the historical report. Tracked subpath exports such as |
|
Context from #4566: with a remote that resolves For scale, the same investigation's measurements of the force path itself (balanced on every runtime version) are here:
|
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
1 participant
Contract:
init()and the default instancepackages/runtime/src/index.tskeeps a module-level default instance (DefaultFederationInstance) that backs the top-level convenience APIsloadRemote,loadShare,loadShareSync,preloadRemote,registerRemotes,registerPlugins,registerSharedandgetInstance()(called without a finder). This PR makes the contract explicit, documents it in JSDoc oninit()/createInstance(), and encodes it in tests:init()establishes the default.init()calls create or reuse named instances and return them, but never redirect the top-level APIs away from the established default.createInstance()never establishes or changes the default.createInstance()and later passed throughinit()with the same name becomes the default only if no default exists yet.getInstance(finder)searches__FEDERATION__.__INSTANCES__independently of the default (unchanged).init()is restructured so instance selection happens first and the default is assigned in exactly one place:Defect this fixes
Previously
init()unconditionally overwrote the singleton whenever it created a new instance:When a remote entry resolves
@module-federation/runtimeto the same module copy as the host (typical for Node SSR builds that externalizenode_modules), executing the remote container callsinit({ name: '<remote>' })through the bundler runtime. That created a new instance and silently re-pointed the host's top-level API at the remote.Measured hijack
In a Node repro, after a single
loadRemote('repro_remote/Widget'):getInstance().namechanged fromrepro_hosttorepro_remoteregisterRemotes(...)wrote into the remote instance's emptyoptions.remotesinstead of the host'sThis breaks any host that refreshes remote definitions at runtime (see #4566).
Tests
New
default instance contractblock inpackages/runtime/__tests__/api.spec.ts. Each test gets a fresh runtime module (rs.resetModules()+ dynamic import) and a fresh global instance list (resetFederationGlobalInfo()from@module-federation/runtime-core), so neither the module-level default nor__FEDERATION__.__INSTANCES__leaks between tests. Covered:init()establishes the default (getInstance()isnullbefore, the host after)init()with a different name returns its own instance and does not replace the defaultinit()with the same name reuses the instance and merges options (a remote registered by the second call is visible on the first instance)createInstance()never establishes the default;createInstance()after a default never changes itgetInstance(finder)still searches global instances independently of the defaultregisterRemotes,registerShared,registerPlugins,preloadRemote,loadRemote,loadShare) routes to the default: spies on both instances assert the call lands on the default and not on the secondinit()instanceregisterRemotesmutates the host'soptions.remotes, not the laterinit()instance's (the original regression)createInstance()instance adopted byinit()becomes the default when none exists, and does not when one already existsThe "returns the default instance when no finder is provided" test now reads the current default via
getInstance()instead of assuming the latestinitwins.Sanity check: reverting
??=to=ininit()fails 4 of the new tests.pnpm --filter @module-federation/runtime test: 12 files, 103 tests, all passing.Changeset added for
@module-federation/runtime(patch).🤖 Generated with Claude Code