Repository navigation
refactor(router): discovery plugin as phase modules (S15) - #1243
Merged
Merged
Conversation
createRouterDiscoveryPlugin's hooks now each call a phase module over DiscoveryState: discovery/dev-server-phase.ts (configureServer), discovery/build-discovery-phase.ts (buildStart, buildApp post, buildEnd) and discovery/bundle-scan-phase.ts (generateBundle). The temp RSC server moves to vite/temp-rsc-server.ts and the buildEnv helpers to discovery/build-env.ts, so the phases import them without a cycle back to the plugin file. The plugin closure's viteCommand, viteMode and transitionWhenOverlay become DiscoveryState fields. No logic change.
49 of 50 tasks
ivogt
added a commit
that referenced
this pull request
Oct 23, 2026
Brings in S16 (#1248, wire-params.ts WIRE), S5 (#1245, deriveRenderPass), S15 (#1243, discovery phases), #1246 and #1247. Conflicts, each resolved by keeping S14's structure and applying S16's intent to it: - rsc/handler.ts imports: kept the request-stages.js import; dropped the classifyRequest, RequestPlan and INTERNAL_RANGO_DEBUG imports S14 had already moved out; added S16's WIRE import, still read by the code that stayed in the handler (warmup, loader refuse, redirect tracking mode, isPartialAction). - rsc/handler.ts response tail: kept the finalizeRequestResponse call; the inline partial/action interception S16 rewrote lives in request-stages.ts and now reads WIRE.partial and WIRE.action. - rsc/handler.ts terminal plans: kept the terminalPlanResponse call; the action-rewrite check, partial redirect and version-mismatch log S16 rewrote live in request-stages.ts and now read WIRE.action, WIRE.partial and WIRE.version. - testing/dispatch.ts imports: added WIRE; dropped applyStreamIdleTimeout, which the stages now call. - testing/dispatch.ts isPartial: kept S14's shape (the reverse and isAction moved into the stages) with S16's WIRE.partial. - tools/internals-baseline.json: took the stack's version; check-internals --update leaves it unchanged (rsc-param-literal 4, all in use-loader.tsx). request-stages.ts has no `_rsc_*` literal left. The handler and stages together make the same searchParams calls as S16's handler.ts: has() on action x3, partial x3, loader and warmup; get() on loader and version.
16 of 17 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
devcorner: rango-t146 · session rango-s272
Problem
createRouterDiscoveryPlugin(src/vite/router-discovery.ts:527on the base, d9cbaa0) was one closure of 2,146 lines. ItsconfigureServerhook alone ran:643-2298;buildStartran:2303-2409,buildApppost:2423-2441,buildEnd:2446-2455andgenerateBundle:2547-2653. Three variables of plugin-instance lifetime (viteCommand,viteMode,transitionWhenOverlay) lived in the closure instead of onDiscoveryState, so no code outside the closure could see the state the hooks shared. This is S15 of the staged reorganisation (plan rango-d195 section 3; target rango-d194 section 3.6). It is a move only: nothing a consumer writes, sees or configures changes.Before / after (internal shape)
Before:
After:
Code-change summary
30 files, +2,557 / -2,458. Source (
src/vite/): +2,526 / -2,431, so the move costs 95 lines of module headers, imports and JSDoc.src/vite/router-discovery.tsconfigResolved,transform,handleHotUpdate,resolveId/loadandcloseBundle, unchanged.src/vite/discovery/dev-server-phase.ts(new)devServerPhase(s, server): theconfigureServerbody, verbatim. AlsoisFileOutsideApp, the overlay helpers and the/__rsc_shellboot-race injection set.src/vite/discovery/build-discovery-phase.ts(new)buildDiscoveryPhase(buildStart),postBuildPhase(buildApppost),buildEndPhase(buildEnd) andresetPrerenderCollection.src/vite/discovery/bundle-scan-phase.ts(new)bundleScanPhase(generateBundle).src/vite/temp-rsc-server.ts(new)createTempRscServerand thecloudflare:*Node loader-hook registration.src/vite/discovery/build-env.ts(new)resolveBuildEnv,acquireBuildEnv,releaseBuildEnv,wranglerProxyOptions.src/vite/discovery/state.tsviteCommand,viteMode,transitionWhenOverlayfields; the defaults are the closure's ("build","production",null).Why these locations:
router-discovery.ts, the phases would import the plugin file and the plugin file would import the phases, a cycle. Two leaf modules avoid it.temp-rsc-server.tssits insrc/vite/, notsrc/vite/discovery/.ensureCloudflareProtocolLoaderRegisteredresolves./plugins/cloudflare-protocol-loader-hook.mjsagainst its ownimport.meta.url. That relative path is correct fromsrc/vite/and from the published bundledist/vite/index.js, which ships the hook atdist/vite/plugins/. Fromsrc/vite/discovery/it would point to the wrong place.buildApppost moved withbuildEnd.buildStart's finally defers closing the kept temp server and releasing the buildEnv as a pair. ThebuildApppost hook releases that pair on success andbuildEndreleases it on abort, so all three now sit in one module.Edits inside the moved code, all mechanical:
s.viteCommand,s.viteModeands.transitionWhenOverlay. The overlay helperssendTransitionWhenOverlayandshowTransitionWhenErrortakesas a parameter.opts?.presetandopts?.staticRouteTypesGenerationreads.opts?.….s.optsis the sameoptsobject and is never reassigned.thisbecame an explicitcontext: Rolldown.PluginContextparameter inbuildStartandgenerateBundle../discovery/gate-state.ts, "ourhandleRouteFileChange") now name the new module.Checked token for token (whitespace and trailing commas ignored, the substitutions above applied): every moved body (
configureServer,buildStart,buildApp,buildEnd,generateBundle),createTempRscServerwith the loader hook, the build-env helpers,isFileOutsideApp,modulesForFile,TempServerResult,injectedShellNotReadyPathsandresetPrerenderCollectionmatch the base. The only other difference is theexportkeyword onacquireBuildEnvandreleaseBuildEnv.Not touched: virtual module ids and contents, codegen output, the HMR protocol and the plugin options.
Dist comparison (production builds)
Method: I ran
pnpm build-routerat the base (d9cbaa0) and at the head, and kept eachdist/vite/index.js. That bundle is the only router artifact the app builds load: the runtime resolves tosrc/, andsrc/outsidesrc/vite/is unchanged. Then I ranvite buildinpackages/rangojs-router/e2e/test-appandtests/cloudflare-basicthree times: base (b1), base again (b2) and head (h1). All runs used the same checkout path andRANGO_ENCRYPTION_KEYpinned to one key. Without the pinned key,__vite_rsc_encryption_key.jsdiffers on every build.The apps'
distis not byte-reproducible even on one commit:Date.now()/ ISO timestamps and random tokens (stok-…,:<base36>) into Prerender, Static and PPR shell payloads.__pr-/__ps-/__st-<hash>asset names, the three build manifests,buildVersionand theROUTER_VERSIONStable indeferred-resolution-*.jsall derive from those payloads.So the comparison has two parts. Files byte-identical in all three builds are compared by raw hash. The files that vary between two base builds are compared by hash after masking those values.
sha256 pathlist of those (b1 = b2 = h1)87404ad20177e9f501ed57a0205358017315665b6972bd7e4de6025a8b72df40Base vs head differs raw in exactly as many path entries as base vs base: 60 (test-app) and 98 (cloudflare-basic), the same manifests, payload assets and
deferred-resolution-*chunk. After masking, every file matches by manifest key in b1/b2, b1/h1 and b2/h1: 0 mismatches.Masked hashes of the files that vary between builds (h1; b1 and b2 lists are identical)
test-app (32 files):
cloudflare-basic (51 files):
Test plan
pnpm build-router; production builds of test-app and cloudflare-basic on base and head. Dist comparison above.node tools/generate-route-types.mjs --check: current (1,172 source files, 16 apps, 235 per-module files, 0 stale).node tools/check-bundle-guards.mjs: green across 3 apps.git status --porcelain | grep '\.gen\.ts': empty.pnpm --filter @rangojs/router test:hmr-routes-local(port offset 71): 20 passed, 1 flaky.an inline transition({ when }) edit republishes the hoisted predicatetimed out inwriteFileAndAwaitHmrand passed on retry. See Notes.pnpm --filter @rangojs/router test:hmr-local(port offset 71): dev-warmup + hmr-basename + hmr-prerender 9 passed (includes all 6prerender-hmrtests); hmr-routes 21 passed.route-types.test.ts, dev and production projects (port offset 71): 12 passed (9 dev, 3 production).test:e2e:hmr-local(--project=hmr, port offset 71). CI skips it, and it is the only suite that drives the Cloudflare branch of the moved dev phase: the temp server,forceCloudflareWorkerReloadand the readiness probe. Result:hmr.test.ts7 passed andhmr-cold-discovery-recovery1 passed;hmr-burst-save2 tests skipped.hmr-route-mutationshad no clean run, on base or on head. See Notes.pnpm run typecheck: 16/16 tasks.pnpm run test:unit:all: router 560 files / 9,234 passed (5 skipped, 6 todo); router rsc 81 files / 1,054 passed (3 expected fail); mini 25 + 3; vite-rsc-demo 27 + 1; cloudflare-basic 90 + 39; shared-e2e 20. 0 failures.oxlint --deny-warnings: 0 warnings, 0 errors.oxfmt --check .: clean.check-e2e-bucketing,check-e2e-parity --strict,check-e2e-console-baseline,check-suspense-contract,check-no-external-store,node --test tools/*.test.mjs(51 pass),check-doc-links,check-docs-api(448 identifiers),check-skills,generate-skills-catalog --check,generate-route-types --check,check-internals(counts unchanged: 21 / 327 / 71 / 1).Practices
oxlint --deny-warningsandoxfmt --check .on the whole repo; 0 findings afteroxfmtformatted the new files and the edited doc tables.createTempRscServer,resolveBuildEnvandisFileOutsideApphave callers in their new modules, and the three unit tests now import them from there. No re-exports were kept for the old path. Kept on purpose: the pre-existingexport { VIRTUAL_ROUTES_MANIFEST_ID }inrouter-discovery.ts, which nothing imports from that path. It predates this step, and removing it is outside a move.dev-discovery-recovery,temp-rsc-server-realm,build-env-auto-root,route-watcher-scope,bundle-postprocessand the rest ofsrc/vite, 62 files / 1,040 tests. Test edits are import paths, one fixture's three new requiredDiscoveryStatefields and comments. No new behaviour, so no new test. The HMR suites ran locally as listed, because CI skips them.docs/internal/feature-file-map.mdlists the five new files under the Vite plugin row. It also repoints the PPR-shell, progressiveChunkSize, dev-discovery-handshake and clientUrls entries.docs/internal/feature-map.mdnames the phase layout under Dev and HMR and repointswranglerProxyOptionsand the transition overlay.manifests.md,prerender-api-design.mdandreact-compiler.mdname the new homes ofisFileOutsideApp, the devdevServerOriginbranch andcreateTempRscServer. Comments insrc/vite/, unit tests and e2e files that namedrouter-discovery.tsfor moved code now name the phase module.Notes
route-types-hmr›an inline transition({ when }) edit republishes the hoisted predicatefails on base and on head, so this step did not cause it.Timed out waiting for HMR after writing …/urls/blog.tsx. sawServerSignal=false …, withpage.waitForFunctiontiming out inwaitForHydration.hmr-routesruns: head 5 runs gave 1 flaky, clean, clean, 1 flaky, and 1 run that failed on isolated-serverbeforeAllstart timeouts at load average 43. Base 5 runs were clean.transitiontests (--retries=0, 6 pairs): the same test failed once on base (base-2), with the identical error. Head failed once (head-2) on abeforeAllserver-start timeout. Every other run passed.tests/cloudflare-basichmr-route-mutations.test.tsnever passed locally on either side, so the Cloudflare HMR route-mutation path of the moved code has no green local run.expectServedafter the route add (:264), at thewebServer60 s start timeout, and at the clientUrls route-shape test (:230).:264, twice) and at "converges an open page". The furthest run passed 4 of 14 tests first.expectServedtiming out after 30 s: the gen file is rewritten, but workerd does not serve the new route shape in time. Load average was 8-31./__rsc_prerenderand/__rsc_shellendpoints are invite/router-discovery.ts:src/rsc/shell-serve.ts:57,src/rsc/shell-serve.ts:90andsrc/rsc/shell-build-manifest.ts:200. These comments end up in the RSC server bundle, so rewording them would change the appdistbytes and break the comparison above. They point to the plugin file, which wiresconfigureServertodevServerPhase. They can be reworded in a later commit with its own dist check.configResolved,resolveId/load,transform,handleHotUpdateandcloseBundlestay inline in the plugin. They are config capture or short wiring, and this step names only the three phase modules. rango-d194'sconfigPhase/virtualModulessplit stays open for a later step.configureServerbody's own locals stay function locals ofdevServerPhase, with the same per-dev-server lifetime as before: the temp server handles, the registries, the gate, the debounce timer and the prerender memo. Only the plugin closure's variables becameDiscoveryStatefields.