Module Federation: Extract shared remote scaffolding - #2071
Conversation
|
Warning Review limit reached
Next review available in: 9 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (44)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Adds clients/shared, the scaffolding every phase remote in this repository copied by hand until now: - rspack/federatedDependencies.mjs owns the singleton share scope, so the host and the remotes read it from one place and cannot drift apart. - rspack/createRspackConfig.mjs is the rspack config factory for a remote. - runtime/mountRemote.tsx and runtime/StandaloneNotice.tsx are the standalone dev page. It is deliberately not a yarn workspace: the files are consumed through relative imports, so they compile inside each component's own rspack and TypeScript setup and need no dependency, lockfile, or Dockerfile change. The build-time half sits under rspack/ rather than the more obvious build/, because clients/.gitignore ignores build/ at any depth and would have kept the factory out of the repository while every local checkout still worked.
The seven remote rspack.config.mjs files were ~110 identical lines each, differing only in the component name, the dev port, and assessment's @hookform/resolvers alias. Each is now a call to createRspackConfig; the alias goes through a resolveAlias hook that receives the component directory. clients/core stays on its own config, since it is the host and shares almost nothing with a remote beyond the share scope, which it now imports from clients/shared instead of restating.
Every remote had the same bootstrap.tsx plus an App.tsx whose only job was the 'load this from inside the Prompt Core' notice. Both collapse into a two-line bootstrap.tsx over mountRemote and StandaloneNotice, and App.tsx is gone (it was never exposed to core, only imported by its own bootstrap). mountRemote throws when the root element is missing instead of casting null through unknown, which surfaces the mismatch this also fixes: the assessment bootstrap looked for 'assessment-root' while its template.html declared 'template-root', so the standalone page rendered nothing. Matching, team allocation, and self team allocation had likewise inherited a root id from the component they were copied from; each now uses its own.
…lding - scripts/new-course-phase.sh rewrote the COMPONENT_DEV_PORT constant, which no longer exists; it now rewrites the factory's port option. Same for template-repository/init.sh. - quality-clients lints per module directory, so clients/shared would have gone unchecked; the lint step now includes it, and generator-smoke also triggers on it since the generated config depends on it. - The MF skill and rule, the phase docs, and the two component readmes described the old per-component constants. The trailing-whitespace hook additionally normalized the markdown hard breaks in clients/matching_component/readme. - template-repository is a staging area for an external repo that cannot import from this monorepo. Its runbook now says to vendor clients/shared into the template and how to shorten the two import paths.
da956a3 to
b969de7
Compare
createRspackConfig fell back to rspack's default port when `port` was omitted, and federatedDependencies emitted a share entry without a requiredVersion for any singleton package missing from clients/package.json. Both now throw.
The port substitution was the only anchor in either generator that failed silently. A generated phase would have kept port 3001 and collided with example_component under `make clients`.
mathildeshagl
left a comment
mathildeshagl
left a comment
There was a problem hiding this comment.
Approving. This is the best-verified PR of the four, and the description is doing real work rather than narrating the diff.
I reproduced your central check independently instead of trusting it: worktrees for the merge base and this branch, each rspack.config.mjs imported with the working directory rspack uses, called for both NODE_ENV values, and the result serialized including regex sources and plugin option objects.
- All seven remotes: resolved configs byte-identical in both development and production.
- Core: the only difference is the
?${Date.now()}cache-buster on the remote URLs. Nothing else, in either mode.
Other claims I checked rather than assumed:
exposeswas already the same three entries in all seven configs on main, so the factory hardcoding them loses nothing.- The assessment bug is real: main had
assessment-rootin the bootstrap againsttemplate-rootintemplate.html, so that standalone page rendered nothing at all. The other three inherited root ids are now self-consistent. clients/sharedis tracked and not ignored. Andbuild/genuinely would have been swallowed at any depth by bothclients/.gitignoreandclients/.dockerignore, so therspack/naming is load-bearing rather than cosmetic. Thanks for writing that story down in the description; it is the kind of thing that costs the next person an afternoon.rootDir: ".."in the component tsconfigs already covers../../shared/..., so the relative imports need no tsconfig change.- Docker: the base image's
COPY . ./from theclients/context includesshared/, and each component image isFROMthat base and only overlays its own directory, so/app/sharedis present atyarn buildtime.
The configUrl reasoning is the part I'd have most likely got wrong myself. Deriving the component directory from the caller while leaving context unset, so entry, the HTML template, the public/ copy and output.uniqueName all keep resolving exactly as before, is subtle and the byte-identical configs confirm you got it right.
I did go looking for one failure mode and it is not there, so recording it as checked: since shared now reaches a component build only through the base image, a stale base would silently ship old scaffolding. It cannot happen in CI, because clients-base runs unconditionally on every run and every component job consumes IMAGE_TAG from that same run. And dev.yml uses paths-ignore rather than paths, so a clients/shared-only change still triggers the whole matrix.
Small things, none blocking:
- The
resolveAliascallback exists only so its one consumer can resolve againstcomponentDir. On Node 24 that component could useimport.meta.dirnameand pass a plain object, which removes the callback indirection from the shared factory entirely. The same trick would letconfigUrlbecomedirname: import.meta.dirnameand drop thefileURLToPathhop. yarn biome check "<module>" sharedlintssharedonce per matrix entry, so eight times. Redundant, butsharedcannot be a matrix entry of its own since it has no tsconfig or build script, so this looks like the right trade rather than an oversight.- Worth a line in
clients/shared/readme.md: building a single component image locally against a staleprompt-clients-base:latestnow silently uses old shared code. That was impossible before, when all of a component's build inputs came from its own context.
One cross-PR note rather than a change request: #1939 adds an eighth remote with its own copy of exactly this scaffolding, and it does not know about clients/shared. Whichever of the two lands second needs a follow-up commit. Details on that PR.
2 participants
Summary
Every phase remote carried its own copy of the Module Federation scaffolding: seven
rspack.config.mjsfiles of ~110 lines that differed only in a name, a port, and one alias, plus abootstrap.tsxand anApp.tsxper component that did the same two things. This extracts thatscaffolding into
clients/shared/so a remote declares only what is actually specific to it.Details
Where the shared code lives:
clients/shared/, deliberately not a yarn workspace.A workspace package was the obvious alternative and was rejected: the runtime files are TypeScript
sources, so consuming them through
node_moduleswould put them behind every component'sexclude: /node_modules/swc-loader rule and they would ship untranspiled. A plain directoryconsumed through relative imports compiles inside each component's own rspack and TypeScript setup,
and needs no dependency, lockfile, workspace, or Dockerfile change. It is already present in every
build context:
clients/Dockerfilecopies all ofclients/into theprompt-clients-baseimage at/app, and each component image isFROMthat base, so/app/sharedexists beforeyarn buildruns in
/app/<component>.The build-time half sits under
rspack/rather than the more obviousbuild/becauseclients/.gitignoreandclients/.dockerignoreboth ignorebuildat any depth. The first versionof this branch used
build/, and the effect was instructive: every local checkout worked while thefactory was never committed and never reached the base image, so CI failed on the client builds and
the e2e stack failed with
MODULE_NOT_FOUNDon/app/core/rspack.config.mjs. Worth knowing beforeadding anything else called
buildunderclients/.The build factory. Each remote config is now:
configUrlis what keeps the factory faithful. The old configs used__dirnamefordevServer.staticandoutput.pathwhile leavingcontextunset, soentry, the HTML template,and the
public/copy stayed relative to the working directory. Moving the file would have silentlyrepointed the first group at the shared directory and, worse, changed
output.uniqueName, whichrspack derives from the
package.jsonnearestcontext. The factory therefore resolves thecomponent directory from the caller's
import.meta.urland never setscontext.The assessment alias goes through
resolveAlias: (componentDir) => ({ … }). It is a callbackrather than a plain object because the two
@hookform/resolverspaths are resolved against thecomponent directory, which only the factory knows.
Share scope.
federatedDependencies.mjsis imported byclients/coreand by the factory, sohost and remotes now read the same five singletons (
react,react-dom,react-router-dom,@tanstack/react-query,@tumaet/prompt-shared-state) from one place and cannot drift apart. Thisis the only part of core's config that changed. Core keeps its own config otherwise: it is the host,
and its
remotes,publicPath, css rules, aliases,optimization, and compression plugin havenothing in common with a remote.
The runtime scaffolding replaces
bootstrap.tsx+App.tsxper component with a two-linebootstrap.tsx.App.tsxis deleted; it was never exposed to core (a phase is mounted through./routes, never through a root component) and was only imported by its own bootstrap.Two latent bugs surfaced and are fixed.
mountRemotethrows on a missing root element insteadof casting
nullthroughunknown, which exposed that the assessment bootstrap looked forassessment-rootwhile itstemplate.htmldeclaredtemplate-root, so its standalone page renderednothing at all. Matching, team allocation, and self team allocation had likewise inherited a root id
from whichever component they were copied from (
interview-root,template-root); each now uses itsown.
Tooling that referenced the removed constants, all updated:
scripts/new-course-phase.shandtemplate-repository/init.shrewroteCOMPONENT_DEV_PORT, which no longer exists; themodule-federation-remoteskill, themodule-federation/remotesrule,new_course_phase.md, andthe two component readmes documented the old per-component constants.
quality-clients.ymllintsone module directory per matrix entry, so
clients/sharedwould have gone unlinted; the lint stepnow includes it.
generator-smoke.ymlalso triggers onclients/shared/**, since the config itgenerates depends on it.
template-repository/stages an external repository that by definition cannot import from thismonorepo. That migration has not been executed, so the runbook now carries the step to vendor
clients/sharedinto the template and the two import depths to shorten.MIGRATION.mdis the onlyplace this is recorded, because the staged
client/directory itself is created fromclients/example_componentat migration time.One unrelated change rode along: the repo's
trailing-whitespacepre-commit hook normalized themarkdown hard breaks in
clients/matching_component/readmewhen that file was touched.Deliberately not included
src/index.jsandsrc/provide/index.tsare named in the issue as duplicated, but they are not.Each is one to three lines of genuinely per-component content: assessment imports
./loadStyleswhere the others import
./styles.css, certificate has asrc/provide.tsxfile instead of adirectory, example's
provide/index.tsis empty, and the rest re-export a component-specificStudentDetail. There is nothing to factor out of them.intro_course_developer_component,github_challenge_component) livein their own repositories and are untouched.
Reason / Link to issue
Closes #1981
How to Test
The risk here is that a config or share-scope change breaks the app at runtime rather than at build
time, so this was verified by comparing artifacts against
mainrather than by typecheck alone.rspack.config.mjswith the sameworking directory rspack uses, calls it for
NODE_ENV=developmentand=production, andserializes the result (regexes and plugin option objects included). All seven remotes produce
output identical to
mainin both modes. Core's differs only in the?${Date.now()}cache-buster; with that normalized it is identical too.
yarn --cwd <c> build, cachescleared on both sides). Six of the seven
remoteEntry.jsfiles are byte-identical tomain;assessment's differs in exactly one chunk contenthash reference. Per component the only changed
assets are the standalone-page chunk, the entry chunk, and the two HTML files, which is precisely
the surface this PR edits.
mainchunk is byte-identical. Its runtime chunk differs only in theMF module ids, which hash the timestamped remote URLs and therefore already differ between two
builds of an unmodified tree; normalizing the timestamp and the ids makes base, new, and a second
new build all identical.
yarn tsc --noEmitper component,make lint, and CI's per-moduleyarn biome check "<module>" sharedall pass with the same warning count asmainand no errors.scripts/new-course-phase.sh ci_smoke 3990 8990 5490produces acomponent whose factory call, root element id, and notice title are all renamed consistently,
passes the smoke workflow's
! grep -rin 'example'assertion, and typechecks, lints, and buildsa
remoteEntry.jswith the full share scope. Thescaffoldjob covers this in CI.host, plus a full local
make test-e2erun.Manual check for a reviewer:
cd clients && yarn dev, open http://localhost:3000, and confirm acourse with several phase types still lazy-loads each remote. Then open a remote's own port directly
(e.g. http://localhost:3007) and confirm the standalone notice renders, which is the part that was
broken for assessment before this PR.
PR Checklist