fix: middleware crash in dev mode with cloudflare plugin

[james-elicx]

Adds regression tests for two bugs that occur when @cloudflare/vite-plugin is present alongside middleware.ts.

Bug 1 — outsideEmitter crash

The connect handler called runMiddleware() → server.ssrLoadModule(), which constructs an SSRCompatModuleRunner. That runner requires a hot channel that doesn't exist in the host process when the Cloudflare plugin is loaded, crashing immediately with:

TypeError: Cannot read properties of undefined (reading 'outsideEmitter')

Fixed by switching to createDirectRunner(), which calls environment.fetchModule() directly and never touches the hot channel.

Bug 2 — requests intercepted before reaching the Worker

After the crash was resolved, the connect handler was still calling createSSRHandler() for any Pages Router route match, rendering pages in the host process instead of dispatching to the Cloudflare Worker. Routes like / that matched both a pages/ stub and an app/ route were served by the host instead of miniflare.

Fixed by the hasCloudflarePlugin early-return: after middleware runs, the connect handler calls next() unconditionally, handing off to the Cloudflare plugin.

How the tests assert Worker execution

• /api/hello returns navigator.userAgent — "Cloudflare-Workers" inside miniflare, absent or wrong if intercepted by the host.
• / has competing handlers: app/page.tsx renders "vinext on Cloudflare Workers", pages/index.tsx renders "pages index". The wrong text appearing means the Worker was bypassed.

[pkg-pr-new]

Open in StackBlitz

npm i https://pkg.pr.new/vinext@344

commit: 909b673

[github-actions]

Example	Preview	Production	Original
app-router-cloudflare	preview	production
pages-router-cloudflare	preview	production
app-router-playground	preview	production	original
realworld-api-rest	preview	production
nextra-docs-template	preview	production
benchmarks	preview	production
hackernews	preview	production	original

[james-elicx]

/bigbonk review this pr please

[ask-bonk]

Review: middleware crash in dev mode with cloudflare plugin

Overall this is a well-structured fix with good test coverage. The root cause analysis is accurate, the fix is minimal and targeted, and the regression tests are well-designed. A few observations:

What looks good

1. createDirectRunner() approach is correct. DevEnvironment.fetchModule() is a plain async method that doesn't touch the hot channel — this cleanly avoids the outsideEmitter crash without any fragile workarounds. The transport implementation in dev-module-runner.ts is clean and handles both fetchModule and getBuiltins invoke types.

2. Lazy initialization of pagesRunner via getPagesRunner() is the right call. It ensures all environments are fully registered before inspection, and the fallback chain ("ssr" → non-"rsc" → first available) is sensible.

3. hasCloudflarePlugin early-return at line 2947 is well-placed. It sits after middleware execution (so rewrites are applied to req.url) and before the host-side config redirect/rewrite/SSR logic (which would duplicate what the Worker entry already does). The extensive block comment explaining the reasoning is appreciated.

4. Test design is clever. Using navigator.userAgent === "Cloudflare-Workers" to assert Worker execution, and the competing pages/index.tsx vs app/page.tsx to catch routing bypass — both are effective regression detectors.

5. shims.test.ts changes are mechanical (mock shape update from ssrLoadModule to import) and correct.

Observations / minor items

1. DevEnvironmentLike interface typing (dev-module-runner.ts:72-78): fetchModule() returns Promise<Record<string, unknown>> in your interface, but Vite's actual DevEnvironment.fetchModule() returns Promise<FetchResult> (which is { externalize?: string; id?: string; code?: string; ... }). This is fine in practice since the invoke handler passes the result through opaquely (result: await environment.fetchModule(...)) and ModuleRunner knows how to interpret it. But the interface comment says "duck-typed to the minimal surface we need" — worth noting the return type is actually FetchResult, not an arbitrary record. Not blocking.

2. runner.close() lifecycle: The docstring example in dev-module-runner.ts shows await runner.close(), but pagesRunner in index.ts is never closed (e.g., in a server.close hook). This is fine for dev servers (process exits), but if there's ever a graceful shutdown path, this is a minor leak. Not blocking.

3. instrumentation.ts still uses server.ssrLoadModule() (line 118). The file's own comment at line 32-33 says this is safe because "Pages Router always uses plain Vite + Node.js (never @cloudflare/vite-plugin)". That reasoning holds today — configureServer() calls runInstrumentation() only for Pages Router. But if someone adds a pages/ directory to an app-router-cloudflare project (which this PR's test fixture literally does!), runInstrumentation() would run with @cloudflare/vite-plugin loaded and hit the same outsideEmitter crash. The hasCloudflarePlugin early-return at line 2947 papers over the routing bug but doesn't protect the instrumentation path. Worth verifying whether instrumentation is skipped for the cloudflare case.

4. api-handler.ts also uses server.ssrLoadModule() (line 179). Same concern as above — this is only called for Pages Router API routes, and the hasCloudflarePlugin early-return at line 2947 prevents it from running. But if someone has a Pages Router project deployed to Cloudflare Workers (the pages-router-cloudflare example exists), the dev server would still call ssrLoadModule for API routes. The hasCloudflarePlugin guard only protects the connect handler that runs middleware — not the API handler itself. This is pre-existing and not introduced by this PR, but worth noting.

5. No cleanup of pagesRunner on HMR invalidation: When middleware.ts is edited, the ModuleRunner will re-import the module via fetchModule() on the next request. ModuleRunner.import() should handle invalidation correctly since it delegates to fetchModule() which respects the Vite module graph. If there's module-level state in the middleware file, it will be re-evaluated. This matches the existing ssrLoadModule behavior, so no regression.

Verdict

The fix is correct for the bugs described. The createDirectRunner() approach is clean and well-documented. The tests are good regression coverage. The remaining ssrLoadModule callers in instrumentation.ts and api-handler.ts are protected by the hasCloudflarePlugin early-return for now, but could be fragile if the control flow changes in the future — consider migrating those to createDirectRunner() in a follow-up.

[ask-bonk]

Review posted. Here's a summary:

Overall: Good fix, well-tested, correct approach.

The PR fixes two bugs:

1. outsideEmitter crash — solved by replacing server.ssrLoadModule() with a createDirectRunner() that calls environment.fetchModule() directly, avoiding the hot channel entirely.
2. Host-side routing bypass — solved by the hasCloudflarePlugin early-return that delegates all rendering to the Cloudflare Worker after middleware runs.

Key observations in the review:

• The DevEnvironmentLike interface return type is technically FetchResult, not Record<string, unknown> — works in practice but is a minor typing inaccuracy.
• pagesRunner is never closed on server shutdown (minor, not blocking).
• instrumentation.ts and api-handler.ts still use server.ssrLoadModule() — they're currently protected by the hasCloudflarePlugin early-return, but could be fragile if control flow changes. Worth migrating in a follow-up.
• Tests are well-designed: navigator.userAgent check and competing pages/ vs app/ routes effectively detect both bugs.

github run

[james-elicx]

/bigbonk can you re-review please

[ask-bonk]

Re-review: middleware crash in dev mode with cloudflare plugin

Took a thorough second look at all 19 changed files. The fix is solid and CI is all green. A few additional observations beyond the first review:

Confirmed good

1. createDirectRunner() is clean and correct. The transport's invoke handler correctly unpacks the HotPayload shape and delegates to environment.fetchModule(). The getBuiltins handler returning [] is correct for the host Node.js process. The error fallback for unexpected invoke names is a nice touch.

2. Lazy getPagesRunner() with fallback chain is robust. The "ssr" → non-"rsc" → first available priority is the right call. Lazy init avoids the timing issue where environments aren't fully registered during configureServer().

3. hasCloudflarePlugin early-return placement is exactly right — after middleware (so rewrites are applied to req.url) and before config redirects/rewrites/headers/SSR (which the Worker handles internally). The extensive block comment at lines 2928-2946 is appreciated and accurate.

4. Test design is effective. The navigator.userAgent === "Cloudflare-Workers" check is a reliable signal for Worker execution. The competing pages/index.tsx vs app/page.tsx test on / is a clever way to detect the routing bypass bug. The x-mw-ran header checks confirm middleware actually ran.

5. shims.test.ts changes are mechanical — mockServer.ssrLoadModule → mockRunner.import with no logic changes. Correct.

6. Playwright config additions are well-structured. The cloudflare-pages-router-dev project correctly includes both its own spec files and instrumentation-startup.spec.ts. Port assignments (4177-4179) don't collide with existing projects.

7. instrumentation-state.ts using a temp file is the correct approach for cross-process state bridging between the host Node.js process and the miniflare Worker subprocess. The file comment at lines 7-29 explains this well.

One real issue: runInstrumentation silently fails in cloudflare-pages-router-dev

The previous review flagged this as observation #3, but I want to elevate it because CI is masking the problem:

• pages-router-cloudflare has instrumentation.ts + @cloudflare/vite-plugin + no app/ directory
• The guard if (instrumentationPath && !hasAppDir) at line 2563 means runInstrumentation(server, instrumentationPath) will be called
• runInstrumentation uses server.ssrLoadModule(), which should hit the outsideEmitter crash
• But it doesn't crash the server because runInstrumentation wraps the call in try/catch and only logs the error (line 130-135)
• instrumentation-startup.spec.ts only checks res.status() === 200 — it doesn't verify registerCalled === true

So right now: the server starts, the test passes, but register() and onRequestError() are silently not working in cloudflare-pages-router-dev. The test gives a false sense of coverage.

This isn't blocking for the PR — the fix for middleware is the priority and it's correct. But I'd recommend either:

• (a) Adding a test assertion in instrumentation-startup.spec.ts that registerCalled is true (which will fail and force migrating runInstrumentation to use getPagesRunner()), or
• (b) Migrating runInstrumentation in this PR to be consistent with the middleware fix

Minor nits (not blocking)

• DevEnvironmentLike return type (line 77): Promise<Record<string, unknown>> works in practice since the result is passed through opaquely, but the actual Vite type is Promise<FetchResult>. Consider importing FetchResult from vite/module-runner for accuracy.

• No runner.close() on server shutdown: pagesRunner is never closed. Fine for dev servers (process exits), but worth a server.httpServer?.on("close", ...) hook if graceful shutdown ever matters.

Overall: good fix, well-tested for the bugs it targets, ship it. The instrumentation gap is the one thing I'd address before or shortly after merge.

[ask-bonk]

Review posted on the PR. Here's a summary:

Overall: Good fix, ship it. The createDirectRunner() approach is correct, the hasCloudflarePlugin early-return is well-placed, and the test coverage for the two target bugs is effective. All CI is green.

One real issue I elevated: runInstrumentation() in instrumentation.ts still uses server.ssrLoadModule(), which silently fails when @cloudflare/vite-plugin is present (Pages Router + no app/ dir). It doesn't crash the server because it catches the error, but register() never runs. The instrumentation-startup.spec.ts test only checks HTTP 200 — not whether registerCalled is true — so CI masks the problem. Recommend either strengthening the test assertion or migrating runInstrumentation to use the direct runner in this PR or a fast follow-up.

github run

[james-elicx]

/bigbonk re-review again please

[ask-bonk]

Re-review #3: middleware crash in dev mode with cloudflare plugin

Read every changed file again. All CI checks (25/25) are green. The instrumentation issue from the prior two reviews has been fully addressed. Ship it.

Instrumentation issue — resolved

Both prior reviews flagged that runInstrumentation() still used server.ssrLoadModule(), which would silently fail when @cloudflare/vite-plugin was present. This has been fixed:

1. instrumentation.ts now accepts a ModuleImporter interface (runner.import()) instead of server — line 127-129. The module comment (lines 32-36) explicitly documents why createDirectRunner() is used instead of ssrLoadModule().

2. index.ts passes getPagesRunner() to runInstrumentation() at line 2608 (was server).

3. instrumentation-startup.spec.ts (line 44-45) now asserts data.registerCalled === true, not just HTTP 200. This runs under both cloudflare-pages-router (prod build) and cloudflare-pages-router-dev (vite dev) Playwright projects — both pass in CI.

4. Production path is also covered: generateServerEntry() now emits instrumentationImportCode and instrumentationInitCode as top-level awaits in the Worker bundle (lines 821-827), matching the App Router pattern in generateRscEntry().

createDirectRunner — correct

The transport implementation is clean. invoke correctly unpacks the HotPayload shape that normalizeModuleRunnerTransport() passes through, handles fetchModule and getBuiltins, and returns a structured error for unexpected invoke names. hmr: false and sourcemapInterceptor: false are the right settings for a non-HMR runner.

hasCloudflarePlugin early-return — correct

The guard at line 2990 sits after middleware execution (rewrites applied to req.url at line 2964) and before config redirects/rewrites/headers/SSR. The block comment (lines 2971-2989) accurately explains the reasoning. This protects all remaining ssrLoadModule callers in the connect handler — handleApiRoute() and createSSRHandler() are never reached when @cloudflare/vite-plugin is present.

Remaining ssrLoadModule callers — not a concern

api-handler.ts:179 and dev-server.ts (20+ calls) still use server.ssrLoadModule(). These are only reachable when hasCloudflarePlugin is false (the early-return at line 2990 prevents them from running otherwise). This is correct: without the Cloudflare plugin, the SSR environment is a standard RunnableDevEnvironment and ssrLoadModule() works fine.

Test coverage — thorough

• cloudflare-dev: navigator.userAgent === "Cloudflare-Workers" check on /api/hello and content check on / (competing pages/ vs app/ routes) effectively catch both the crash bug and the routing bypass bug.
• cloudflare-pages-router-dev: x-mw-ran header checks on matched/unmatched routes, plus navigator.userAgent checks on API routes and SSR pages.
• cloudflare-pages-router (prod): Same middleware assertions against a built Worker bundle.
• instrumentation-startup: Asserts registerCalled === true under both dev and prod Cloudflare projects.
• Unit tests: instrumentation.test.ts and features.test.ts mocks updated from ssrLoadModule to runner.import — mechanical and correct.

Two minor nits (not blocking)

1. DevEnvironmentLike.fetchModule return type (dev-module-runner.ts:77): Promise<Record<string, unknown>> works in practice since the result is passed through opaquely, but Vite's actual type is FetchResult. Consider importing it from vite/module-runner for documentation accuracy.

2. No runner.close() on server shutdown: pagesRunner is never closed. Fine for dev servers (process exits), but a server.httpServer?.on("close", () => pagesRunner?.close()) hook would be cleaner if graceful shutdown ever matters.

[ask-bonk]

Review posted with APPROVE. Here's the summary:

The instrumentation issue flagged in the two prior reviews has been fully fixed. runInstrumentation() now accepts a ModuleImporter interface and uses runner.import() instead of server.ssrLoadModule(). The test (instrumentation-startup.spec.ts) now asserts registerCalled === true, not just HTTP 200, and it runs under both the dev and prod Cloudflare Playwright projects — both pass.

All 25 CI checks are green. The createDirectRunner() implementation is correct, the hasCloudflarePlugin early-return is well-placed, and the remaining ssrLoadModule callers in api-handler.ts and dev-server.ts are properly protected by the early-return (unreachable when the Cloudflare plugin is present). Two minor non-blocking nits noted: the DevEnvironmentLike.fetchModule return type could use FetchResult instead of Record<string, unknown>, and pagesRunner isn't closed on server shutdown.

github run