Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(dev): break implicit rerouting loop #10737

Merged
merged 3 commits into from
Apr 10, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/astro/src/vite-plugin-astro-server/route.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
96 changes: 47 additions & 49 deletions packages/astro/test/custom-404-implicit-rerouting.test.js
Original file line number Diff line number Diff line change
@@ -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);
});
});
});
}
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
<p>all good here... or is it?</p>
Loading