fix(config): pass correct Next.js phase to function-form configs + instrumentation test coverage#273
Conversation
unwrapConfig() hardcoded "phase-development-server" regardless of whether vinext was running in dev or build mode. Apps that branch on the phase argument in next.config.ts received wrong config during production builds. Map Vite's env.command to the correct Next.js phase constant: - "serve" → "phase-development-server" - "build" → "phase-production-build"
The instrumentation module had zero tests. Add 11 tests covering: - findInstrumentationFile: file discovery across root and src/, priority ordering, null when absent - runInstrumentation: register() lifecycle, onRequestError handler storage, graceful handling of empty modules, error isolation when ssrLoadModule fails - reportRequestError: handler invocation with correct args, no-op without handler, error isolation when handler throws
The static-export test had its own startFixtureServer that loaded the fixture's vite.config.ts via configFile. This caused RSC plugin timing issues on Node 25 / macOS — the App Router dev server returned 404 for all pages. The Pages Router section (same approach) worked because it doesn't use the RSC plugin. Switch the App Router section to use the shared startFixtureServerHelper from helpers.ts, which uses configFile: false with explicit plugin registration. This matches how app-router.test.ts (193 passing tests) creates its dev server and avoids the RSC timing issue.
commit: |
loadNextConfig() hardcoded "phase-development-server" regardless of context. Add optional phase parameter (matching cloudflare#273) and pass the correct phase from each call site: - vinext build: "phase-production-build" - runStaticExport: "phase-export" This ensures function-form next.config files that branch on the phase argument receive the correct value during builds and exports.
|
/bigbonk review please |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Solid PR — the config phase fix is a real bug fix, the instrumentation tests are well-structured, and the static-export test fix is a reasonable approach. A few things to consider:
Config phase fix: Correct and well-tested. One improvement: import the existing PHASE_* constants from shims/constants.ts instead of hardcoding string literals in two more places.
Instrumentation tests: Good coverage of the public API. The vi.resetModules() pattern is the right approach for testing module-level state. One issue: temp directories are created but never cleaned up.
Static export test fix: The switch to startFixtureServerHelper makes sense and aligns with app-router.test.ts. Note that the old local startFixtureServer function is now only used by the Pages Router section — consider switching that too for consistency (and removing the local function), or adding a comment explaining why it's different.
|
The review was submitted successfully. Here's a summary of the feedback I posted: Review posted on PR #273 with 4 inline comments:
The review summary also noted that the now-local |
…strumentation-tests
- Import PHASE_PRODUCTION_BUILD and PHASE_DEVELOPMENT_SERVER constants from shims/constants.ts instead of hardcoding string literals in index.ts and config/next-config.ts - Add afterEach cleanup for temp directories in instrumentation tests
…etup - Replace hardcoded phase strings in next-config.test.ts with PHASE_PRODUCTION_BUILD and PHASE_DEVELOPMENT_SERVER constants - Remove unnecessary vi.resetModules() from findInstrumentationFile tests — it's a pure function with no module-level state to reset
Remove the local startFixtureServer function — the shared helper from tests/helpers.ts does the same thing. The Pages Router fixture's vite.config.ts is just `vinext()` with no special options, so the shared helper (which uses configFile: false + vinext plugin) works identically.
|
@southpolesteve pls have a look :) |
|
/bigbonk re-review please |
|
@NathanDrake2406 thanks for keeping on working on this! |
There was a problem hiding this comment.
Clean PR — the config phase fix is correct and well-tested, the instrumentation test coverage is solid, and the static-export test fix is the right approach. Previous review feedback (use PHASE_* constants, temp directory cleanup) has all been addressed.
One minor nit inline, otherwise LGTM.
| }; | ||
|
|
||
| // Should not throw | ||
| await runInstrumentation(server, "/fake/instrumentation.ts"); |
There was a problem hiding this comment.
Nit: The runInstrumentation tests verify register() is called and onRequestError is stored, but there's no test for the case where register() itself throws. The implementation catches the outer ssrLoadModule failure (tested at line 106), but if register() throws, the error propagates uncaught from runInstrumentation since only the ssrLoadModule call is inside the try/catch — actually, looking again at the source, the await mod.register() is also inside the try/catch block at instrumentation.ts:92-109, so it would be caught. A test confirming that would be a nice addition, but not blocking.
|
Review submitted. Here's a summary of my findings: Approved — this is a solid PR with three well-scoped changes:
One minor nit left as a non-blocking comment about adding a test for |
Np haha, I had a bunch of tokens left before it reset, just putting it to good use |
Keep both imports: PHASE_DEVELOPMENT_SERVER (from this PR) and normalizePageExtensions (from upstream).
3 participants
Summary
fix(config):
unwrapConfig()hardcoded"phase-development-server"when calling function-form Next.js configs, even during production builds. Apps that branch onphaseinnext.config.ts(e.g.(phase) => phase === "phase-production-build" ? prodConfig : devConfig) received the wrong config duringvinext build. Now maps Vite'senv.commandto the correct Next.js phase ("build"→"phase-production-build","serve"→"phase-development-server").test(instrumentation): Added 11 tests for
server/instrumentation.tswhich previously had zero coverage. CoversfindInstrumentationFile(file discovery priority, src/ fallback, null when absent),runInstrumentation(register lifecycle, onRequestError storage, error isolation), andreportRequestError(handler forwarding, no-op without handler, handler error isolation).fix(test): Fixed 6 pre-existing failures in
static-export.test.tsApp Router section. The test used a localstartFixtureServerthat loaded the fixture'svite.config.tsviaconfigFile, which caused RSC plugin timing issues on Node 25. Switched to the sharedstartFixtureServerHelperfromhelpers.ts(usesconfigFile: falsewith explicit plugins), matching the pattern used byapp-router.test.ts.Test plan
npx vitest run tests/next-config.test.ts— 3 tests verifying phase propagation (production build phase, dev default, object-form ignores phase)npx vitest run tests/instrumentation.test.ts— 11 tests covering file discovery, register/onRequestError lifecycle, error isolationnpx vitest run tests/static-export.test.ts— 14 tests now all pass (was 6 failures)npx vitest run— full suite passes (2182 passed, 0 failed)pnpm run typecheck— clean