From 1943cae91de7295de336cf503164a00735129ea9 Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Wed, 10 Apr 2024 00:22:57 +0000 Subject: [PATCH 1/3] fix(dev): infinite implicit rerouting --- .../src/vite-plugin-astro-server/route.ts | 2 +- .../custom-404-implicit-rerouting.test.js | 52 ++++--------------- .../src/pages/index.astro | 1 + 3 files changed, 12 insertions(+), 43 deletions(-) create mode 100644 packages/astro/test/fixtures/custom-404-loop-case-5/src/pages/index.astro 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..221d917052dd 100644 --- a/packages/astro/test/custom-404-implicit-rerouting.test.js +++ b/packages/astro/test/custom-404-implicit-rerouting.test.js @@ -2,7 +2,7 @@ import assert from 'node:assert/strict'; import { after, before, describe, it } from 'node:test'; import { loadFixture } from './test-utils.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> */ let fixture; @@ -19,51 +19,19 @@ for (const caseNumber of [1, 2, 3, 4]) { }); // 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); - } - ); + it('dev server handles normal requests', async () => { + const response = await fixture.fetch('/', { signal: AbortSignal.timeout(1000) }); + assert.equal(response.status, 200); + }); - 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); - } - ); + // IMPORTANT: never skip + it('dev server stays responsive', async () => { + const response = await fixture.fetch('/alvsibdlvjks', { signal: AbortSignal.timeout(1000) }); + assert.equal(response.status, 404); + }); after(async () => { await devServer.stop(); }); }); } - -/***** UTILITY FUNCTIONS *****/ - -const timeout = Symbol('timeout'); - -/** @template Res */ -function withTimeout( - /** @type Promise */ - responsePromise, - /** @type number */ - timeLimit -) { - /** @type Promise */ - const timeoutPromise = new Promise((resolve) => setTimeout(() => resolve(timeout), timeLimit)); - - return Promise.race([responsePromise, timeoutPromise]); -} 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 @@ +

all good here... or is it?

\ No newline at end of file From 8fe50fbdf88181b5965e24b37efdf54f2c14441d Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Wed, 10 Apr 2024 00:34:42 +0000 Subject: [PATCH 2/3] test adapter --- .../custom-404-implicit-rerouting.test.js | 62 ++++++++++++++----- 1 file changed, 46 insertions(+), 16 deletions(-) diff --git a/packages/astro/test/custom-404-implicit-rerouting.test.js b/packages/astro/test/custom-404-implicit-rerouting.test.js index 221d917052dd..5dcda1329d89 100644 --- a/packages/astro/test/custom-404-implicit-rerouting.test.js +++ b/packages/astro/test/custom-404-implicit-rerouting.test.js @@ -1,37 +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, 5]) { describe(`Custom 404 with implicit rerouting - Case #${caseNumber}`, () => { - /** @type Awaited> */ + /** @type {import('./test-utils.js').Fixture} */ let fixture; - /** @type Awaited> */ - 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('dev server handles normal requests', async () => { - const response = await fixture.fetch('/', { signal: AbortSignal.timeout(1000) }); - assert.equal(response.status, 200); - }); + describe("dev server", () => { + /** @type {import('./test-utils.js').DevServer} */ + let devServer; + + before(async () => { + await fixture.build() + devServer = await fixture.startDevServer(); + }); - // IMPORTANT: never skip - it('dev server stays responsive', async () => { - const response = await fixture.fetch('/alvsibdlvjks', { signal: AbortSignal.timeout(1000) }); - assert.equal(response.status, 404); + // 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; + + before(async () => { + app = await fixture.loadTestAdapterApp(); + }); - after(async () => { - await devServer.stop(); + // 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); + }); }); }); } From a410188fb2f97648c22985da970863e1fe9816e4 Mon Sep 17 00:00:00 2001 From: lilnasy <69170106+lilnasy@users.noreply.github.com> Date: Wed, 10 Apr 2024 00:54:59 +0000 Subject: [PATCH 3/3] changeset --- .changeset/sharp-elephants-clap.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/sharp-elephants-clap.md 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.