Pull request overview

This PR fixes a Vite 7 dev-server startup crash when a project contains instrumentation.ts by deferring the ssrLoadModule()-based instrumentation load until Vite’s SSR transport channel is initialized.

Changes:

• Move runInstrumentation() invocation out of the configureServer body and into the returned post-middleware hook.
• Add regression tests ensuring runInstrumentation() doesn’t throw on Vite SSR transport/channel errors and that it invokes register() when exported.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Review

The core fix is correct — moving runInstrumentation() into the returned post-middleware function so that ssrLoadModule() is called after Vite's SSR transport channel is initialized. Good diagnosis of the root cause.

A few issues worth addressing:

Review: fix(instrumentation): defer ssrLoadModule to post-middleware hook

The core fix is correct and well-motivated. Moving runInstrumentation() from the configureServer body into the returned post-middleware function is the right approach for the Vite 7 SSR transport timing issue. The author has addressed all of bigbonk's feedback (re-added .catch(), removed the duplicate test, tightened the assertion).

Correctness

The fix is correct. In Vite's createServer(), the lifecycle is:

1. configureServer hooks are called (collecting post-hooks)
2. Internal middlewares are installed (SSR transport channel initialized here)
3. Post-hooks are called synchronously via postHooks.forEach(fn => fn && fn())
4. listen() is called later

The original code called ssrLoadModule() during step 1, before the SSR transport channel existed. The fix moves it to step 3, where the channel is ready. This directly addresses the TypeError: Cannot read properties of undefined (reading 'outsideEmitter') crash from issue #167.

Next.js parity

Next.js docs state that register() is called "once when a new Next.js server instance is initiated" and "must complete before the server is ready to handle requests." The current implementation fires runInstrumentation() without await, so it's a fire-and-forget async call. This means there's a theoretical race where early requests could arrive before register() completes.

However, this is a pre-existing issue, not introduced by this PR. The old code had the same fire-and-forget pattern (just in a different location where it crashed). And in practice, the post-hook runs during createServer() before listen() is called, so register() will almost certainly complete before any request arrives. This is acceptable for a dev server.

Non-blocking note (pre-existing, out of scope): For strict Next.js parity, runInstrumentation should ideally be awaited to guarantee register() completes before the first request. But the post-hook function is synchronous (Vite doesn't await it), so this would require a different approach (e.g., a middleware that blocks until instrumentation is ready). Not worth blocking this PR over. Could be filed as a separate enhancement.

Security

No security concerns. Deferring instrumentation loading by a few milliseconds during dev server startup doesn't create an exploitable window. Instrumentation is for observability (Sentry, OpenTelemetry), not for security enforcement. Even if a request arrived before register() completed, the only consequence would be that one request wouldn't be traced.

Dev/prod parity

No parity issue. I checked all four server files:

• server/prod-server.ts: No instrumentation loading at all (production server doesn't use ssrLoadModule)
• cloudflare/index.ts: No instrumentation handling
• server/dev-server.ts: Only imports reportRequestError (the consumer side), not runInstrumentation
• server/app-dev-server.ts: Same, only imports reportRequestError

The runInstrumentation call only exists in index.ts (the Vite plugin's configureServer hook), which is dev-only. The production path doesn't have this issue because it doesn't use Vite's SSR module runner.

Pre-existing note (out of scope): prod-server.ts and the Cloudflare worker entry don't call runInstrumentation at all, meaning instrumentation.ts is completely ignored in production. This is a separate gap worth tracking but not introduced by this PR.

Test quality

The new test is good: it verifies that runInstrumentation handles the specific outsideEmitter TypeError gracefully (catches it, logs it, doesn't crash). The assertion is tight (exact error message match) and console.error is properly spied/restored.

As bigbonk correctly noted (and the author acknowledged), a unit test with mocks can't verify the actual timing behavior (that ssrLoadModule works in the post-hook but not in configureServer). That would require an integration test against a real Vite dev server. The existing test coverage in instrumentation.test.ts already covers the core runInstrumentation behavior thoroughly (register called, onRequestError stored, missing exports handled, ssrLoadModule failures caught).

Edge cases

• No instrumentation file: Handled by the if (instrumentationPath) guard. No change in behavior.
• instrumentation.ts throws: Handled by runInstrumentation's internal try/catch plus the outer .catch() safety net. Logs error, continues.
• Slow instrumentation: Same fire-and-forget behavior as before. Early requests might not have instrumentation active. Acceptable for dev.

Approving. Clean fix for a real crash, good diagnosis, feedback addressed. The pre-existing gaps (fire-and-forget timing, no production instrumentation support) should be tracked separately.