From 0a303e8297129cc8ec79e5f4c2320cc66fa6b50a Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Wed, 4 Sep 2024 14:13:33 +0200 Subject: [PATCH 01/11] Skipping partial html files generated due to enabling ppr in Nextjs --- src/frameworks/next/index.ts | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/frameworks/next/index.ts b/src/frameworks/next/index.ts index dcc88846ca3..ec059a3dfe8 100644 --- a/src/frameworks/next/index.ts +++ b/src/frameworks/next/index.ts @@ -536,6 +536,15 @@ export async function ɵcodegenPublicDirectory( let defaultDestPath = isDefaultLocale && join(destDir, basePath, ...destPartsOrIndex); if (!fileExistsSync(sourcePath) && fileExistsSync(`${sourcePath}.html`)) { sourcePath += ".html"; + // Check if the HTML file is partial (doesn't have closing tag) + const content = await readFile(sourcePath, "utf8"); + if (!content.includes("")) { + logger.debug( + `Skipping ${path} as it appears to be a partial HTML file which is not supported by Firebase Hosting`, + ); + return; + } + if (localizedDestPath) localizedDestPath += ".html"; if (defaultDestPath) defaultDestPath += ".html"; } else if ( From 703b6a96b462671584c6f5e3b0468f3e913cab8f Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 12 Sep 2024 23:29:22 +0200 Subject: [PATCH 02/11] Detect partial html files in build and add them to reasonsForBackend. - Move common logic to utils functions --- src/frameworks/next/index.ts | 55 +++++++++++++++++----------------- src/frameworks/next/utils.ts | 57 ++++++++++++++++++++++++++++++++++++ 2 files changed, 85 insertions(+), 27 deletions(-) diff --git a/src/frameworks/next/index.ts b/src/frameworks/next/index.ts index ec059a3dfe8..0d966531e17 100644 --- a/src/frameworks/next/index.ts +++ b/src/frameworks/next/index.ts @@ -58,6 +58,10 @@ import { whichNextConfigFile, installEsbuild, findEsbuildPath, + createPagesManifestLikePrerender, + isPartialHTML, + getAppPathRoute, + getContentDist, } from "./utils"; import { NODE_VERSION, NPM_COMMAND_TIMEOUT_MILLIES, SHARP_VERSION, I18N_ROOT } from "../constants"; import type { @@ -290,6 +294,24 @@ export async function build( reasonsForBackend.add(`route with server action ${key}`); } } + + const allRoutes: PrerenderManifest["routes"] = { + ...prerenderManifest.routes, + ...createPagesManifestLikePrerender(pagesManifestJSON), + }; + + for (const [path, route] of Object.entries(allRoutes)) { + const appPathRoute = getAppPathRoute(route, appPathRoutesManifest); + const contentDist = getContentDist(dir, distDir, appPathRoute); + const sourceParts = path.split("/").filter((it) => !!it); + const sourcePartsOrIndex = sourceParts.length > 0 ? sourceParts : ["index"]; + + const sourcePath = join(contentDist, ...sourcePartsOrIndex); + + if (await isPartialHTML(`${sourcePath}.html`)) { + reasonsForBackend.add(`partial HTML file ${path}`); + } + } } const isEveryRedirectSupported = nextJsRedirects @@ -432,8 +454,6 @@ export async function ɵcodegenPublicDirectory( ), ]); - const appPathRoutesEntries = Object.entries(appPathRoutesManifest); - const middlewareMatcherRegexes = getMiddlewareMatcherRegexes(middlewareManifest); const { redirects = [], rewrites = [], headers = [] } = routesManifest; @@ -465,26 +485,9 @@ export async function ɵcodegenPublicDirectory( appPathRoutesManifest, ); - const pagesManifestLikePrerender: PrerenderManifest["routes"] = Object.fromEntries( - Object.entries(pagesManifest) - .filter(([, srcRoute]) => srcRoute.endsWith(".html")) - .map(([path]) => { - return [ - path, - { - srcRoute: null, - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }, - ]; - }), - ); - const routesToCopy: PrerenderManifest["routes"] = { ...prerenderManifest.routes, - ...pagesManifestLikePrerender, + ...createPagesManifestLikePrerender(pagesManifest), }; await Promise.all( @@ -505,9 +508,8 @@ export async function ɵcodegenPublicDirectory( return; } - const appPathRoute = - route.srcRoute && appPathRoutesEntries.find(([, it]) => it === route.srcRoute)?.[0]; - const contentDist = join(sourceDir, distDir, "server", appPathRoute ? "app" : "pages"); + const appPathRoute = getAppPathRoute(route, appPathRoutesManifest || {}); + const contentDist = getContentDist(sourceDir, distDir, appPathRoute); const sourceParts = path.split("/").filter((it) => !!it); const locale = i18n?.locales.includes(sourceParts[0]) ? sourceParts[0] : undefined; @@ -536,11 +538,10 @@ export async function ɵcodegenPublicDirectory( let defaultDestPath = isDefaultLocale && join(destDir, basePath, ...destPartsOrIndex); if (!fileExistsSync(sourcePath) && fileExistsSync(`${sourcePath}.html`)) { sourcePath += ".html"; - // Check if the HTML file is partial (doesn't have closing tag) - const content = await readFile(sourcePath, "utf8"); - if (!content.includes("")) { + // Check if the HTML file is partial + if (await isPartialHTML(sourcePath)) { logger.debug( - `Skipping ${path} as it appears to be a partial HTML file which is not supported by Firebase Hosting`, + `skipping ${path} as it appears to be a partial HTML file which is not supported by Firebase Hosting`, ); return; } diff --git a/src/frameworks/next/utils.ts b/src/frameworks/next/utils.ts index 2ead4fb8d30..ff827c5ebb2 100644 --- a/src/frameworks/next/utils.ts +++ b/src/frameworks/next/utils.ts @@ -37,6 +37,7 @@ import { dirExistsSync, fileExistsSync } from "../../fsutils"; import { IS_WINDOWS } from "../../utils"; import { execSync } from "child_process"; import { FirebaseError } from "../../error"; +import { PrerenderManifest } from "next/dist/build"; export const I18N_SOURCE = /\/:nextInternalLocale(\([^\)]+\))?/; @@ -549,3 +550,59 @@ export function installEsbuild(version: string): void { } } } + +/** + * Create a PrerenderManifest["routes"] object from PagesManifest + */ +export function createPagesManifestLikePrerender( + pagesManifestJSON: PagesManifest, +): PrerenderManifest["routes"] { + const pagesManifestLikePrerender: PrerenderManifest["routes"] = {}; + for (const [path, route] of Object.entries(pagesManifestJSON)) { + if (typeof route === "string" && route.endsWith(".html")) { + pagesManifestLikePrerender[path] = { + srcRoute: null, + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }; + } + } + return pagesManifestLikePrerender; +} + +/** + * Check if an HTML file is partial (doesn't have closing tag) + */ +export async function isPartialHTML(filePath: string): Promise { + try { + const content = await readFile(filePath, "utf8"); + return !content.includes(""); + } catch (error) { + console.error(`Error reading file ${filePath}:`, error); + // Assume it's not partial if we can't read the file + return false; + } +} + +/** + * Get the app path route for a given route + */ +export function getAppPathRoute( + route: PrerenderManifest["routes"][string], + appPathRoutesManifest: AppPathRoutesManifest, +): string | undefined { + if (!route.srcRoute) { + return undefined; + } + const entry = Object.entries(appPathRoutesManifest).find(([, it]) => it === route.srcRoute); + return entry ? entry[0] : undefined; +} + +/** + * Get the content distribution directory for a given route + */ +export function getContentDist(sourceDir: string, distDir: string, appPathRoute?: string): string { + return join(sourceDir, distDir, "server", appPathRoute ? "app" : "pages"); +} From c4eedfec2dc2b58addf87ea7c25072947318beb5 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 12 Sep 2024 23:29:38 +0200 Subject: [PATCH 03/11] Add tests cases for new utils functions --- src/frameworks/next/utils.spec.ts | 135 ++++++++++++++++++++++++++++++ 1 file changed, 135 insertions(+) diff --git a/src/frameworks/next/utils.spec.ts b/src/frameworks/next/utils.spec.ts index f82713bd8ed..ff357a3f7db 100644 --- a/src/frameworks/next/utils.spec.ts +++ b/src/frameworks/next/utils.spec.ts @@ -6,6 +6,7 @@ import * as sinon from "sinon"; import * as glob from "glob"; import * as childProcess from "child_process"; import { FirebaseError } from "../../error"; +import * as path from "path"; import { EXPORT_MARKER, @@ -38,6 +39,10 @@ import { getRoutesWithServerAction, findEsbuildPath, installEsbuild, + createPagesManifestLikePrerender, + isPartialHTML, + getAppPathRoute, + getContentDist, } from "./utils"; import * as frameworksUtils from "../utils"; @@ -72,6 +77,9 @@ import { serverReferenceManifest, } from "./testing"; import { pathsWithCustomRoutesInternalPrefix } from "./testing/i18n"; +import { AppPathRoutesManifest } from "./interfaces"; +import { PrerenderManifest } from "next/dist/build"; +import { PagesManifest } from "next/dist/build/webpack/plugins/pages-manifest-plugin"; describe("Next.js utils", () => { describe("cleanEscapedChars", () => { @@ -626,4 +634,131 @@ describe("Next.js utils", () => { } }); }); + + describe("createPagesManifestLikePrerender", () => { + it("should create a PrerenderManifest routes object from PagesManifest", () => { + const pagesManifest: PagesManifest = { + "/": "pages/index.html", + "/about": "pages/about.html", + "/api/data": "pages/api/data.js", + }; + + const result = createPagesManifestLikePrerender(pagesManifest); + + expect(result).to.deep.equal({ + "/": { + srcRoute: null, + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }, + "/about": { + srcRoute: null, + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }, + }); + }); + }); + + describe("isPartialHTML", () => { + let sandbox: sinon.SinonSandbox; + + beforeEach(() => { + sandbox = sinon.createSandbox(); + }); + + afterEach(() => { + sandbox.restore(); + }); + + it("should return true for partial HTML", async () => { + sandbox.stub(fsPromises, "readFile").resolves("Partial"); + expect(await isPartialHTML("partial.html")).to.be.true; + }); + + it("should return false for complete HTML", async () => { + sandbox.stub(fsPromises, "readFile").resolves("Complete"); + expect(await isPartialHTML("complete.html")).to.be.false; + }); + + it("should return false and log error if file read fails", async () => { + sandbox.stub(fsPromises, "readFile").rejects(new Error("File not found")); + const consoleErrorStub = sandbox.stub(console, "error"); + + expect(await isPartialHTML("nonexistent.html")).to.be.false; + expect(consoleErrorStub.calledOnce).to.be.true; + }); + }); + + describe("getAppPathRoute", () => { + it("should return the correct app path route", () => { + const route: PrerenderManifest["routes"][string] = { + srcRoute: "/app/page", + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }; + const appPathRoutesManifest: AppPathRoutesManifest = { + "/": "/app/page", + "/about": "/app/about/page", + }; + + expect(getAppPathRoute(route, appPathRoutesManifest)).to.equal("/"); + }); + + it("should return undefined if no matching route is found", () => { + const route: PrerenderManifest["routes"][string] = { + srcRoute: "/app/nonexistent", + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }; + const appPathRoutesManifest: AppPathRoutesManifest = { + "/": "/app/page", + "/about": "/app/about/page", + }; + + expect(getAppPathRoute(route, appPathRoutesManifest)).to.be.undefined; + }); + + it("should return undefined if srcRoute is null", () => { + const route: PrerenderManifest["routes"][string] = { + srcRoute: null, + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }; + const appPathRoutesManifest: AppPathRoutesManifest = { + "/": "/app/page", + }; + + expect(getAppPathRoute(route, appPathRoutesManifest)).to.be.undefined; + }); + }); + + describe("getContentDist", () => { + it("should return the correct path for app directory", () => { + const sourceDir = "/project"; + const distDir = ".next"; + const appPathRoute = "/app/page"; + + const expected = path.join("/project", ".next", "server", "app"); + expect(getContentDist(sourceDir, distDir, appPathRoute)).to.equal(expected); + }); + + it("should return the correct path for pages directory", () => { + const sourceDir = "/project"; + const distDir = ".next"; + + const expected = path.join("/project", ".next", "server", "pages"); + expect(getContentDist(sourceDir, distDir)).to.equal(expected); + }); + }); }); From 5aab663f17907d2f4c6ef76ef2d7f95705d0e140 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 4 Oct 2024 14:44:06 +0200 Subject: [PATCH 04/11] Check if file .html exists before trying to read it. --- src/frameworks/next/utils.spec.ts | 17 +++++++++++++++-- src/frameworks/next/utils.ts | 7 ++++++- 2 files changed, 21 insertions(+), 3 deletions(-) diff --git a/src/frameworks/next/utils.spec.ts b/src/frameworks/next/utils.spec.ts index b4b425316c9..9f89738b2be 100644 --- a/src/frameworks/next/utils.spec.ts +++ b/src/frameworks/next/utils.spec.ts @@ -676,21 +676,34 @@ describe("Next.js utils", () => { }); it("should return true for partial HTML", async () => { + sandbox.stub(fsExtra, "pathExists").resolves(true); sandbox.stub(fsPromises, "readFile").resolves("Partial"); expect(await isPartialHTML("partial.html")).to.be.true; }); it("should return false for complete HTML", async () => { + sandbox.stub(fsExtra, "pathExists").resolves(true); sandbox.stub(fsPromises, "readFile").resolves("Complete"); expect(await isPartialHTML("complete.html")).to.be.false; }); + it("should return false if file does not exist", async () => { + sandbox.stub(fsExtra, "pathExists").resolves(false); + const consoleWarnStub = sandbox.stub(console, "warn"); + + expect(await isPartialHTML("nonexistent.html")).to.be.false; + expect(consoleWarnStub.calledOnce).to.be.true; + expect(consoleWarnStub.firstCall.args[0]).to.include("File does not exist"); + }); + it("should return false and log error if file read fails", async () => { - sandbox.stub(fsPromises, "readFile").rejects(new Error("File not found")); + sandbox.stub(fsExtra, "pathExists").resolves(true); + sandbox.stub(fsPromises, "readFile").rejects(new Error("File read error")); const consoleErrorStub = sandbox.stub(console, "error"); - expect(await isPartialHTML("nonexistent.html")).to.be.false; + expect(await isPartialHTML("error.html")).to.be.false; expect(consoleErrorStub.calledOnce).to.be.true; + expect(consoleErrorStub.firstCall.args[0]).to.include("Error processing file"); }); }); diff --git a/src/frameworks/next/utils.ts b/src/frameworks/next/utils.ts index 3d637c4c005..bea284c424e 100644 --- a/src/frameworks/next/utils.ts +++ b/src/frameworks/next/utils.ts @@ -577,10 +577,15 @@ export function createPagesManifestLikePrerender( */ export async function isPartialHTML(filePath: string): Promise { try { + // Check if the file exists before attempting to read it + if (!(await pathExists(filePath))) { + return false; + } + const content = await readFile(filePath, "utf8"); return !content.includes(""); } catch (error) { - console.error(`Error reading file ${filePath}:`, error); + console.error(`Error processing file ${filePath}:`, error); // Assume it's not partial if we can't read the file return false; } From c86c96a97f2ea3d58a76120f19a0fa062323d2e4 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 4 Oct 2024 15:03:27 +0200 Subject: [PATCH 05/11] Remove warning if file does not exist --- src/frameworks/next/utils.spec.ts | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/frameworks/next/utils.spec.ts b/src/frameworks/next/utils.spec.ts index 9f89738b2be..b304f821b77 100644 --- a/src/frameworks/next/utils.spec.ts +++ b/src/frameworks/next/utils.spec.ts @@ -689,11 +689,8 @@ describe("Next.js utils", () => { it("should return false if file does not exist", async () => { sandbox.stub(fsExtra, "pathExists").resolves(false); - const consoleWarnStub = sandbox.stub(console, "warn"); expect(await isPartialHTML("nonexistent.html")).to.be.false; - expect(consoleWarnStub.calledOnce).to.be.true; - expect(consoleWarnStub.firstCall.args[0]).to.include("File does not exist"); }); it("should return false and log error if file read fails", async () => { From 74ec984c099faa5f1234a492f2fd8f9e0cdc999e Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Fri, 4 Oct 2024 15:09:15 +0200 Subject: [PATCH 06/11] Changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index e93b2cf976d..d8e382e1fdf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,2 +1,3 @@ - Added emulator support for extensions defined by SDK. - Various emulator bug fixes. +- Added Next.js utils for checking if an HTML file is partial. From f04ed224f36b8828b7d6301869ecdc5c82032261 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Wed, 9 Oct 2024 16:52:48 +0200 Subject: [PATCH 07/11] Promise.all test isPartialHTML --- src/frameworks/next/index.ts | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/frameworks/next/index.ts b/src/frameworks/next/index.ts index 0d966531e17..793fd4b1d53 100644 --- a/src/frameworks/next/index.ts +++ b/src/frameworks/next/index.ts @@ -300,18 +300,18 @@ export async function build( ...createPagesManifestLikePrerender(pagesManifestJSON), }; - for (const [path, route] of Object.entries(allRoutes)) { - const appPathRoute = getAppPathRoute(route, appPathRoutesManifest); - const contentDist = getContentDist(dir, distDir, appPathRoute); - const sourceParts = path.split("/").filter((it) => !!it); - const sourcePartsOrIndex = sourceParts.length > 0 ? sourceParts : ["index"]; - - const sourcePath = join(contentDist, ...sourcePartsOrIndex); - - if (await isPartialHTML(`${sourcePath}.html`)) { - reasonsForBackend.add(`partial HTML file ${path}`); - } - } + await Promise.all( + Object.entries(allRoutes).map(async ([path, route]) => { + const appPathRoute = getAppPathRoute(route, appPathRoutesManifest); + const contentDist = getContentDist(dir, distDir, appPathRoute); + const sourceParts = path.split("/").filter((it) => !!it); + const sourcePartsOrIndex = sourceParts.length > 0 ? sourceParts : ["index"]; + const sourcePath = join(contentDist, ...sourcePartsOrIndex); + if (await isPartialHTML(`${sourcePath}.html`)) { + reasonsForBackend.add(`partial HTML file ${path}`); + } + }), + ); } const isEveryRedirectSupported = nextJsRedirects From 47c3def54dc9418816d0b0c041fdc1350cc91802 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 17 Oct 2024 13:55:47 +0200 Subject: [PATCH 08/11] Refactor to use .meta files for PPR --- src/frameworks/next/index.ts | 73 +++++----- src/frameworks/next/testing/app.ts | 1 + src/frameworks/next/utils.spec.ts | 208 ++++++----------------------- src/frameworks/next/utils.ts | 76 ++--------- 4 files changed, 94 insertions(+), 264 deletions(-) diff --git a/src/frameworks/next/index.ts b/src/frameworks/next/index.ts index 793fd4b1d53..9a31ea4a376 100644 --- a/src/frameworks/next/index.ts +++ b/src/frameworks/next/index.ts @@ -49,7 +49,7 @@ import { getMiddlewareMatcherRegexes, getNonStaticRoutes, getNonStaticServerComponents, - getHeadersFromMetaFiles, + getAppMetadataFromMetaFiles, cleanI18n, getNextVersion, hasStaticAppNotFoundComponent, @@ -58,10 +58,6 @@ import { whichNextConfigFile, installEsbuild, findEsbuildPath, - createPagesManifestLikePrerender, - isPartialHTML, - getAppPathRoute, - getContentDist, } from "./utils"; import { NODE_VERSION, NPM_COMMAND_TIMEOUT_MILLIES, SHARP_VERSION, I18N_ROOT } from "../constants"; import type { @@ -256,7 +252,7 @@ export async function build( ]); if (appPathRoutesManifest) { - const headersFromMetaFiles = await getHeadersFromMetaFiles( + const { headers: headersFromMetaFiles, pprRoutes } = await getAppMetadataFromMetaFiles( dir, distDir, baseUrl, @@ -264,6 +260,10 @@ export async function build( ); headers.push(...headersFromMetaFiles); + for (const route of pprRoutes) { + reasonsForBackend.add(`route with ppr ${route}`); + } + if (appPathsManifest) { const unrenderedServerComponents = getNonStaticServerComponents( appPathsManifest, @@ -294,24 +294,6 @@ export async function build( reasonsForBackend.add(`route with server action ${key}`); } } - - const allRoutes: PrerenderManifest["routes"] = { - ...prerenderManifest.routes, - ...createPagesManifestLikePrerender(pagesManifestJSON), - }; - - await Promise.all( - Object.entries(allRoutes).map(async ([path, route]) => { - const appPathRoute = getAppPathRoute(route, appPathRoutesManifest); - const contentDist = getContentDist(dir, distDir, appPathRoute); - const sourceParts = path.split("/").filter((it) => !!it); - const sourcePartsOrIndex = sourceParts.length > 0 ? sourceParts : ["index"]; - const sourcePath = join(contentDist, ...sourcePartsOrIndex); - if (await isPartialHTML(`${sourcePath}.html`)) { - reasonsForBackend.add(`partial HTML file ${path}`); - } - }), - ); } const isEveryRedirectSupported = nextJsRedirects @@ -454,6 +436,8 @@ export async function ɵcodegenPublicDirectory( ), ]); + const appPathRoutesEntries = Object.entries(appPathRoutesManifest); + const middlewareMatcherRegexes = getMiddlewareMatcherRegexes(middlewareManifest); const { redirects = [], rewrites = [], headers = [] } = routesManifest; @@ -485,11 +469,35 @@ export async function ɵcodegenPublicDirectory( appPathRoutesManifest, ); + const pagesManifestLikePrerender: PrerenderManifest["routes"] = Object.fromEntries( + Object.entries(pagesManifest) + .filter(([, srcRoute]) => srcRoute.endsWith(".html")) + .map(([path]) => { + return [ + path, + { + srcRoute: null, + initialRevalidateSeconds: false, + dataRoute: "", + experimentalPPR: false, + prefetchDataRoute: "", + }, + ]; + }), + ); + const routesToCopy: PrerenderManifest["routes"] = { ...prerenderManifest.routes, - ...createPagesManifestLikePrerender(pagesManifest), + ...pagesManifestLikePrerender, }; + const { pprRoutes } = await getAppMetadataFromMetaFiles( + sourceDir, + distDir, + basePath, + appPathRoutesManifest, + ); + await Promise.all( Object.entries(routesToCopy).map(async ([path, route]) => { if (route.initialRevalidateSeconds) { @@ -507,9 +515,9 @@ export async function ɵcodegenPublicDirectory( logger.debug(`skipping ${path} due to server action`); return; } - - const appPathRoute = getAppPathRoute(route, appPathRoutesManifest || {}); - const contentDist = getContentDist(sourceDir, distDir, appPathRoute); + const appPathRoute = + route.srcRoute && appPathRoutesEntries.find(([, it]) => it === route.srcRoute)?.[0]; + const contentDist = join(sourceDir, distDir, "server", appPathRoute ? "app" : "pages"); const sourceParts = path.split("/").filter((it) => !!it); const locale = i18n?.locales.includes(sourceParts[0]) ? sourceParts[0] : undefined; @@ -536,13 +544,12 @@ export async function ɵcodegenPublicDirectory( locale && join(destDir, I18N_ROOT, locale, basePath, ...destPartsOrIndex); let defaultDestPath = isDefaultLocale && join(destDir, basePath, ...destPartsOrIndex); + if (!fileExistsSync(sourcePath) && fileExistsSync(`${sourcePath}.html`)) { sourcePath += ".html"; - // Check if the HTML file is partial - if (await isPartialHTML(sourcePath)) { - logger.debug( - `skipping ${path} as it appears to be a partial HTML file which is not supported by Firebase Hosting`, - ); + + if (pprRoutes.includes(path)) { + logger.debug(`skipping ${path} due to ppr`); return; } diff --git a/src/frameworks/next/testing/app.ts b/src/frameworks/next/testing/app.ts index e4e77b63817..e29d402db88 100644 --- a/src/frameworks/next/testing/app.ts +++ b/src/frameworks/next/testing/app.ts @@ -16,6 +16,7 @@ export const appPathRoutesManifest: AppPathRoutesManifest = { "/server-action/page": "/server-action", "/ssr/page": "/ssr", "/server-action/edge/page": "/server-action/edge", + "/ppr/page": "/ppr", }; export const pagesManifest: PagesManifest = { diff --git a/src/frameworks/next/utils.spec.ts b/src/frameworks/next/utils.spec.ts index b304f821b77..65eea3f100e 100644 --- a/src/frameworks/next/utils.spec.ts +++ b/src/frameworks/next/utils.spec.ts @@ -6,7 +6,6 @@ import * as sinon from "sinon"; import * as glob from "glob"; import * as childProcess from "child_process"; import { FirebaseError } from "../../error"; -import * as path from "path"; import { EXPORT_MARKER, @@ -33,16 +32,12 @@ import { getMiddlewareMatcherRegexes, getNonStaticRoutes, getNonStaticServerComponents, - getHeadersFromMetaFiles, + getAppMetadataFromMetaFiles, isUsingNextImageInAppDirectory, getNextVersion, getRoutesWithServerAction, findEsbuildPath, installEsbuild, - createPagesManifestLikePrerender, - isPartialHTML, - getAppPathRoute, - getContentDist, } from "./utils"; import * as frameworksUtils from "../utils"; @@ -77,9 +72,6 @@ import { serverReferenceManifest, } from "./testing"; import { pathsWithCustomRoutesInternalPrefix } from "./testing/i18n"; -import { AppPathRoutesManifest } from "./interfaces"; -import { PrerenderManifest } from "next/dist/build"; -import { PagesManifest } from "next/dist/build/webpack/plugins/pages-manifest-plugin"; describe("Next.js utils", () => { describe("cleanEscapedChars", () => { @@ -480,38 +472,63 @@ describe("Next.js utils", () => { }); }); - describe("getHeadersFromMetaFiles", () => { + describe("getAppMetadataFromMetaFiles", () => { let sandbox: sinon.SinonSandbox; beforeEach(() => (sandbox = sinon.createSandbox())); afterEach(() => sandbox.restore()); - it("should get headers from meta files", async () => { + it("should return the correct headers and pprRoutes from meta files", async () => { const distDir = ".next"; const readJsonStub = sandbox.stub(frameworksUtils, "readJSON"); const dirExistsSyncStub = sandbox.stub(fsUtils, "dirExistsSync"); const fileExistsSyncStub = sandbox.stub(fsUtils, "fileExistsSync"); + // /api/static dirExistsSyncStub.withArgs(`${distDir}/server/app/api/static`).returns(true); fileExistsSyncStub.withArgs(`${distDir}/server/app/api/static.meta`).returns(true); readJsonStub.withArgs(`${distDir}/server/app/api/static.meta`).resolves(metaFileContents); + // /ppr + dirExistsSyncStub.withArgs(`${distDir}/server/app/ppr`).returns(true); + fileExistsSyncStub.withArgs(`${distDir}/server/app/ppr.meta`).returns(true); + readJsonStub.withArgs(`${distDir}/server/app/ppr.meta`).resolves({ + ...metaFileContents, + postponed: "true", + }); + expect( - await getHeadersFromMetaFiles(".", distDir, "/asdf", appPathRoutesManifest), - ).to.deep.equal([ - { - source: "/asdf/api/static", - headers: [ - { - key: "content-type", - value: "application/json", - }, - { - key: "custom-header", - value: "custom-value", - }, - ], - }, - ]); + await getAppMetadataFromMetaFiles(".", distDir, "/asdf", appPathRoutesManifest), + ).to.deep.equal({ + headers: [ + { + source: "/asdf/api/static", + headers: [ + { + key: "content-type", + value: "application/json", + }, + { + key: "custom-header", + value: "custom-value", + }, + ], + }, + { + source: "/asdf/ppr", + headers: [ + { + key: "content-type", + value: "application/json", + }, + { + key: "custom-header", + value: "custom-value", + }, + ], + }, + ], + pprRoutes: ["/ppr"], + }); }); }); @@ -634,141 +651,4 @@ describe("Next.js utils", () => { } }); }); - - describe("createPagesManifestLikePrerender", () => { - it("should create a PrerenderManifest routes object from PagesManifest", () => { - const pagesManifest: PagesManifest = { - "/": "pages/index.html", - "/about": "pages/about.html", - "/api/data": "pages/api/data.js", - }; - - const result = createPagesManifestLikePrerender(pagesManifest); - - expect(result).to.deep.equal({ - "/": { - srcRoute: null, - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }, - "/about": { - srcRoute: null, - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }, - }); - }); - }); - - describe("isPartialHTML", () => { - let sandbox: sinon.SinonSandbox; - - beforeEach(() => { - sandbox = sinon.createSandbox(); - }); - - afterEach(() => { - sandbox.restore(); - }); - - it("should return true for partial HTML", async () => { - sandbox.stub(fsExtra, "pathExists").resolves(true); - sandbox.stub(fsPromises, "readFile").resolves("Partial"); - expect(await isPartialHTML("partial.html")).to.be.true; - }); - - it("should return false for complete HTML", async () => { - sandbox.stub(fsExtra, "pathExists").resolves(true); - sandbox.stub(fsPromises, "readFile").resolves("Complete"); - expect(await isPartialHTML("complete.html")).to.be.false; - }); - - it("should return false if file does not exist", async () => { - sandbox.stub(fsExtra, "pathExists").resolves(false); - - expect(await isPartialHTML("nonexistent.html")).to.be.false; - }); - - it("should return false and log error if file read fails", async () => { - sandbox.stub(fsExtra, "pathExists").resolves(true); - sandbox.stub(fsPromises, "readFile").rejects(new Error("File read error")); - const consoleErrorStub = sandbox.stub(console, "error"); - - expect(await isPartialHTML("error.html")).to.be.false; - expect(consoleErrorStub.calledOnce).to.be.true; - expect(consoleErrorStub.firstCall.args[0]).to.include("Error processing file"); - }); - }); - - describe("getAppPathRoute", () => { - it("should return the correct app path route", () => { - const route: PrerenderManifest["routes"][string] = { - srcRoute: "/app/page", - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }; - const appPathRoutesManifest: AppPathRoutesManifest = { - "/": "/app/page", - "/about": "/app/about/page", - }; - - expect(getAppPathRoute(route, appPathRoutesManifest)).to.equal("/"); - }); - - it("should return undefined if no matching route is found", () => { - const route: PrerenderManifest["routes"][string] = { - srcRoute: "/app/nonexistent", - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }; - const appPathRoutesManifest: AppPathRoutesManifest = { - "/": "/app/page", - "/about": "/app/about/page", - }; - - expect(getAppPathRoute(route, appPathRoutesManifest)).to.be.undefined; - }); - - it("should return undefined if srcRoute is null", () => { - const route: PrerenderManifest["routes"][string] = { - srcRoute: null, - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }; - const appPathRoutesManifest: AppPathRoutesManifest = { - "/": "/app/page", - }; - - expect(getAppPathRoute(route, appPathRoutesManifest)).to.be.undefined; - }); - }); - - describe("getContentDist", () => { - it("should return the correct path for app directory", () => { - const sourceDir = "/project"; - const distDir = ".next"; - const appPathRoute = "/app/page"; - - const expected = path.join("/project", ".next", "server", "app"); - expect(getContentDist(sourceDir, distDir, appPathRoute)).to.equal(expected); - }); - - it("should return the correct path for pages directory", () => { - const sourceDir = "/project"; - const distDir = ".next"; - - const expected = path.join("/project", ".next", "server", "pages"); - expect(getContentDist(sourceDir, distDir)).to.equal(expected); - }); - }); }); diff --git a/src/frameworks/next/utils.ts b/src/frameworks/next/utils.ts index bea284c424e..50e02d020db 100644 --- a/src/frameworks/next/utils.ts +++ b/src/frameworks/next/utils.ts @@ -37,7 +37,6 @@ import { dirExistsSync, fileExistsSync } from "../../fsutils"; import { IS_WINDOWS } from "../../utils"; import { execSync } from "child_process"; import { FirebaseError } from "../../error"; -import { PrerenderManifest } from "next/dist/build"; export const I18N_SOURCE = /\/:nextInternalLocale(\([^\)]+\))?/; @@ -366,15 +365,16 @@ export function getNonStaticServerComponents( } /** - * Get headers from .meta files + * Get metadata from .meta files */ -export async function getHeadersFromMetaFiles( +export async function getAppMetadataFromMetaFiles( sourceDir: string, distDir: string, basePath: string, appPathRoutesManifest: AppPathRoutesManifest, -): Promise { +): Promise<{ headers: HostingHeadersWithSource[]; pprRoutes: string[] }> { const headers: HostingHeadersWithSource[] = []; + const pprRoutes: string[] = []; await Promise.all( Object.entries(appPathRoutesManifest).map(async ([key, source]) => { @@ -386,17 +386,20 @@ export async function getHeadersFromMetaFiles( const metadataPath = `${routePath}.meta`; if (dirExistsSync(routePath) && fileExistsSync(metadataPath)) { - const meta = await readJSON<{ headers?: Record }>(metadataPath); + const meta = await readJSON<{ headers?: Record; postponed?: string }>( + metadataPath, + ); if (meta.headers) headers.push({ source: posix.join(basePath, source), headers: Object.entries(meta.headers).map(([key, value]) => ({ key, value })), }); + if (meta.postponed) pprRoutes.push(source); } }), ); - return headers; + return { headers, pprRoutes }; } /** @@ -550,64 +553,3 @@ export function installEsbuild(version: string): void { } } } - -/** - * Create a PrerenderManifest["routes"] object from PagesManifest - */ -export function createPagesManifestLikePrerender( - pagesManifestJSON: PagesManifest, -): PrerenderManifest["routes"] { - const pagesManifestLikePrerender: PrerenderManifest["routes"] = {}; - for (const [path, route] of Object.entries(pagesManifestJSON)) { - if (typeof route === "string" && route.endsWith(".html")) { - pagesManifestLikePrerender[path] = { - srcRoute: null, - initialRevalidateSeconds: false, - dataRoute: "", - experimentalPPR: false, - prefetchDataRoute: "", - }; - } - } - return pagesManifestLikePrerender; -} - -/** - * Check if an HTML file is partial (doesn't have closing tag) - */ -export async function isPartialHTML(filePath: string): Promise { - try { - // Check if the file exists before attempting to read it - if (!(await pathExists(filePath))) { - return false; - } - - const content = await readFile(filePath, "utf8"); - return !content.includes(""); - } catch (error) { - console.error(`Error processing file ${filePath}:`, error); - // Assume it's not partial if we can't read the file - return false; - } -} - -/** - * Get the app path route for a given route - */ -export function getAppPathRoute( - route: PrerenderManifest["routes"][string], - appPathRoutesManifest: AppPathRoutesManifest, -): string | undefined { - if (!route.srcRoute) { - return undefined; - } - const entry = Object.entries(appPathRoutesManifest).find(([, it]) => it === route.srcRoute); - return entry ? entry[0] : undefined; -} - -/** - * Get the content distribution directory for a given route - */ -export function getContentDist(sourceDir: string, distDir: string, appPathRoute?: string): string { - return join(sourceDir, distDir, "server", appPathRoute ? "app" : "pages"); -} From 9dd127aecfa13dac480ecc20cb1cb4a86af94358 Mon Sep 17 00:00:00 2001 From: Chalo Salvador Date: Thu, 17 Oct 2024 14:02:29 +0200 Subject: [PATCH 09/11] Changelog --- CHANGELOG.md | 2 +- src/frameworks/next/index.ts | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f8a007b4a9..4b5f3b85e08 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1 @@ -- Added Next.js utils for checking if an HTML file is partial. +- Fixed issue with ppr routes not rendering correctly. diff --git a/src/frameworks/next/index.ts b/src/frameworks/next/index.ts index 9a31ea4a376..42556f5faa6 100644 --- a/src/frameworks/next/index.ts +++ b/src/frameworks/next/index.ts @@ -544,7 +544,6 @@ export async function ɵcodegenPublicDirectory( locale && join(destDir, I18N_ROOT, locale, basePath, ...destPartsOrIndex); let defaultDestPath = isDefaultLocale && join(destDir, basePath, ...destPartsOrIndex); - if (!fileExistsSync(sourcePath) && fileExistsSync(`${sourcePath}.html`)) { sourcePath += ".html"; From ff589a5919d2f1f8c613593b305a4e4c34618d3a Mon Sep 17 00:00:00 2001 From: Leonardo Ortiz Date: Thu, 24 Oct 2024 12:53:31 -0300 Subject: [PATCH 10/11] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4b5f3b85e08..5dc4fd502c4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1 @@ -- Fixed issue with ppr routes not rendering correctly. +- Fixed issue with ppr routes not rendering correctly. (#7625) From 17ca89cead1d22a469d1a2e3d87aaeaae5010a37 Mon Sep 17 00:00:00 2001 From: Leonardo Ortiz Date: Thu, 24 Oct 2024 12:55:11 -0300 Subject: [PATCH 11/11] Update CHANGELOG.md --- CHANGELOG.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5dc4fd502c4..4e7058f86bc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1 +1 @@ -- Fixed issue with ppr routes not rendering correctly. (#7625) +- Fixed Next.js issue with PPR routes not rendering correctly. (#7625)