diff --git a/examples/app-router-cloudflare/app/api/error-route/route.ts b/examples/app-router-cloudflare/app/api/error-route/route.ts new file mode 100644 index 00000000..18cef592 --- /dev/null +++ b/examples/app-router-cloudflare/app/api/error-route/route.ts @@ -0,0 +1,5 @@ +// Route that throws an error — used by instrumentation e2e tests to verify +// that onRequestError() is called when a route handler throws. +export async function GET() { + throw new Error("Intentional route handler error"); +} diff --git a/examples/app-router-cloudflare/app/api/instrumentation-test/route.ts b/examples/app-router-cloudflare/app/api/instrumentation-test/route.ts new file mode 100644 index 00000000..c662ab6b --- /dev/null +++ b/examples/app-router-cloudflare/app/api/instrumentation-test/route.ts @@ -0,0 +1,29 @@ +import { NextResponse } from "next/server"; +import { + isRegisterCalled, + getCapturedErrors, + resetInstrumentationState, +} from "../../../instrumentation-state"; + +/** + * API route that exposes the current instrumentation state for e2e testing. + * + * GET /api/instrumentation-test + * Returns { registerCalled, errors } so Playwright tests can assert that + * instrumentation.ts register() was called on startup and that + * onRequestError() fired for any unhandled route errors. + * + * DELETE /api/instrumentation-test + * Resets the captured state so tests can start from a clean slate. + */ +export async function GET() { + return NextResponse.json({ + registerCalled: isRegisterCalled(), + errors: getCapturedErrors(), + }); +} + +export async function DELETE() { + resetInstrumentationState(); + return NextResponse.json({ ok: true }); +} diff --git a/examples/app-router-cloudflare/instrumentation-state.ts b/examples/app-router-cloudflare/instrumentation-state.ts new file mode 100644 index 00000000..96045bf4 --- /dev/null +++ b/examples/app-router-cloudflare/instrumentation-state.ts @@ -0,0 +1,51 @@ +/** + * Shared state for instrumentation.ts testing in app-router-cloudflare. + * + * ## Why plain module-level variables work here + * + * register() is now emitted as a top-level `await` inside the generated RSC + * entry module (see `generateRscEntry` in `app-dev-server.ts`). This means it + * runs inside the Cloudflare Worker process — the same process and module graph + * as the API routes. Plain module-level variables are therefore visible to both + * the instrumentation code and the API route that reads them. + * + * This is different from the old approach (writing to a temp file on disk) which + * was needed when register() ran in the host Node.js process and API routes ran + * in the miniflare Worker subprocess (two separate processes, no shared memory). + */ + +export type CapturedRequestError = { + message: string; + path: string; + method: string; + routerKind: string; + routePath: string; + routeType: string; +} + +/** Set to true when instrumentation.ts register() is called. */ +let registerCalled = false; + +/** List of errors captured by onRequestError(). */ +const capturedErrors: CapturedRequestError[] = []; + +export function isRegisterCalled(): boolean { + return registerCalled; +} + +export function getCapturedErrors(): CapturedRequestError[] { + return [...capturedErrors]; +} + +export function markRegisterCalled(): void { + registerCalled = true; +} + +export function recordRequestError(entry: CapturedRequestError): void { + capturedErrors.push(entry); +} + +export function resetInstrumentationState(): void { + registerCalled = false; + capturedErrors.length = 0; +} diff --git a/examples/app-router-cloudflare/instrumentation.ts b/examples/app-router-cloudflare/instrumentation.ts new file mode 100644 index 00000000..983abf17 --- /dev/null +++ b/examples/app-router-cloudflare/instrumentation.ts @@ -0,0 +1,52 @@ +/** + * instrumentation.ts for app-router-cloudflare example. + * + * This file exercises the instrumentation.ts feature with @cloudflare/vite-plugin. + * + * ## How it works (new approach) + * + * register() is emitted as a top-level `await` inside the generated RSC entry + * module by `generateRscEntry` in `app-dev-server.ts`. This means it runs: + * + * - Inside the Cloudflare Worker subprocess (miniflare) when + * @cloudflare/vite-plugin is present — the same process as the API routes. + * - Inside the RSC Vite environment when @vitejs/plugin-rsc is used standalone. + * + * In both cases, register() runs in the same process/environment as request + * handling, which is exactly what Next.js specifies: "called once when the + * server starts, before any request handling." + * + * ## State visibility + * + * Because register() and the API routes now run in the same Worker module graph, + * plain module-level variables in instrumentation-state.ts are shared between + * them. No temp-file bridge or globalThis tricks are needed. + */ + +import { + markRegisterCalled, + recordRequestError, +} from "./instrumentation-state"; + +export async function register(): Promise { + markRegisterCalled(); +} + +export async function onRequestError( + error: Error, + request: { path: string; method: string; headers: Record }, + context: { + routerKind: string; + routePath: string; + routeType: string; + }, +): Promise { + recordRequestError({ + message: error.message, + path: request.path, + method: request.method, + routerKind: context.routerKind, + routePath: context.routePath, + routeType: context.routeType, + }); +} diff --git a/examples/app-router-cloudflare/tsconfig.json b/examples/app-router-cloudflare/tsconfig.json index 3abab456..9c8d2a88 100644 --- a/examples/app-router-cloudflare/tsconfig.json +++ b/examples/app-router-cloudflare/tsconfig.json @@ -9,5 +9,5 @@ "skipLibCheck": true, "types": ["@cloudflare/workers-types"] }, - "include": ["app", "worker"] + "include": ["app", "worker", "*.ts"], } diff --git a/packages/vinext/src/global.d.ts b/packages/vinext/src/global.d.ts index 80fe8e4a..97b83d63 100644 --- a/packages/vinext/src/global.d.ts +++ b/packages/vinext/src/global.d.ts @@ -15,6 +15,7 @@ */ import type { Root } from "react-dom/client"; +import type { OnRequestErrorHandler } from "./server/instrumentation"; // --------------------------------------------------------------------------- // Window globals — browser-side state shared across module boundaries @@ -243,7 +244,19 @@ declare global { * is not yet available (e.g. during SSR of Link components). */ // eslint-disable-next-line no-var - var __VINEXT_DEFAULT_LOCALE__: string | undefined; + var __VINEXT_DEFAULT_LOCALE__: string | undefined; + + /** + * The onRequestError handler registered by instrumentation.ts. + * Set by the instrumentation.ts register() function. + * + * The handler is stored on `globalThis` so it is visible across the RSC and + * SSR Vite environments (separate module graphs, same Node.js process). With + * `@cloudflare/vite-plugin` it runs entirely inside the Worker, so + * `globalThis` is the Worker's global — also correct. + */ + // eslint-disable-next-line no-var + var __VINEXT_onRequestErrorHandler__: OnRequestErrorHandler | undefined; } // --------------------------------------------------------------------------- diff --git a/packages/vinext/src/index.ts b/packages/vinext/src/index.ts index dcaccea2..0c8f6374 100644 --- a/packages/vinext/src/index.ts +++ b/packages/vinext/src/index.ts @@ -2343,7 +2343,7 @@ hydrate(); headers: nextConfig?.headers, allowedOrigins: nextConfig?.serverActionsAllowedOrigins, allowedDevOrigins: nextConfig?.serverActionsAllowedOrigins, - }); + }, instrumentationPath); } if (id === RESOLVED_APP_SSR_ENTRY && hasAppDir) { return generateSsrEntry(); @@ -2523,7 +2523,13 @@ hydrate(); // Must be inside the returned function — ssrLoadModule() requires the // SSR environment's transport channel, which is not initialized until // after configureServer() returns. (See issue #167) - if (instrumentationPath) { + // + // App Router: register() is baked into the generated RSC entry as a + // top-level await, so it runs inside the Worker process (or RSC Vite + // environment) — the same process as request handling. Calling + // runInstrumentation() here too would run it a second time in the host + // process, which is wrong when @cloudflare/vite-plugin is present. + if (instrumentationPath && !hasAppDir) { runInstrumentation(server, instrumentationPath).catch((err) => { console.error("[vinext] Instrumentation error:", err); }); diff --git a/packages/vinext/src/server/app-dev-server.ts b/packages/vinext/src/server/app-dev-server.ts index 3210b746..4b7bb013 100644 --- a/packages/vinext/src/server/app-dev-server.ts +++ b/packages/vinext/src/server/app-dev-server.ts @@ -49,6 +49,7 @@ export function generateRscEntry( basePath?: string, trailingSlash?: boolean, config?: AppRouterConfig, + instrumentationPath?: string | null, ): string { const bp = basePath ?? ""; const ts = trailingSlash ?? false; @@ -220,6 +221,7 @@ import { ErrorBoundary, NotFoundBoundary } from "vinext/error-boundary"; import { LayoutSegmentProvider } from "vinext/layout-segment-context"; import { MetadataHead, mergeMetadata, resolveModuleMetadata, ViewportHead, mergeViewport, resolveModuleViewport } from "vinext/metadata"; ${middlewarePath ? `import * as middlewareModule from ${JSON.stringify(middlewarePath.replace(/\\/g, "/"))};` : ""} +${instrumentationPath ? `import * as _instrumentation from ${JSON.stringify(instrumentationPath.replace(/\\/g, "/"))};` : ""} ${effectiveMetaRoutes.length > 0 ? `import { sitemapToXml, robotsToText, manifestToJson } from ${JSON.stringify(fileURLToPath(new URL("./metadata-routes.js", import.meta.url)).replace(/\\/g, "/"))};` : ""} import { _consumeRequestScopedCacheLife, _runWithCacheState } from "next/cache"; import { runWithFetchCache } from "vinext/fetch-cache"; @@ -379,6 +381,23 @@ function rscOnError(error) { ${imports.join("\n")} +${instrumentationPath ? `// Run instrumentation register() once at module evaluation time — before any +// requests are handled. This runs inside the Worker process (or RSC environment), +// which is exactly where request handling happens. Matches Next.js semantics: +// register() is called once on startup in the process that handles requests. +if (typeof _instrumentation.register === "function") { + await _instrumentation.register(); +} +// Store the onRequestError handler on globalThis so it is visible to +// reportRequestError() (imported as _reportRequestError above) regardless +// of which Vite environment module graph it is called from. With +// @vitejs/plugin-rsc the RSC and SSR environments run in the same Node.js +// process and share globalThis. With @cloudflare/vite-plugin everything +// runs inside the Worker so globalThis is the Worker's global — also correct. +if (typeof _instrumentation.onRequestError === "function") { + globalThis.__VINEXT_onRequestErrorHandler__ = _instrumentation.onRequestError; +}` : ""} + const routes = [ ${routeEntries.join(",\n")} ]; diff --git a/packages/vinext/src/server/instrumentation.ts b/packages/vinext/src/server/instrumentation.ts index c5f4bfd6..5d6cb15f 100644 --- a/packages/vinext/src/server/instrumentation.ts +++ b/packages/vinext/src/server/instrumentation.ts @@ -11,6 +11,26 @@ * * References: * - https://nextjs.org/docs/app/building-your-application/optimizing/instrumentation + * + * ## App Router + * + * For App Router, `register()` is baked directly into the generated RSC entry + * as a top-level `await` at module evaluation time (see `app-dev-server.ts` + * `generateRscEntry`). This means it runs inside the Worker process (or RSC + * Vite environment) — the same process that handles requests — before any + * request is served. `runInstrumentation()` is NOT called from `configureServer` + * for App Router. + * + * The `onRequestError` handler is stored on `globalThis` so it is visible across + * the RSC and SSR Vite environments (separate module graphs, same Node.js process). + * With `@cloudflare/vite-plugin` it runs entirely inside the Worker, so + * `globalThis` is the Worker's global — also correct. + * + * ## Pages Router + * + * Pages Router has no RSC entry, so `configureServer()` is the right place to + * call `register()`. Pages Router always uses plain Vite + Node.js (never + * `@cloudflare/vite-plugin`), so `server.ssrLoadModule()` is safe here. */ import fs from "node:fs"; @@ -64,25 +84,30 @@ export type OnRequestErrorHandler = ( context: OnRequestErrorContext, ) => void | Promise; -/** Module-level reference to the onRequestError handler. */ -let _onRequestError: OnRequestErrorHandler | null = null; - /** * Get the registered onRequestError handler (if any). + * + * Reads from globalThis so it works across Vite environment boundaries. */ export function getOnRequestErrorHandler(): OnRequestErrorHandler | null { - return _onRequestError; + return globalThis.__VINEXT_onRequestErrorHandler__ ?? null; } /** - * Load and execute the instrumentation file. + * Load and execute the instrumentation file via Vite's SSR module loader. + * + * Called once during Pages Router server startup (`configureServer`). It: + * 1. Loads the instrumentation module via `server.ssrLoadModule()`. + * 2. Calls the `register()` function if exported. + * 3. Stores the `onRequestError()` handler on `globalThis` so it is visible + * to all Vite environment module graphs (SSR and the host process share + * the same Node.js `globalThis`). * - * This should be called once during server startup. It: - * 1. Loads the instrumentation module via Vite's SSR module loader - * 2. Calls the `register()` function if exported - * 3. Stores the `onRequestError()` handler if exported + * **App Router** does not use this function. For App Router, `register()` is + * emitted as a top-level `await` inside the generated RSC entry module so it + * runs in the same Worker/environment as request handling. * - * @param server - Vite dev server (for SSR module loading) + * @param server - Vite dev server (exposes `ssrLoadModule`) * @param instrumentationPath - Absolute path to the instrumentation file */ export async function runInstrumentation( @@ -97,9 +122,10 @@ export async function runInstrumentation( await mod.register(); } - // Store onRequestError handler if exported + // Store onRequestError handler on globalThis so environments can reach the + // same handler. if (typeof mod.onRequestError === "function") { - _onRequestError = mod.onRequestError as OnRequestErrorHandler; + globalThis.__VINEXT_onRequestErrorHandler__ = mod.onRequestError as OnRequestErrorHandler; } } catch (err) { console.error( @@ -113,15 +139,20 @@ export async function runInstrumentation( * Report a request error via the instrumentation handler. * * No-op if no onRequestError handler is registered. + * + * Reads the handler from globalThis so this function works correctly regardless + * of which environment it is called from. */ export async function reportRequestError( error: Error, request: { path: string; method: string; headers: Record }, context: OnRequestErrorContext, ): Promise { - if (!_onRequestError) return; + const handler = getOnRequestErrorHandler(); + if (!handler) return; + try { - await _onRequestError(error, request, context); + await handler(error, request, context); } catch (reportErr) { console.error( "[vinext] onRequestError handler threw:", diff --git a/playwright.config.ts b/playwright.config.ts index 0db57881..717556b2 100644 --- a/playwright.config.ts +++ b/playwright.config.ts @@ -19,6 +19,7 @@ const projectServers = { }, "app-router": { testDir: "./tests/e2e/app-router", + use: { baseURL: "http://localhost:4174" }, server: { command: "npx vite --port 4174", cwd: "./tests/fixtures/app-basic", @@ -51,7 +52,12 @@ const projectServers = { }, }, "cloudflare-workers": { - testDir: "./tests/e2e/cloudflare-workers", + testDir: "./tests/e2e", + testMatch: [ + "**/cloudflare-workers/**/*.spec.ts", + "**/app-router/instrumentation.spec.ts", + ], + use: { baseURL: "http://localhost:4176" }, server: { // Build app-router-cloudflare with Vite, then serve with wrangler dev (miniflare) command: @@ -90,9 +96,14 @@ export default defineConfig({ // Use chromium only — fast and sufficient for our tests browserName: "chromium", }, - projects: activeProjects.map((name) => ({ - name, - testDir: projectServers[name].testDir, - })), + projects: activeProjects.map((name) => { + const p = projectServers[name]; + return { + name, + testDir: p.testDir, + ...("testMatch" in p ? { testMatch: p.testMatch } : {}), + ...("use" in p ? { use: p.use } : {}), + }; + }), webServer: activeProjects.map((name) => projectServers[name].server), }); diff --git a/tests/e2e/app-router/instrumentation.spec.ts b/tests/e2e/app-router/instrumentation.spec.ts new file mode 100644 index 00000000..b4d51081 --- /dev/null +++ b/tests/e2e/app-router/instrumentation.spec.ts @@ -0,0 +1,121 @@ +/** + * E2E tests for instrumentation.ts support. + * + * Next.js calls register() once at server startup and onRequestError() whenever + * an unhandled error occurs during request handling. + * + * The app-basic fixture has instrumentation.ts at the project root which: + * - Sets a flag when register() is called + * - Records errors passed to onRequestError() + * + * The /api/instrumentation-test route exposes that state for assertions. + * + * References: + * - https://nextjs.org/docs/app/building-your-application/optimizing/instrumentation + */ + +import { test, expect } from "@playwright/test"; + +/** + * Tests that verify the startup state — these must NOT reset before running, + * because register() is called once at server startup and the flag won't be + * re-set after a DELETE reset. + */ +test.describe("instrumentation.ts startup", () => { + test("register() was called before the first request", async ({ + request, + }) => { + // Do NOT reset first — we want to see the state from server startup. + const res = await request.get("/api/instrumentation-test"); + expect(res.status()).toBe(200); + + const data = await res.json(); + // register() must have been invoked once when the dev server started. + expect(data.registerCalled).toBe(true); + expect(Array.isArray(data.errors)).toBe(true); + }); +}); + +/** + * Tests for onRequestError() behaviour — each test resets state first so + * errors from earlier tests don't bleed through. + */ +test.describe("instrumentation.ts onRequestError", () => { + test.beforeEach(async ({ request }) => { + // Reset captured state before each test so tests are independent. + // Note: this also resets registerCalled, which is why register() startup + // tests live in a separate describe block above. + const res = await request.delete("/api/instrumentation-test"); + expect(res.status()).toBe(200); + }); + + test("onRequestError() is called when a route handler throws", async ({ + request, + }) => { + // /api/error-route throws an unhandled Error — vinext should invoke + // the onRequestError handler registered in instrumentation.ts. + const errorRes = await request.get("/api/error-route"); + expect(errorRes.status()).toBe(500); + + // Give the async reportRequestError() call a moment to complete. + await new Promise((resolve) => setTimeout(resolve, 200)); + + const stateRes = await request.get("/api/instrumentation-test"); + expect(stateRes.status()).toBe(200); + + const data = await stateRes.json(); + expect(data.errors.length).toBeGreaterThanOrEqual(1); + + const err = data.errors[data.errors.length - 1]; + expect(err.message).toBe("Intentional route handler error"); + expect(err.path).toBe("/api/error-route"); + expect(err.method).toBe("GET"); + expect(err.routerKind).toBe("App Router"); + expect(err.routeType).toBe("route"); + }); + + test("onRequestError() receives the correct route path pattern", async ({ + request, + }) => { + const errorRes = await request.get("/api/error-route"); + expect(errorRes.status()).toBe(500); + + await new Promise((resolve) => setTimeout(resolve, 200)); + + const stateRes = await request.get("/api/instrumentation-test"); + const data = await stateRes.json(); + expect(data.errors.length).toBeGreaterThanOrEqual(1); + + const err = data.errors[data.errors.length - 1]; + // The routePath should be the file-system route pattern, not the concrete URL. + expect(err.routePath).toContain("error-route"); + }); + + test("multiple errors are captured independently", async ({ request }) => { + // Fire the error route twice and verify both entries are recorded. + await request.get("/api/error-route"); + await request.get("/api/error-route"); + + await new Promise((resolve) => setTimeout(resolve, 400)); + + const stateRes = await request.get("/api/instrumentation-test"); + const data = await stateRes.json(); + expect(data.errors.length).toBeGreaterThanOrEqual(2); + }); + + test("successful requests do not trigger onRequestError()", async ({ + request, + }) => { + // A normal successful API request should not produce an error entry. + const okRes = await request.get("/api/hello"); + expect(okRes.status()).toBe(200); + + await new Promise((resolve) => setTimeout(resolve, 200)); + + const stateRes = await request.get("/api/instrumentation-test"); + const data = await stateRes.json(); + // After the reset in beforeEach, no new errors should have been recorded + // by the successful /api/hello request. + expect(data.errors.length).toBe(0); + }); +}); diff --git a/tests/fixtures/app-basic/app/api/instrumentation-test/route.ts b/tests/fixtures/app-basic/app/api/instrumentation-test/route.ts new file mode 100644 index 00000000..c662ab6b --- /dev/null +++ b/tests/fixtures/app-basic/app/api/instrumentation-test/route.ts @@ -0,0 +1,29 @@ +import { NextResponse } from "next/server"; +import { + isRegisterCalled, + getCapturedErrors, + resetInstrumentationState, +} from "../../../instrumentation-state"; + +/** + * API route that exposes the current instrumentation state for e2e testing. + * + * GET /api/instrumentation-test + * Returns { registerCalled, errors } so Playwright tests can assert that + * instrumentation.ts register() was called on startup and that + * onRequestError() fired for any unhandled route errors. + * + * DELETE /api/instrumentation-test + * Resets the captured state so tests can start from a clean slate. + */ +export async function GET() { + return NextResponse.json({ + registerCalled: isRegisterCalled(), + errors: getCapturedErrors(), + }); +} + +export async function DELETE() { + resetInstrumentationState(); + return NextResponse.json({ ok: true }); +} diff --git a/tests/fixtures/app-basic/instrumentation-state.ts b/tests/fixtures/app-basic/instrumentation-state.ts new file mode 100644 index 00000000..efb2bf9c --- /dev/null +++ b/tests/fixtures/app-basic/instrumentation-state.ts @@ -0,0 +1,39 @@ +/** + * Shared in-memory state for instrumentation.ts testing. + */ + + export type CapturedRequestError = { + message: string; + path: string; + method: string; + routerKind: string; + routePath: string; + routeType: string; + } + + /** Set to true when instrumentation.ts register() is called. */ + let registerCalled = false; + + /** List of errors captured by onRequestError(). */ + const capturedErrors: CapturedRequestError[] = []; + + export function isRegisterCalled(): boolean { + return registerCalled; + } + + export function getCapturedErrors(): CapturedRequestError[] { + return [...capturedErrors]; + } + + export function markRegisterCalled(): void { + registerCalled = true; + } + + export function recordRequestError(entry: CapturedRequestError): void { + capturedErrors.push(entry); + } + + export function resetInstrumentationState(): void { + registerCalled = false; + capturedErrors.length = 0; + } diff --git a/tests/fixtures/app-basic/instrumentation.ts b/tests/fixtures/app-basic/instrumentation.ts new file mode 100644 index 00000000..0a860be6 --- /dev/null +++ b/tests/fixtures/app-basic/instrumentation.ts @@ -0,0 +1,38 @@ +/** + * instrumentation.ts for app-basic test fixture. + * + * Next.js calls register() once at server startup and onRequestError() + * whenever an unhandled error occurs during request handling. + * + * This file records calls into the shared instrumentation-state module + * so that the /api/instrumentation-test route can expose the state for + * e2e tests to assert against. + */ + +import { + markRegisterCalled, + recordRequestError, +} from "./instrumentation-state"; + +export async function register(): Promise { + markRegisterCalled(); +} + +export async function onRequestError( + error: Error, + request: { path: string; method: string; headers: Record }, + context: { + routerKind: string; + routePath: string; + routeType: string; + }, +): Promise { + recordRequestError({ + message: error.message, + path: request.path, + method: request.method, + routerKind: context.routerKind, + routePath: context.routePath, + routeType: context.routeType, + }); +} diff --git a/tests/instrumentation.test.ts b/tests/instrumentation.test.ts index d8e84265..be01a270 100644 --- a/tests/instrumentation.test.ts +++ b/tests/instrumentation.test.ts @@ -66,6 +66,10 @@ describe("runInstrumentation", () => { getOnRequestErrorHandler = mod.getOnRequestErrorHandler; }); + afterEach(() => { + delete globalThis.__VINEXT_onRequestErrorHandler__; + }) + it("calls register() when exported", async () => { const register = vi.fn(); const server = {