From 1fc036252e3891599db8f9d619429c7149de71c5 Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Mon, 7 Apr 2025 22:43:39 -0400 Subject: [PATCH 1/6] only add x-fah-middleware routes to routes that have middleware enabled --- .../adapter-nextjs/src/overrides.ts | 53 ++++++++++++------- 1 file changed, 33 insertions(+), 20 deletions(-) diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.ts b/packages/@apphosting/adapter-nextjs/src/overrides.ts index 48982951..ae109ad7 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.ts @@ -146,9 +146,12 @@ export async function validateNextConfigOverride( * Modifies the app's route manifest (routes-manifest.json) to add Firebase App Hosting * specific overrides (i.e headers). * - * This function adds the following headers to all routes: + * It adds the following headers to all routes: * - x-fah-adapter: The Firebase App Hosting adapter version used to build the app. + * + * It also adds the following headers to all routes for which middleware is enabled: * - x-fah-middleware: When middleware is enabled. + * * @param appPath The path to the app directory. * @param distDir The path to the dist directory. * @param adapterMetadata The adapter metadata. @@ -158,8 +161,9 @@ export async function addRouteOverrides( distDir: string, adapterMetadata: AdapterMetadata, ) { - const middlewareManifest = loadMiddlewareManifest(appPath, distDir); const routeManifest = loadRouteManifest(appPath, distDir); + + // Add the adapter version to all routes routeManifest.headers.push({ source: "/:path*", headers: [ @@ -167,29 +171,38 @@ export async function addRouteOverrides( key: "x-fah-adapter", value: `nextjs-${adapterMetadata.adapterVersion}`, }, - ...(middlewareExists(middlewareManifest) - ? [ - { - key: "x-fah-middleware", - value: "true", - }, - ] - : []), ], /* - NextJs converts the source string to a regex using path-to-regexp (https://github.com/pillarjs/path-to-regexp) at - build time: https://github.com/vercel/next.js/blob/canary/packages/next/src/build/index.ts#L1273. - This regex is then used to match the route against the request path. + NextJs converts the source string to a regex using path-to-regexp (https://github.com/pillarjs/path-to-regexp) at + build time: https://github.com/vercel/next.js/blob/canary/packages/next/src/build/index.ts#L1273. + This regex is then used to match the route against the request path. - This regex was generated by building a sample NextJs app with the source string `/:path*` and then inspecting the - routes-manifest.json file. - */ + This regex was generated by building a sample NextJs app with the source string `/:path*` and then inspecting the + routes-manifest.json file. + */ regex: "^(?:/((?:[^/]+?)(?:/(?:[^/]+?))*))?(?:/)?$", }); - await writeRouteManifest(appPath, distDir, routeManifest); -} + // Add the middleware header to all routes for which middleware is enabled + const middlewareManifest = loadMiddlewareManifest(appPath, distDir); + const rootMiddleware = middlewareManifest.middleware["/"]; + if (!rootMiddleware?.matchers) { + console.log("No middleware found for root path, skipping route overrides"); + return; + } -function middlewareExists(middlewareManifest: MiddlewareManifest) { - return Object.keys(middlewareManifest.middleware).length > 0; + rootMiddleware.matchers.forEach((matcher) => { + routeManifest.headers.push({ + source: matcher.regexp, + headers: [ + { + key: "x-fah-middleware", + value: "true", + }, + ], + regex: matcher.regexp, + }); + }); + + await writeRouteManifest(appPath, distDir, routeManifest); } From f483d340292247671e1464b54ec958b638e371b0 Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Wed, 9 Apr 2025 15:39:37 +0000 Subject: [PATCH 2/6] start tests --- .../adapter-nextjs/src/overrides.spec.ts | 40 ++++++++++--------- 1 file changed, 22 insertions(+), 18 deletions(-) diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts b/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts index 3d478b18..bb1cca76 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts @@ -85,7 +85,7 @@ describe("route overrides", () => { assert.deepStrictEqual(updatedManifest, expectedManifest); }); - it("should add middleware header when middleware exists", async () => { + it("should add middleware header only to routes for which middleware is enabled", async () => { const { addRouteOverrides } = await importOverrides; const initialManifest: RoutesManifest = { version: 3, @@ -109,8 +109,8 @@ describe("route overrides", () => { page: "/", matchers: [ { - regexp: "^/.*$", - originalSource: "/:path*", + regexp: "/hello", + originalSource: "/hello", }, ], }, @@ -150,9 +150,13 @@ describe("route overrides", () => { key: "x-fah-adapter", value: "nextjs-1.0.0", }, - { key: "x-fah-middleware", value: "true" }, ], }, + { + source: "/hello", + regex: "/hello", + headers: [{ key: "x-fah-middleware", value: "true" }], + }, ], }; @@ -172,13 +176,13 @@ describe("next config overrides", () => { ...config, images: { ...(config.images || {}), - ...(config.images?.unoptimized === undefined && config.images?.loader === undefined - ? { unoptimized: true } + ...(config.images?.unoptimized === undefined && config.images?.loader === undefined + ? { unoptimized: true } : {}), }, }); - const config = typeof originalConfig === 'function' + const config = typeof originalConfig === 'function' ? async (...args) => { const resolvedConfig = await originalConfig(...args); return fahOptimizedConfig(resolvedConfig); @@ -194,12 +198,12 @@ describe("next config overrides", () => { const { overrideNextConfig } = await importOverrides; const originalConfig = ` // @ts-check - + /** @type {import('next').NextConfig} */ const nextConfig = { /* config options here */ } - + module.exports = nextConfig `; @@ -213,7 +217,7 @@ describe("next config overrides", () => { normalizeWhitespace(` // @ts-nocheck const originalConfig = require('./next.config.original.js'); - + ${nextConfigOverrideBody} module.exports = config; @@ -225,14 +229,14 @@ describe("next config overrides", () => { const { overrideNextConfig } = await importOverrides; const originalConfig = ` // @ts-check - + /** * @type {import('next').NextConfig} */ const nextConfig = { /* config options here */ } - + export default nextConfig `; @@ -257,7 +261,7 @@ describe("next config overrides", () => { const { overrideNextConfig } = await importOverrides; const originalConfig = ` // @ts-check - + export default (phase, { defaultConfig }) => { /** * @type {import('next').NextConfig} @@ -280,7 +284,7 @@ describe("next config overrides", () => { import originalConfig from './next.config.original.mjs'; ${nextConfigOverrideBody} - + export default config; `), ); @@ -290,11 +294,11 @@ describe("next config overrides", () => { const { overrideNextConfig } = await importOverrides; const originalConfig = ` import type { NextConfig } from 'next' - + const nextConfig: NextConfig = { /* config options here */ } - + export default nextConfig `; @@ -307,9 +311,9 @@ describe("next config overrides", () => { normalizeWhitespace(` // @ts-nocheck import originalConfig from './next.config.original'; - + ${nextConfigOverrideBody} - + module.exports = config; `), ); From 306ddcf9d4296fd926ff43fd0bef6eb596af899c Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Thu, 10 Apr 2025 20:52:48 +0000 Subject: [PATCH 3/6] fix bug and fix unit tests --- .../adapter-nextjs/src/overrides.spec.ts | 2 +- .../adapter-nextjs/src/overrides.ts | 31 +++++++++---------- 2 files changed, 16 insertions(+), 17 deletions(-) diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts b/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts index bb1cca76..d5ea7928 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.spec.ts @@ -130,7 +130,7 @@ describe("route overrides", () => { fs.readFileSync(routesManifestPath, "utf-8"), ) as RoutesManifest; - assert.strictEqual(updatedManifest.headers.length, 1); + assert.strictEqual(updatedManifest.headers.length, 2); const expectedManifest: RoutesManifest = { version: 3, diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.ts b/packages/@apphosting/adapter-nextjs/src/overrides.ts index ae109ad7..ec107c4f 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.ts @@ -186,23 +186,22 @@ export async function addRouteOverrides( // Add the middleware header to all routes for which middleware is enabled const middlewareManifest = loadMiddlewareManifest(appPath, distDir); const rootMiddleware = middlewareManifest.middleware["/"]; - if (!rootMiddleware?.matchers) { - console.log("No middleware found for root path, skipping route overrides"); - return; - } - - rootMiddleware.matchers.forEach((matcher) => { - routeManifest.headers.push({ - source: matcher.regexp, - headers: [ - { - key: "x-fah-middleware", - value: "true", - }, - ], - regex: matcher.regexp, + if (rootMiddleware?.matchers) { + console.log("Middleware detected, adding middleware headers to matching routes"); + + rootMiddleware.matchers.forEach((matcher) => { + routeManifest.headers.push({ + source: matcher.regexp, + headers: [ + { + key: "x-fah-middleware", + value: "true", + }, + ], + regex: matcher.regexp, + }); }); - }); + } await writeRouteManifest(appPath, distDir, routeManifest); } From 4d71f9c1a315623daa1ac5e7b694fd307f9f242d Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Fri, 11 Apr 2025 16:52:20 +0000 Subject: [PATCH 4/6] fixup e2e tests --- .../adapter-nextjs/e2e/middleware.spec.ts | 16 +++++++++++++++- .../@apphosting/adapter-nextjs/e2e/run-local.ts | 6 +++--- .../@apphosting/adapter-nextjs/src/overrides.ts | 2 +- 3 files changed, 19 insertions(+), 5 deletions(-) diff --git a/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts b/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts index 74477f53..e9922aee 100644 --- a/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts +++ b/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts @@ -15,7 +15,7 @@ before(() => { }); describe("middleware", () => { - it("should have x-fah-adapter header and x-fah-middleware header on all routes", async () => { + it("should have x-fah-adapter header on all routes and x-fah-middleware header on all routes", async () => { const routes = [ "/", "/ssg", @@ -40,4 +40,18 @@ describe("middleware", () => { ); } }); + + it("should have x-fah-middleware header on middleware routes", async () => { + // Middleware is configured to run on these routes via run-local.ts with-middleware setup function + const routes = ["/ssg", "/ssr"]; + + for (const route of routes) { + const response = await fetch(posix.join(host, route)); + assert.equal( + response.headers.get("x-fah-middleware"), + "true", + `Route ${route} missing x-fah-middleware header`, + ); + } + }); }); diff --git a/packages/@apphosting/adapter-nextjs/e2e/run-local.ts b/packages/@apphosting/adapter-nextjs/e2e/run-local.ts index 23104afa..a46d2a93 100644 --- a/packages/@apphosting/adapter-nextjs/e2e/run-local.ts +++ b/packages/@apphosting/adapter-nextjs/e2e/run-local.ts @@ -35,15 +35,15 @@ const scenarios: Scenario[] = [ setup: async (cwd: string) => { // Create a middleware.ts file const middlewareContent = ` - import type { NextRequest } from 'next/server' + import type { NextRequest } from "next/server"; export function middleware(request: NextRequest) { // This is a simple middleware that doesn't modify the request - console.log('Middleware executed', request.nextUrl.pathname); + console.log("Middleware executed", request.nextUrl.pathname); } export const config = { - matcher: '/((?!api|_next/static|_next/image|favicon.ico).*)', + matcher: ["/ssg", "/ssr"], }; `; diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.ts b/packages/@apphosting/adapter-nextjs/src/overrides.ts index ec107c4f..b37e9077 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.ts @@ -191,7 +191,7 @@ export async function addRouteOverrides( rootMiddleware.matchers.forEach((matcher) => { routeManifest.headers.push({ - source: matcher.regexp, + source: matcher.originalSource, headers: [ { key: "x-fah-middleware", From b6bfd2b288cf71a973a68fd67f887d66dd79c9e9 Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Fri, 11 Apr 2025 16:54:27 +0000 Subject: [PATCH 5/6] fix lint error --- packages/@apphosting/adapter-nextjs/src/overrides.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/packages/@apphosting/adapter-nextjs/src/overrides.ts b/packages/@apphosting/adapter-nextjs/src/overrides.ts index b37e9077..42ff210a 100644 --- a/packages/@apphosting/adapter-nextjs/src/overrides.ts +++ b/packages/@apphosting/adapter-nextjs/src/overrides.ts @@ -1,4 +1,4 @@ -import { AdapterMetadata, MiddlewareManifest } from "./interfaces.js"; +import { AdapterMetadata } from "./interfaces.js"; import { loadRouteManifest, writeRouteManifest, From 1bf91fce70b3d8672a2cf1289ed622e57b1f3809 Mon Sep 17 00:00:00 2001 From: Mathusan Selvarajah Date: Fri, 11 Apr 2025 16:58:19 +0000 Subject: [PATCH 6/6] fix e2e --- packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts b/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts index e9922aee..4e1cd930 100644 --- a/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts +++ b/packages/@apphosting/adapter-nextjs/e2e/middleware.spec.ts @@ -15,7 +15,7 @@ before(() => { }); describe("middleware", () => { - it("should have x-fah-adapter header on all routes and x-fah-middleware header on all routes", async () => { + it("should have x-fah-adapter header on all routes", async () => { const routes = [ "/", "/ssg", @@ -33,11 +33,6 @@ describe("middleware", () => { `nextjs-${adapterVersion}`, `Route ${route} missing x-fah-adapter header`, ); - assert.equal( - response.headers.get("x-fah-middleware"), - "true", - `Route ${route} missing x-fah-middleware header`, - ); } });