From 8a30f257b1f3618b01212a591b82ad7a63c82fbb Mon Sep 17 00:00:00 2001
From: Arsh <69170106+lilnasy@users.noreply.github.com>
Date: Wed, 10 Apr 2024 06:25:33 +0530
Subject: [PATCH] fix(dev): break implicit rerouting loop (#10737)
* fix(dev): infinite implicit rerouting
* test adapter
* changeset
---
.changeset/sharp-elephants-clap.md | 5 +
.../src/vite-plugin-astro-server/route.ts | 2 +-
.../custom-404-implicit-rerouting.test.js | 96 +++++++++----------
.../src/pages/index.astro | 1 +
4 files changed, 54 insertions(+), 50 deletions(-)
create mode 100644 .changeset/sharp-elephants-clap.md
create mode 100644 packages/astro/test/fixtures/custom-404-loop-case-5/src/pages/index.astro
diff --git a/.changeset/sharp-elephants-clap.md b/.changeset/sharp-elephants-clap.md
new file mode 100644
index 000000000000..b1bf39bfa308
--- /dev/null
+++ b/.changeset/sharp-elephants-clap.md
@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fixes a regression where constructing and returning 404 responses from a middleware resulted in the dev server getting stuck in a loop.
diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts
index c793151a445a..3228641489fb 100644
--- a/packages/astro/src/vite-plugin-astro-server/route.ts
+++ b/packages/astro/src/vite-plugin-astro-server/route.ts
@@ -287,7 +287,7 @@ export async function handleRoute({
}
if (response.status === 404 && response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no') {
const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline);
- if (options)
+ if (options && options.route !== fourOhFourRoute?.route)
return handleRoute({
...options,
matchedRoute: fourOhFourRoute,
diff --git a/packages/astro/test/custom-404-implicit-rerouting.test.js b/packages/astro/test/custom-404-implicit-rerouting.test.js
index 4986b34c42ba..5dcda1329d89 100644
--- a/packages/astro/test/custom-404-implicit-rerouting.test.js
+++ b/packages/astro/test/custom-404-implicit-rerouting.test.js
@@ -1,69 +1,67 @@
import assert from 'node:assert/strict';
import { after, before, describe, it } from 'node:test';
import { loadFixture } from './test-utils.js';
+import testAdapter from './test-adapter.js';
-for (const caseNumber of [1, 2, 3, 4]) {
+for (const caseNumber of [1, 2, 3, 4, 5]) {
describe(`Custom 404 with implicit rerouting - Case #${caseNumber}`, () => {
- /** @type Awaited<ReturnType<typeof loadFixture>> */
+ /** @type {import('./test-utils.js').Fixture} */
let fixture;
- /** @type Awaited<ReturnType<typeof fixture['startDevServer']>> */
- let devServer;
before(async () => {
fixture = await loadFixture({
+ output: 'server',
root: `./fixtures/custom-404-loop-case-${caseNumber}/`,
site: 'http://example.com',
+ adapter: testAdapter()
});
-
- devServer = await fixture.startDevServer();
});
- // sanity check
- it.skip(
- 'dev server handles normal requests',
- {
- todo: 'To re-enabled after we understand why this fails when the test suite is run in parallel',
- },
- async () => {
- const resPromise = fixture.fetch('/');
- const result = await withTimeout(resPromise, 1000);
- assert.notEqual(result, timeout);
- assert.equal(result.status, 200);
- }
- );
+ describe("dev server", () => {
+ /** @type {import('./test-utils.js').DevServer} */
+ let devServer;
- it.skip(
- 'dev server stays responsive',
- {
- todo: 'To re-enabled after we understand why this fails when the test suite is run in parallel',
- },
- async () => {
- const resPromise = fixture.fetch('/alvsibdlvjks');
- const result = await withTimeout(resPromise, 1000);
- assert.notEqual(result, timeout);
- assert.equal(result.status, 404);
- }
- );
+ before(async () => {
+ await fixture.build()
+ devServer = await fixture.startDevServer();
+ });
- after(async () => {
- await devServer.stop();
+ // sanity check
+ it('dev server handles normal requests', { timeout: 1000 }, async () => {
+ const response = await fixture.fetch('/');
+ assert.equal(response.status, 200);
+ });
+
+ // IMPORTANT: never skip
+ it('dev server stays responsive', { timeout: 1000 }, async () => {
+ const response = await fixture.fetch('/alvsibdlvjks');
+ assert.equal(response.status, 404);
+ });
+
+ after(async () => {
+ await devServer.stop();
+ });
});
- });
-}
+
+ describe("prod server", () => {
+ /** @type {import('./test-utils.js').App} */
+ let app;
-/***** UTILITY FUNCTIONS *****/
-
-const timeout = Symbol('timeout');
-
-/** @template Res */
-function withTimeout(
- /** @type Promise<Res> */
- responsePromise,
- /** @type number */
- timeLimit
-) {
- /** @type Promise<typeof timeout> */
- const timeoutPromise = new Promise((resolve) => setTimeout(() => resolve(timeout), timeLimit));
+ before(async () => {
+ app = await fixture.loadTestAdapterApp();
+ });
- return Promise.race([responsePromise, timeoutPromise]);
+ // sanity check
+ it('prod server handles normal requests', { timeout: 1000 }, async () => {
+ const response = await app.render(new Request('https://example.com/'));
+ assert.equal(response.status, 200);
+ });
+
+ // IMPORTANT: never skip
+ it('prod server stays responsive', { timeout: 1000 }, async () => {
+ const response = await app.render(new Request('https://example.com/alvsibdlvjks'));
+ assert.equal(response.status, 404);
+ });
+ });
+ });
}
diff --git a/packages/astro/test/fixtures/custom-404-loop-case-5/src/pages/index.astro b/packages/astro/test/fixtures/custom-404-loop-case-5/src/pages/index.astro
new file mode 100644
index 000000000000..a65c81fdb2f1
--- /dev/null
+++ b/packages/astro/test/fixtures/custom-404-loop-case-5/src/pages/index.astro
@@ -0,0 +1 @@
+<p>all good here... or is it?</p>
\ No newline at end of file