diff --git a/.changeset/curvy-trees-compare.md b/.changeset/curvy-trees-compare.md new file mode 100644 index 0000000000..0580ab6d70 --- /dev/null +++ b/.changeset/curvy-trees-compare.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Fixes an issue where `404.astro` was ignored with `i18n` routing enabled. diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 2c334223fd..7c8d0067af 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -320,6 +320,7 @@ export class App { }); } + // We remove internally-used header before we send the response to the user agent. if (response.headers.has(REROUTE_DIRECTIVE_HEADER)) { response.headers.delete(REROUTE_DIRECTIVE_HEADER); } diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 50aa7b710b..1bf5d652a8 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -13,6 +13,7 @@ import { renderEndpoint } from '../runtime/server/endpoint.js'; import { renderPage } from '../runtime/server/index.js'; import { ASTRO_VERSION, + REROUTE_DIRECTIVE_HEADER, ROUTE_TYPE_HEADER, clientAddressSymbol, clientLocalsSymbol, @@ -102,7 +103,12 @@ export class RenderContext { streaming, routeData ); + // Signal to the i18n middleware to maybe act on this response response.headers.set(ROUTE_TYPE_HEADER, 'page'); + // Signal to the error-page-rerouting infra to let this response pass through to avoid loops + if (routeData.route === "/404" || routeData.route === "/500") { + response.headers.set(REROUTE_DIRECTIVE_HEADER, "no") + } return response; } : type === 'fallback' diff --git a/packages/astro/src/i18n/middleware.ts b/packages/astro/src/i18n/middleware.ts index 2098ffc7a8..4affbf9318 100644 --- a/packages/astro/src/i18n/middleware.ts +++ b/packages/astro/src/i18n/middleware.ts @@ -2,7 +2,7 @@ import { appendForwardSlash, joinPaths } from '@astrojs/internal-helpers/path'; import type { APIContext, Locales, MiddlewareHandler, SSRManifest } from '../@types/astro.js'; import type { SSRManifestI18n } from '../core/app/types.js'; import { shouldAppendForwardSlash } from '../core/build/util.js'; -import { ROUTE_TYPE_HEADER } from '../core/constants.js'; +import { REROUTE_DIRECTIVE_HEADER, ROUTE_TYPE_HEADER } from '../core/constants.js'; import { getPathByLocale, normalizeTheLocale } from './index.js'; // Checks if the pathname has any locale, exception for the defaultLocale, which is ignored on purpose. @@ -44,12 +44,9 @@ export function createI18nMiddleware( } } - // Astro can't know where the default locale is supposed to be, so it returns a 404 with no content. + // Astro can't know where the default locale is supposed to be, so it returns a 404. else if (!pathnameHasLocale(url.pathname, i18n.locales)) { - return new Response(null, { - status: 404, - headers: response.headers, - }); + return notFound(response); } return undefined; @@ -66,10 +63,7 @@ export function createI18nMiddleware( if (pathnameContainsDefaultLocale) { const newLocation = url.pathname.replace(`/${i18n.defaultLocale}`, ''); response.headers.set('Location', newLocation); - return new Response(null, { - status: 404, - headers: response.headers, - }); + return notFound(response); } return undefined; @@ -88,10 +82,7 @@ export function createI18nMiddleware( // - the URL doesn't contain a locale const isRoot = url.pathname === base + '/' || url.pathname === base; if (!(isRoot || pathnameHasLocale(url.pathname, i18n.locales))) { - return new Response(null, { - status: 404, - headers: response.headers, - }); + return notFound(response); } return undefined; @@ -201,6 +192,19 @@ export function createI18nMiddleware( }; } +/** + * The i18n returns empty 404 responses in certain cases. + * Error-page-rerouting infra will attempt to render the 404.astro page, causing the middleware to run a second time. + * To avoid loops and overwriting the contents of `404.astro`, we allow error pages to pass through. + */ +function notFound(response: Response) { + if (response.headers.get(REROUTE_DIRECTIVE_HEADER) === "no") return response; + return new Response(null, { + status: 404, + headers: response.headers, + }); +} + /** * Checks if the current locale doesn't belong to a configured domain * @param i18n diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 19af22ff6a..1c533dbc8e 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -274,7 +274,7 @@ export async function handleRoute({ response.headers.get(REROUTE_DIRECTIVE_HEADER) !== 'no' ) { const fourOhFourRoute = await matchRoute('/404', manifestData, pipeline); - if (options && fourOhFourRoute?.route !== options.route) + if (options) return handleRoute({ ...options, matchedRoute: fourOhFourRoute, @@ -288,6 +288,12 @@ export async function handleRoute({ incomingResponse, }); } + + // We remove the internally-used header before we send the response to the user agent. + if (response.headers.has(REROUTE_DIRECTIVE_HEADER)) { + response.headers.delete(REROUTE_DIRECTIVE_HEADER); + } + if (route.type === 'endpoint') { await writeWebResponse(incomingResponse, response); return; diff --git a/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro new file mode 100644 index 0000000000..045415d447 --- /dev/null +++ b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro @@ -0,0 +1,4 @@ + + Not Found + Can't find the page youre looking for. + diff --git a/packages/astro/test/i18n-routing.test.js b/packages/astro/test/i18n-routing.test.js index 8be9847d35..23255ef9bc 100644 --- a/packages/astro/test/i18n-routing.test.js +++ b/packages/astro/test/i18n-routing.test.js @@ -353,6 +353,13 @@ describe('[DEV] i18n routing', () => { assert.equal(response.status, 200); assert.equal((await response.text()).includes('I am index'), true); }); + + it('can render the 404.astro route on unmatched requests', async () => { + const response = await fixture.fetch('/xyz'); + assert.equal(response.status, 404); + const text = await response.text(); + assert.equal(text.includes("Can't find the page youre looking for."), true); + }); }); describe('i18n routing with routing strategy [pathname-prefix-always]', () => { @@ -1338,6 +1345,15 @@ describe('[SSR] i18n routing', () => { assert.equal(response.status, 200); assert.equal((await response.text()).includes('I am index'), true); }); + + + it('can render the 404.astro route on unmatched requests', async () => { + const request = new Request('http://example.com/xyz'); + const response = await app.render(request); + assert.equal(response.status, 404); + const text = await response.text(); + assert.equal(text.includes("Can't find the page youre looking for."), true); + }); }); describe('i18n routing with routing strategy [pathname-prefix-always]', () => { diff --git a/packages/astro/test/units/routing/endpoints.test.js b/packages/astro/test/units/routing/endpoints.test.js index cff44f1b45..43d76af5a5 100644 --- a/packages/astro/test/units/routing/endpoints.test.js +++ b/packages/astro/test/units/routing/endpoints.test.js @@ -62,11 +62,10 @@ describe('endpoints', () => { await done; const headers = res.getHeaders(); assert.equal(headers['location'], 'https://example.com/destination'); - assert.equal(headers['x-astro-reroute'], undefined); assert.equal(res.statusCode, 307); }); - it('should append reroute header for HTTP status 404', async () => { + it('should remove internally-used for HTTP status 404', async () => { const { req, res, done } = createRequestAndResponse({ method: 'GET', url: '/not-found', @@ -74,11 +73,11 @@ describe('endpoints', () => { container.handle(req, res); await done; const headers = res.getHeaders(); - assert.equal(headers['x-astro-reroute'], 'no'); + assert.equal(headers['x-astro-reroute'], undefined); assert.equal(res.statusCode, 404); }); - it('should append reroute header for HTTP status 500', async () => { + it('should remove internally-used header for HTTP status 500', async () => { const { req, res, done } = createRequestAndResponse({ method: 'GET', url: '/internal-error', @@ -86,7 +85,7 @@ describe('endpoints', () => { container.handle(req, res); await done; const headers = res.getHeaders(); - assert.equal(headers['x-astro-reroute'], 'no'); + assert.equal(headers['x-astro-reroute'], undefined); assert.equal(res.statusCode, 500); }); });