diff options
-rw-r--r-- | .changeset/curvy-trees-compare.md | 5 | ||||
-rw-r--r-- | packages/astro/src/core/app/index.ts | 1 | ||||
-rw-r--r-- | packages/astro/src/core/render-context.ts | 6 | ||||
-rw-r--r-- | packages/astro/src/i18n/middleware.ts | 32 | ||||
-rw-r--r-- | packages/astro/src/vite-plugin-astro-server/route.ts | 8 | ||||
-rw-r--r-- | packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro | 4 | ||||
-rw-r--r-- | packages/astro/test/i18n-routing.test.js | 16 | ||||
-rw-r--r-- | packages/astro/test/units/routing/endpoints.test.js | 9 |
8 files changed, 61 insertions, 20 deletions
diff --git a/.changeset/curvy-trees-compare.md b/.changeset/curvy-trees-compare.md new file mode 100644 index 000000000..0580ab6d7 --- /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 2c334223f..7c8d0067a 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 50aa7b710..1bf5d652a 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 2098ffc7a..4affbf931 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; @@ -202,6 +193,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 * @param currentLocale diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 19af22ff6..1c533dbc8 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 000000000..045415d44 --- /dev/null +++ b/packages/astro/test/fixtures/i18n-routing-prefix-always/src/pages/404.astro @@ -0,0 +1,4 @@ +<html> + <head><title>Not Found</title></head> + <body>Can't find the page youre looking for.</body> +</html> diff --git a/packages/astro/test/i18n-routing.test.js b/packages/astro/test/i18n-routing.test.js index 8be9847d3..23255ef9b 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 cff44f1b4..43d76af5a 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); }); }); |