From 097a8e4e916c7df18eafdaa6c8d6ce2991c17ab6 Mon Sep 17 00:00:00 2001
From: Arsh <69170106+lilnasy@users.noreply.github.com>
Date: Thu, 17 Aug 2023 14:04:31 +0530
Subject: [PATCH] Prerendering corner cases (#8070)

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>
---
 .changeset/many-actors-flash.md               |   5 +
 packages/astro/src/core/app/index.ts          |  40 +++--
 packages/astro/src/core/build/static-build.ts |  11 +-
 packages/astro/test/ssr-prerender.test.js     |   3 +-
 .../package.json                              |   3 +-
 .../src/external-stylesheet.css               |   3 +
 .../src/nondeterminism-404.ts                 |  17 ++
 .../src/nondeterminism-500.ts                 |  17 ++
 .../prerender-404-500/src/pages/404.astro     |   5 +
 .../prerender-404-500/src/pages/500.astro     |   6 +
 .../src/pages/fivehundred.astro               |   4 +
 .../src/pages/static.astro                    |   0
 .../prerender-404/src/pages/404.astro         |   5 -
 packages/integrations/node/test/image.test.js |   3 -
 ...-404.test.js => prerender-404-500.test.js} | 145 +++++++++++++++---
 pnpm-lock.yaml                                |   2 +-
 16 files changed, 221 insertions(+), 48 deletions(-)
 create mode 100644 .changeset/many-actors-flash.md
 rename packages/integrations/node/test/fixtures/{prerender-404 => prerender-404-500}/package.json (67%)
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/external-stylesheet.css
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-404.ts
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-500.ts
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/pages/404.astro
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/pages/500.astro
 create mode 100644 packages/integrations/node/test/fixtures/prerender-404-500/src/pages/fivehundred.astro
 rename packages/integrations/node/test/fixtures/{prerender-404 => prerender-404-500}/src/pages/static.astro (100%)
 delete mode 100644 packages/integrations/node/test/fixtures/prerender-404/src/pages/404.astro
 rename packages/integrations/node/test/{prerender-404.test.js => prerender-404-500.test.js} (50%)

diff --git a/.changeset/many-actors-flash.md b/.changeset/many-actors-flash.md
new file mode 100644
index 000000000000..92f70c55a024
--- /dev/null
+++ b/.changeset/many-actors-flash.md
@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fix a handful of edge cases with prerendered 404/500 pages
diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts
index bec5368b6ce2..900efc682293 100644
--- a/packages/astro/src/core/app/index.ts
+++ b/packages/astro/src/core/app/index.ts
@@ -286,10 +286,16 @@ export class App {
 		const errorRouteData = matchRoute('/' + status, this.#manifestData);
 		const url = new URL(request.url);
 		if (errorRouteData) {
-			if (errorRouteData.prerender && !errorRouteData.route.endsWith(`/${status}`)) {
-				const statusURL = new URL(`${this.#baseWithoutTrailingSlash}/${status}`, url);
+			if (errorRouteData.prerender){
+				const maybeDotHtml = errorRouteData.route.endsWith(`/${status}`) ? '.html' : ''
+				const statusURL = new URL(`${this.#baseWithoutTrailingSlash}/${status}${maybeDotHtml}`, url);
 				const response = await fetch(statusURL.toString());
-				return this.#mergeResponses(response, originalResponse);
+
+				// response for /404.html and 500.html is 200, which is not meaningful
+				// so we create an override
+				const override = { status }
+
+				return this.#mergeResponses(response, originalResponse, override);
 			}
 			const mod = await this.#getModuleForRoute(errorRouteData);
 			try {
@@ -316,14 +322,30 @@ export class App {
 		return response;
 	}
 
-	#mergeResponses(newResponse: Response, oldResponse?: Response) {
-		if (!oldResponse) return newResponse;
-		const { status, statusText, headers } = oldResponse;
+	#mergeResponses(newResponse: Response, oldResponse?: Response, override?: { status : 404 | 500 }) {
+		if (!oldResponse) {
+			if (override !== undefined) {
+				return new Response(newResponse.body, {
+					status: override.status,
+					statusText: newResponse.statusText,
+					headers: newResponse.headers
+				})
+			}
+			return newResponse;
+		}
+		
+		const { statusText, headers } = oldResponse;
+
+		// If the the new response did not have a meaningful status, an override may have been provided
+		// If the original status was 200 (default), override it with the new status (probably 404 or 500)
+		// Otherwise, the user set a specific status while rendering and we should respect that one
+		const status = 
+				override?.status ? override.status :
+				oldResponse.status === 200 ? newResponse.status :
+				oldResponse.status
 
 		return new Response(newResponse.body, {
-			// If the original status was 200 (default), override it with the new status (probably 404 or 500)
-			// Otherwise, the user set a specific status while rendering and we should respect that one
-			status: status === 200 ? newResponse.status : status,
+			status,
 			statusText: status === 200 ? newResponse.statusText : statusText,
 			headers: new Headers(Array.from(headers)),
 		});
diff --git a/packages/astro/src/core/build/static-build.ts b/packages/astro/src/core/build/static-build.ts
index 896d771ac55f..ea5bcf7ddee9 100644
--- a/packages/astro/src/core/build/static-build.ts
+++ b/packages/astro/src/core/build/static-build.ts
@@ -308,8 +308,12 @@ async function runPostBuildHooks(
 async function cleanStaticOutput(opts: StaticBuildOptions, internals: BuildInternals) {
 	const allStaticFiles = new Set();
 	for (const pageData of eachPageData(internals)) {
-		if (pageData.route.prerender)
-			allStaticFiles.add(internals.pageToBundleMap.get(pageData.moduleSpecifier));
+		if (pageData.route.prerender) {
+			const { moduleSpecifier } = pageData;
+			const pageBundleId = internals.pageToBundleMap.get(moduleSpecifier);
+			const entryBundleId = internals.entrySpecifierToBundleMap.get(moduleSpecifier);
+			allStaticFiles.add(pageBundleId ?? entryBundleId);
+		}
 	}
 	const ssr = isServerLikeOutput(opts.settings.config);
 	const out = ssr
@@ -337,7 +341,8 @@ async function cleanStaticOutput(opts: StaticBuildOptions, internals: BuildInter
 				// Replace exports (only prerendered pages) with a noop
 				let value = 'const noop = () => {};';
 				for (const e of exports) {
-					value += `\nexport const ${e.n} = noop;`;
+					if (e.n === 'default') value += `\n export default noop;`
+					else value += `\nexport const ${e.n} = noop;`;
 				}
 				await fs.promises.writeFile(url, value, { encoding: 'utf8' });
 			})
diff --git a/packages/astro/test/ssr-prerender.test.js b/packages/astro/test/ssr-prerender.test.js
index 90ec1b6faa54..567371f0bbaa 100644
--- a/packages/astro/test/ssr-prerender.test.js
+++ b/packages/astro/test/ssr-prerender.test.js
@@ -30,8 +30,7 @@ describe('SSR: prerender', () => {
 			const app = await fixture.loadTestAdapterApp();
 			/** @type {Set<string>} */
 			const assets = app.manifest.assets;
-			expect(assets.size).to.equal(1);
-			expect(Array.from(assets)[0].endsWith('static/index.html')).to.be.true;
+			expect(assets).to.contain('/static/index.html');
 		});
 	});
 
diff --git a/packages/integrations/node/test/fixtures/prerender-404/package.json b/packages/integrations/node/test/fixtures/prerender-404-500/package.json
similarity index 67%
rename from packages/integrations/node/test/fixtures/prerender-404/package.json
rename to packages/integrations/node/test/fixtures/prerender-404-500/package.json
index dfd109c9142f..f962fe991e3f 100644
--- a/packages/integrations/node/test/fixtures/prerender-404/package.json
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/package.json
@@ -1,7 +1,8 @@
 {
-  "name": "@test/nodejs-prerender-404",
+  "name": "@test/nodejs-prerender-404-500",
   "version": "0.0.0",
   "private": true,
+  "type": "module",
   "dependencies": {
     "astro": "workspace:*",
     "@astrojs/node": "workspace:*"
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/external-stylesheet.css b/packages/integrations/node/test/fixtures/prerender-404-500/src/external-stylesheet.css
new file mode 100644
index 000000000000..5f331948a60d
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/external-stylesheet.css
@@ -0,0 +1,3 @@
+body {
+    background-color: ivory;
+}
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-404.ts b/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-404.ts
new file mode 100644
index 000000000000..1795c26b07af
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-404.ts
@@ -0,0 +1,17 @@
+// This module is only used by the prerendered 404.astro.
+// It exhibits different behavior if it's called more than once,
+// which is detected by a test and interpreted as a failure.
+
+let usedOnce = false
+let dynamicMessage = "Page was not prerendered"
+
+export default function () {
+    if (usedOnce === false) {
+        usedOnce = true
+        return "Page does not exist"
+    }
+    
+    dynamicMessage += "+"
+    
+    return dynamicMessage
+}
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-500.ts b/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-500.ts
new file mode 100644
index 000000000000..8f8024a6083b
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/nondeterminism-500.ts
@@ -0,0 +1,17 @@
+// This module is only used by the prerendered 500.astro.
+// It exhibits different behavior if it's called more than once,
+// which is detected by a test and interpreted as a failure.
+
+let usedOnce = false
+let dynamicMessage = "Page was not prerendered"
+
+export default function () {
+    if (usedOnce === false) {
+        usedOnce = true
+        return "Something went wrong"
+    }
+    
+    dynamicMessage += "+"
+    
+    return dynamicMessage
+}
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/404.astro b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/404.astro
new file mode 100644
index 000000000000..37fd1c1d39c4
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/404.astro
@@ -0,0 +1,5 @@
+---
+import message from "../nondeterminism-404"
+export const prerender = true;
+---
+{message()}
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/500.astro b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/500.astro
new file mode 100644
index 000000000000..ef91ad0ffa91
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/500.astro
@@ -0,0 +1,6 @@
+---
+import "../external-stylesheet.css"
+import message from "../nondeterminism-500"
+export const prerender = true
+---
+<h1>{message()}</h1>
diff --git a/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/fivehundred.astro b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/fivehundred.astro
new file mode 100644
index 000000000000..99d103567db2
--- /dev/null
+++ b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/fivehundred.astro
@@ -0,0 +1,4 @@
+---
+return new Response(null, { status: 500 })
+---
+<p>This html will not be served</p>
diff --git a/packages/integrations/node/test/fixtures/prerender-404/src/pages/static.astro b/packages/integrations/node/test/fixtures/prerender-404-500/src/pages/static.astro
similarity index 100%
rename from packages/integrations/node/test/fixtures/prerender-404/src/pages/static.astro
rename to packages/integrations/node/test/fixtures/prerender-404-500/src/pages/static.astro
diff --git a/packages/integrations/node/test/fixtures/prerender-404/src/pages/404.astro b/packages/integrations/node/test/fixtures/prerender-404/src/pages/404.astro
deleted file mode 100644
index 230402bbccdb..000000000000
--- a/packages/integrations/node/test/fixtures/prerender-404/src/pages/404.astro
+++ /dev/null
@@ -1,5 +0,0 @@
----
-export const prerender = true;
----
-
-Page does not exist
diff --git a/packages/integrations/node/test/image.test.js b/packages/integrations/node/test/image.test.js
index 0834bc175fa4..f3d9ea71f8f4 100644
--- a/packages/integrations/node/test/image.test.js
+++ b/packages/integrations/node/test/image.test.js
@@ -32,9 +32,6 @@ describe('Image endpoint', () => {
 			'/_image?href=/_astro/some_penguin.97ef5f92.png&w=50&f=webp'
 		);
 
-		console.log(resImage);
-		const content = resImage.text();
-		console.log(content);
 		expect(resImage.status).to.equal(200);
 	});
 });
diff --git a/packages/integrations/node/test/prerender-404.test.js b/packages/integrations/node/test/prerender-404-500.test.js
similarity index 50%
rename from packages/integrations/node/test/prerender-404.test.js
rename to packages/integrations/node/test/prerender-404-500.test.js
index 626f584e0c17..33cebab5ba4a 100644
--- a/packages/integrations/node/test/prerender-404.test.js
+++ b/packages/integrations/node/test/prerender-404-500.test.js
@@ -10,10 +10,11 @@ import { fetch } from 'undici';
 
 async function load() {
 	const mod = await import(
-		`./fixtures/prerender-404/dist/server/entry.mjs?dropcache=${Date.now()}`
+		`./fixtures/prerender-404-500/dist/server/entry.mjs?dropcache=${Date.now()}`
 	);
 	return mod;
 }
+
 describe('Prerender 404', () => {
 	/** @type {import('./test-utils').Fixture} */
 	let fixture;
@@ -25,8 +26,12 @@ describe('Prerender 404', () => {
 			process.env.PRERENDER = true;
 
 			fixture = await loadFixture({
+				// inconsequential config that differs between tests
+				// to bust cache and prevent modules and their state
+				// from being reused
+				site: 'https://test.dev/',
 				base: '/some-base',
-				root: './fixtures/prerender-404/',
+				root: './fixtures/prerender-404-500/',
 				output: 'server',
 				adapter: nodejs({ mode: 'standalone' }),
 			});
@@ -52,13 +57,54 @@ describe('Prerender 404', () => {
 		});
 
 		it('Can handle prerendered 404', async () => {
-			const res = await fetch(`http://${server.host}:${server.port}/some-base/missing`);
-			const html = await res.text();
-			const $ = cheerio.load(html);
+			const url = `http://${server.host}:${server.port}/some-base/missing`;
+			const res1 = await fetch(url);
+			const res2 = await fetch(url);
+			const res3 = await fetch(url);
+			
+			expect(res1.status).to.equal(404);
+			expect(res2.status).to.equal(404);
+			expect(res3.status).to.equal(404);
+			
+			const html1 = await res1.text();
+			const html2 = await res2.text();
+			const html3 = await res3.text();
+
+			expect(html1).to.equal(html2);
+			expect(html2).to.equal(html3);
+			
+			const $ = cheerio.load(html1);
 
-			expect(res.status).to.equal(404);
 			expect($('body').text()).to.equal('Page does not exist');
 		});
+
+		it(' Can handle prerendered 500 called indirectly', async () => {
+			const url = `http://${server.host}:${server.port}/some-base/fivehundred`;
+			const response1 = await fetch(url);
+			const response2 = await fetch(url);
+			const response3 = await fetch(url);
+
+			expect(response1.status).to.equal(500);
+
+			const html1 = await response1.text();
+			const html2 = await response2.text();
+			const html3 = await response3.text();
+
+			expect(html1).to.contain("Something went wrong");
+
+			expect(html1).to.equal(html2);
+			expect(html2).to.equal(html3);
+		});
+
+		it('prerendered 500 page includes expected styles', async () => {
+			const response = await fetch(`http://${server.host}:${server.port}/some-base/fivehundred`);
+			const html = await response.text();
+			const $ = cheerio.load(html);
+			
+			// length will be 0 if the stylesheet does not get included
+			expect($('link[rel=stylesheet]')).to.have.a.lengthOf(1);
+		});
+		
 	});
 
 	describe('Without base', async () => {
@@ -67,12 +113,16 @@ describe('Prerender 404', () => {
 			process.env.PRERENDER = true;
 
 			fixture = await loadFixture({
-				root: './fixtures/prerender-404/',
+				// inconsequential config that differs between tests
+				// to bust cache and prevent modules and their state
+				// from being reused
+				site: 'https://test.info/',
+				root: './fixtures/prerender-404-500/',
 				output: 'server',
 				adapter: nodejs({ mode: 'standalone' }),
 			});
 			await fixture.build();
-			const { startServer } = await await load();
+			const { startServer } = await load();
 			let res = startServer();
 			server = res.server;
 		});
@@ -93,11 +143,24 @@ describe('Prerender 404', () => {
 		});
 
 		it('Can handle prerendered 404', async () => {
-			const res = await fetch(`http://${server.host}:${server.port}/missing`);
-			const html = await res.text();
-			const $ = cheerio.load(html);
+			const url = `http://${server.host}:${server.port}/some-base/missing`
+			const res1 = await fetch(url);
+			const res2 = await fetch(url);
+			const res3 = await fetch(url);
+			
+			expect(res1.status).to.equal(404);
+			expect(res2.status).to.equal(404);
+			expect(res3.status).to.equal(404);
+			
+			const html1 = await res1.text();
+			const html2 = await res2.text();
+			const html3 = await res3.text();
+
+			expect(html1).to.equal(html2);
+			expect(html2).to.equal(html3);
+			
+			const $ = cheerio.load(html1);
 
-			expect(res.status).to.equal(404);
 			expect($('body').text()).to.equal('Page does not exist');
 		});
 	});
@@ -113,13 +176,17 @@ describe('Hybrid 404', () => {
 			process.env.ASTRO_NODE_AUTOSTART = 'disabled';
 			process.env.PRERENDER = false;
 			fixture = await loadFixture({
+				// inconsequential config that differs between tests
+				// to bust cache and prevent modules and their state
+				// from being reused
+				site: 'https://test.com/',
 				base: '/some-base',
-				root: './fixtures/prerender-404/',
+				root: './fixtures/prerender-404-500/',
 				output: 'hybrid',
 				adapter: nodejs({ mode: 'standalone' }),
 			});
 			await fixture.build();
-			const { startServer } = await await load();
+			const { startServer } = await load();
 			let res = startServer();
 			server = res.server;
 		});
@@ -140,11 +207,24 @@ describe('Hybrid 404', () => {
 		});
 
 		it('Can handle prerendered 404', async () => {
-			const res = await fetch(`http://${server.host}:${server.port}/some-base/missing`);
-			const html = await res.text();
-			const $ = cheerio.load(html);
+			const url = `http://${server.host}:${server.port}/some-base/missing`
+			const res1 = await fetch(url);
+			const res2 = await fetch(url);
+			const res3 = await fetch(url);
+			
+			expect(res1.status).to.equal(404);
+			expect(res2.status).to.equal(404);
+			expect(res3.status).to.equal(404);
+			
+			const html1 = await res1.text();
+			const html2 = await res2.text();
+			const html3 = await res3.text();
+
+			expect(html1).to.equal(html2);
+			expect(html2).to.equal(html3);
+			
+			const $ = cheerio.load(html1);
 
-			expect(res.status).to.equal(404);
 			expect($('body').text()).to.equal('Page does not exist');
 		});
 	});
@@ -154,12 +234,16 @@ describe('Hybrid 404', () => {
 			process.env.ASTRO_NODE_AUTOSTART = 'disabled';
 			process.env.PRERENDER = false;
 			fixture = await loadFixture({
-				root: './fixtures/prerender-404/',
+				// inconsequential config that differs between tests
+				// to bust cache and prevent modules and their state
+				// from being reused
+				site: 'https://test.net/',
+				root: './fixtures/prerender-404-500/',
 				output: 'hybrid',
 				adapter: nodejs({ mode: 'standalone' }),
 			});
 			await fixture.build();
-			const { startServer } = await await load();
+			const { startServer } = await load();
 			let res = startServer();
 			server = res.server;
 		});
@@ -180,11 +264,24 @@ describe('Hybrid 404', () => {
 		});
 
 		it('Can handle prerendered 404', async () => {
-			const res = await fetch(`http://${server.host}:${server.port}/missing`);
-			const html = await res.text();
-			const $ = cheerio.load(html);
+			const url = `http://${server.host}:${server.port}/missing`
+			const res1 = await fetch(url);
+			const res2 = await fetch(url);
+			const res3 = await fetch(url);
+			
+			expect(res1.status).to.equal(404);
+			expect(res2.status).to.equal(404);
+			expect(res3.status).to.equal(404);
+			
+			const html1 = await res1.text();
+			const html2 = await res2.text();
+			const html3 = await res3.text();
+
+			expect(html1).to.equal(html2);
+			expect(html2).to.equal(html3);
+			
+			const $ = cheerio.load(html1);
 
-			expect(res.status).to.equal(404);
 			expect($('body').text()).to.equal('Page does not exist');
 		});
 	});
diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml
index b8acb41e4d3a..5f0529643a73 100644
--- a/pnpm-lock.yaml
+++ b/pnpm-lock.yaml
@@ -4641,7 +4641,7 @@ importers:
         specifier: workspace:*
         version: link:../../../../../astro
 
-  packages/integrations/node/test/fixtures/prerender-404:
+  packages/integrations/node/test/fixtures/prerender-404-500:
     dependencies:
       '@astrojs/node':
         specifier: workspace:*