diff options
author | 2024-06-05 13:09:51 +0100 | |
---|---|---|
committer | 2024-06-05 13:09:51 +0100 | |
commit | 35ef53c0897c0d360efc086a71c5f4406721d2fe (patch) | |
tree | 9000904414d7b24aa0d167897ce8bedafaa7c343 | |
parent | 6bcbd15f86cddeb45b982e9d12fb2da35b9ff0e1 (diff) | |
download | astro-35ef53c0897c0d360efc086a71c5f4406721d2fe.tar.gz astro-35ef53c0897c0d360efc086a71c5f4406721d2fe.tar.zst astro-35ef53c0897c0d360efc086a71c5f4406721d2fe.zip |
fix: bubble up errors in rewrites (#11136)
* fix: bubble up errors in rewrites
* docs
* remove commented code
* changesets
* fix string interpolation
24 files changed, 360 insertions, 167 deletions
diff --git a/.changeset/dull-lizards-jog.md b/.changeset/dull-lizards-jog.md new file mode 100644 index 000000000..9f5d7bdb2 --- /dev/null +++ b/.changeset/dull-lizards-jog.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Errors that are emitted during a rewrite are now bubbled up and shown to the user. A 404 response is not returned anymore. diff --git a/.changeset/nervous-pens-retire.md b/.changeset/nervous-pens-retire.md new file mode 100644 index 000000000..7815d37ad --- /dev/null +++ b/.changeset/nervous-pens-retire.md @@ -0,0 +1,7 @@ +--- +"astro": patch +--- + +It's not possible anymore to use `Astro.rewrite("/404")` inside static pages. This isn't counterproductive because Astro will end-up emitting a page that contains the HTML of 404 error page. + +It's still possible to use `Astro.rewrite("/404")` inside on-demand pages, or pages that opt-out from prerendering. diff --git a/packages/astro/src/container/pipeline.ts b/packages/astro/src/container/pipeline.ts index d48d13e42..5dae72b64 100644 --- a/packages/astro/src/container/pipeline.ts +++ b/packages/astro/src/container/pipeline.ts @@ -7,12 +7,13 @@ import type { } from '../@types/astro.js'; import { type HeadElements, Pipeline } from '../core/base-pipeline.js'; import type { SinglePageBuiltModule } from '../core/build/types.js'; -import { RouteNotFound } from '../core/errors/errors-data.js'; +import { InvalidRewrite404, RouteNotFound } from '../core/errors/errors-data.js'; import { AstroError } from '../core/errors/index.js'; import { createModuleScriptElement, createStylesheetElementSet, } from '../core/render/ssr-element.js'; +import { default404Page } from '../core/routing/astro-designed-error-pages.js'; export class ContainerPipeline extends Pipeline { /** @@ -111,4 +112,12 @@ export class ContainerPipeline extends Pipeline { // At the moment it's not used by the container via any public API // @ts-expect-error It needs to be implemented. async getComponentByRoute(_routeData: RouteData): Promise<ComponentInstance> {} + + rewriteKnownRoute(pathname: string, _sourceRoute: RouteData): ComponentInstance { + if (pathname === '/404') { + return { default: default404Page } as any as ComponentInstance; + } + + throw new AstroError(InvalidRewrite404); + } } diff --git a/packages/astro/src/core/app/pipeline.ts b/packages/astro/src/core/app/pipeline.ts index 3eec8068f..b74cc68ee 100644 --- a/packages/astro/src/core/app/pipeline.ts +++ b/packages/astro/src/core/app/pipeline.ts @@ -9,8 +9,11 @@ import type { import { Pipeline } from '../base-pipeline.js'; import type { SinglePageBuiltModule } from '../build/types.js'; import { DEFAULT_404_COMPONENT } from '../constants.js'; +import { RewriteEncounteredAnError } from '../errors/errors-data.js'; +import { AstroError } from '../errors/index.js'; import { RedirectSinglePageBuiltModule } from '../redirects/component.js'; import { createModuleScriptElement, createStylesheetElementSet } from '../render/ssr-element.js'; +import { DEFAULT_404_ROUTE } from '../routing/astro-designed-error-pages.js'; export class AppPipeline extends Pipeline { #manifestData: ManifestData | undefined; @@ -82,36 +85,44 @@ export class AppPipeline extends Pipeline { async tryRewrite( payload: RewritePayload, - request: Request + request: Request, + sourceRoute: RouteData ): Promise<[RouteData, ComponentInstance]> { let foundRoute; for (const route of this.#manifestData!.routes) { + let finalUrl: URL | undefined = undefined; + if (payload instanceof URL) { - if (route.pattern.test(payload.pathname)) { - foundRoute = route; - break; - } + finalUrl = payload; } else if (payload instanceof Request) { - const url = new URL(payload.url); - if (route.pattern.test(url.pathname)) { - foundRoute = route; - break; - } + finalUrl = new URL(payload.url); } else { - const newUrl = new URL(payload, new URL(request.url).origin); - if (route.pattern.test(decodeURI(newUrl.pathname))) { - foundRoute = route; - break; - } + finalUrl = new URL(payload, new URL(request.url).origin); + } + + if (route.pattern.test(decodeURI(finalUrl.pathname))) { + foundRoute = route; + break; + } else if (finalUrl.pathname === '/404') { + foundRoute = DEFAULT_404_ROUTE; + break; } } if (foundRoute) { - const componentInstance = await this.getComponentByRoute(foundRoute); - return [foundRoute, componentInstance]; + if (foundRoute.pathname === '/404') { + const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute); + return [foundRoute, componentInstance]; + } else { + const componentInstance = await this.getComponentByRoute(foundRoute); + return [foundRoute, componentInstance]; + } } - throw new Error('Route not found'); + throw new AstroError({ + ...RewriteEncounteredAnError, + message: RewriteEncounteredAnError.message(payload.toString()), + }); } async getModuleForRoute(route: RouteData): Promise<SinglePageBuiltModule> { @@ -141,4 +152,13 @@ export class AppPipeline extends Pipeline { ); } } + + // We don't need to check the source route, we already are in SSR + rewriteKnownRoute(pathname: string, _sourceRoute: RouteData): ComponentInstance { + if (pathname === '/404') { + return { default: () => new Response(null, { status: 404 }) } as ComponentInstance; + } else { + return { default: () => new Response(null, { status: 500 }) } as ComponentInstance; + } + } } diff --git a/packages/astro/src/core/base-pipeline.ts b/packages/astro/src/core/base-pipeline.ts index 533094bb2..e7baa0f2b 100644 --- a/packages/astro/src/core/base-pipeline.ts +++ b/packages/astro/src/core/base-pipeline.ts @@ -80,11 +80,14 @@ export abstract class Pipeline { * * - if not `RouteData` is found * - * @param {RewritePayload} rewritePayload + * @param {RewritePayload} rewritePayload The payload provided by the user + * @param {Request} request The original request + * @param {RouteData} sourceRoute The original `RouteData` */ abstract tryRewrite( rewritePayload: RewritePayload, - request: Request + request: Request, + sourceRoute: RouteData ): Promise<[RouteData, ComponentInstance]>; /** @@ -92,6 +95,16 @@ export abstract class Pipeline { * @param routeData */ abstract getComponentByRoute(routeData: RouteData): Promise<ComponentInstance>; + + /** + * Attempts to execute a rewrite of a known Astro route: + * - /404 -> src/pages/404.astro -> template + * - /500 -> src/pages/500.astro + * + * @param pathname The pathname where the user wants to rewrite e.g. "/404" + * @param sourceRoute The original route where the first request started. This is needed in case a pipeline wants to check if the current route is pre-rendered or not + */ + abstract rewriteKnownRoute(pathname: string, sourceRoute: RouteData): ComponentInstance; } // eslint-disable-next-line @typescript-eslint/no-empty-interface diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index e5bf622a9..56faa7a7c 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -44,8 +44,6 @@ import { getOutputFilename, isServerLikeOutput } from '../util.js'; import { getOutDirWithinCwd, getOutFile, getOutFolder } from './common.js'; import { cssOrder, mergeInlineCss } from './internal.js'; import { BuildPipeline } from './pipeline.js'; -import { ASTRO_PAGE_MODULE_ID } from './plugins/plugin-pages.js'; -import { getVirtualModulePageName } from './plugins/util.js'; import type { PageBuildData, SinglePageBuiltModule, diff --git a/packages/astro/src/core/build/pipeline.ts b/packages/astro/src/core/build/pipeline.ts index 664d30c9a..59e4b2110 100644 --- a/packages/astro/src/core/build/pipeline.ts +++ b/packages/astro/src/core/build/pipeline.ts @@ -8,7 +8,7 @@ import type { import { getOutputDirectory } from '../../prerender/utils.js'; import { BEFORE_HYDRATION_SCRIPT_ID, PAGE_SCRIPT_ID } from '../../vite-plugin-scripts/index.js'; import type { SSRManifest } from '../app/types.js'; -import { RouteNotFound } from '../errors/errors-data.js'; +import { InvalidRewrite404, RewriteEncounteredAnError } from '../errors/errors-data.js'; import { AstroError } from '../errors/index.js'; import { routeIsFallback, routeIsRedirect } from '../redirects/helpers.js'; import { RedirectSinglePageBuiltModule } from '../redirects/index.js'; @@ -18,6 +18,7 @@ import { createModuleScriptsSet, createStylesheetElementSet, } from '../render/ssr-element.js'; +import { DEFAULT_404_ROUTE } from '../routing/astro-designed-error-pages.js'; import { isServerLikeOutput } from '../util.js'; import { getOutDirWithinCwd } from './common.js'; import { type BuildInternals, cssOrder, getPageData, mergeInlineCss } from './internal.js'; @@ -278,35 +279,43 @@ export class BuildPipeline extends Pipeline { async tryRewrite( payload: RewritePayload, - request: Request + request: Request, + sourceRoute: RouteData ): Promise<[RouteData, ComponentInstance]> { let foundRoute: RouteData | undefined; // options.manifest is the actual type that contains the information for (const route of this.options.manifest.routes) { + let finalUrl: URL | undefined = undefined; + if (payload instanceof URL) { - if (route.pattern.test(payload.pathname)) { - foundRoute = route; - break; - } + finalUrl = payload; } else if (payload instanceof Request) { - const url = new URL(payload.url); - if (route.pattern.test(url.pathname)) { - foundRoute = route; - break; - } + finalUrl = new URL(payload.url); } else { - const newUrl = new URL(payload, new URL(request.url).origin); - if (route.pattern.test(decodeURI(newUrl.pathname))) { - foundRoute = route; - break; - } + finalUrl = new URL(payload, new URL(request.url).origin); + } + + if (route.pattern.test(decodeURI(finalUrl.pathname))) { + foundRoute = route; + break; + } else if (finalUrl.pathname === '/404') { + foundRoute = DEFAULT_404_ROUTE; + break; } } if (foundRoute) { - const componentInstance = await this.getComponentByRoute(foundRoute); - return [foundRoute, componentInstance]; + if (foundRoute.pathname === '/404') { + const componentInstance = await this.rewriteKnownRoute(foundRoute.pathname, sourceRoute); + return [foundRoute, componentInstance]; + } else { + const componentInstance = await this.getComponentByRoute(foundRoute); + return [foundRoute, componentInstance]; + } } else { - throw new AstroError(RouteNotFound); + throw new AstroError({ + ...RewriteEncounteredAnError, + message: RewriteEncounteredAnError.message(payload.toString()), + }); } } @@ -367,6 +376,13 @@ export class BuildPipeline extends Pipeline { return RedirectSinglePageBuiltModule; } + + rewriteKnownRoute(_pathname: string, sourceRoute: RouteData): ComponentInstance { + if (!isServerLikeOutput(this.config) || sourceRoute.prerender) { + throw new AstroError(InvalidRewrite404); + } + throw new Error(`Unreachable, in SSG this route shouldn't be generated`); + } } function createEntryURL(filePath: string, outFolder: URL) { diff --git a/packages/astro/src/core/constants.ts b/packages/astro/src/core/constants.ts index 3347c1ec5..a4d32abe5 100644 --- a/packages/astro/src/core/constants.ts +++ b/packages/astro/src/core/constants.ts @@ -30,7 +30,7 @@ export const ROUTE_TYPE_HEADER = 'X-Astro-Route-Type'; /** * The value of the `component` field of the default 404 page, which is used when there is no user-provided 404.astro page. */ -export const DEFAULT_404_COMPONENT = 'astro-default-404'; +export const DEFAULT_404_COMPONENT = 'astro-default-404.astro'; /** * A response with one of these status codes will be rewritten diff --git a/packages/astro/src/core/errors/errors-data.ts b/packages/astro/src/core/errors/errors-data.ts index 217ade537..1528d253f 100644 --- a/packages/astro/src/core/errors/errors-data.ts +++ b/packages/astro/src/core/errors/errors-data.ts @@ -1120,6 +1120,29 @@ export const MissingMiddlewareForInternationalization = { /** * @docs * @description + * The user tried to rewrite using a route that doesn't exist, or it emitted a runtime error during its rendering phase. + */ +export const RewriteEncounteredAnError = { + name: 'RewriteEncounteredAnError', + title: "Astro couldn't find the route to rewrite, or if was found but it emitted an error during the rendering phase.", + message: (route: string, stack?: string) => `The route ${route} that you tried to render doesn't exist, or it emitted an error during the rendering phase. ${stack ? stack : ""}.` +} satisfies ErrorData; + +/** + * @docs + * @description + * + * The user tried to rewrite a 404 page inside a static page. + */ +export const InvalidRewrite404 = { + name: 'InvalidRewrite404', + title: "You attempted to rewrite a 404 inside a static page, and this isn't allowed.", + message: 'Rewriting a 404 is only allowed inside on-demand pages.', +} satisfies ErrorData; + +/** + * @docs + * @description * Astro could not find an associated file with content while trying to render the route. This is an Astro error and not a user error. If restarting the dev server does not fix the problem, please file an issue. */ export const CantRenderPage = { diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts index 22d259470..5a484446b 100644 --- a/packages/astro/src/core/render-context.ts +++ b/packages/astro/src/core/render-context.ts @@ -20,6 +20,7 @@ import { renderEndpoint } from '../runtime/server/endpoint.js'; import { renderPage } from '../runtime/server/index.js'; import { ASTRO_VERSION, + DEFAULT_404_COMPONENT, REROUTE_DIRECTIVE_HEADER, ROUTE_TYPE_HEADER, clientAddressSymbol, @@ -38,6 +39,9 @@ import { type Pipeline, Slots, getParams, getProps } from './render/index.js'; * It contains data unique to each request. It is responsible for executing middleware, calling endpoints, and rendering the page by gathering necessary data from a `Pipeline`. */ export class RenderContext { + // The first route that this instance of the context attempts to render + originalRoute: RouteData; + private constructor( readonly pipeline: Pipeline, public locals: App.Locals, @@ -50,7 +54,9 @@ export class RenderContext { public params = getParams(routeData, pathname), protected url = new URL(request.url), public props: Props = {} - ) {} + ) { + this.originalRoute = routeData; + } /** * A flag that tells the render content if the rewriting was triggered @@ -124,18 +130,15 @@ export class RenderContext { const lastNext = async (ctx: APIContext, payload?: RewritePayload) => { if (payload) { if (this.pipeline.manifest.rewritingEnabled) { - try { - const [routeData, component] = await pipeline.tryRewrite(payload, this.request); - this.routeData = routeData; - componentInstance = component; - } catch (e) { - return new Response('Not found', { - status: 404, - statusText: 'Not found', - }); - } finally { - this.isRewriting = true; - } + // we intentionally let the error bubble up + const [routeData, component] = await pipeline.tryRewrite( + payload, + this.request, + this.originalRoute + ); + this.routeData = routeData; + componentInstance = component; + this.isRewriting = true; } else { this.pipeline.logger.warn( 'router', @@ -166,6 +169,7 @@ export class RenderContext { result.cancelled = true; throw e; } + // 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 @@ -218,29 +222,25 @@ export class RenderContext { const rewrite = async (reroutePayload: RewritePayload) => { pipeline.logger.debug('router', 'Called rewriting to:', reroutePayload); - try { - const [routeData, component] = await pipeline.tryRewrite(reroutePayload, this.request); - this.routeData = routeData; - if (reroutePayload instanceof Request) { - this.request = reroutePayload; - } else { - this.request = this.#copyRequest( - new URL(routeData.pathname ?? routeData.route, this.url.origin), - this.request - ); - } - this.url = new URL(this.request.url); - this.cookies = new AstroCookies(this.request); - this.params = getParams(routeData, url.toString()); - this.isRewriting = true; - return await this.render(component); - } catch (e) { - pipeline.logger.debug('router', 'Rewrite failed.', e); - return new Response('Not found', { - status: 404, - statusText: 'Not found', - }); + const [routeData, component] = await pipeline.tryRewrite( + reroutePayload, + this.request, + this.originalRoute + ); + this.routeData = routeData; + if (reroutePayload instanceof Request) { + this.request = reroutePayload; + } else { + this.request = this.#copyRequest( + new URL(routeData.pathname ?? routeData.route, this.url.origin), + this.request + ); } + this.url = new URL(this.request.url); + this.cookies = new AstroCookies(this.request); + this.params = getParams(routeData, url.toString()); + this.isRewriting = true; + return await this.render(component); }; return { @@ -409,30 +409,26 @@ export class RenderContext { }; const rewrite = async (reroutePayload: RewritePayload) => { - try { - pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload); - const [routeData, component] = await pipeline.tryRewrite(reroutePayload, this.request); - this.routeData = routeData; - if (reroutePayload instanceof Request) { - this.request = reroutePayload; - } else { - this.request = this.#copyRequest( - new URL(routeData.pathname ?? routeData.route, this.url.origin), - this.request - ); - } - this.url = new URL(this.request.url); - this.cookies = new AstroCookies(this.request); - this.params = getParams(routeData, url.toString()); - this.isRewriting = true; - return await this.render(component); - } catch (e) { - pipeline.logger.debug('router', 'Rerouting failed, returning a 404.', e); - return new Response('Not found', { - status: 404, - statusText: 'Not found', - }); + pipeline.logger.debug('router', 'Calling rewrite: ', reroutePayload); + const [routeData, component] = await pipeline.tryRewrite( + reroutePayload, + this.request, + this.originalRoute + ); + this.routeData = routeData; + if (reroutePayload instanceof Request) { + this.request = reroutePayload; + } else { + this.request = this.#copyRequest( + new URL(routeData.pathname ?? routeData.route, this.url.origin), + this.request + ); } + this.url = new URL(this.request.url); + this.cookies = new AstroCookies(this.request); + this.params = getParams(routeData, url.toString()); + this.isRewriting = true; + return await this.render(component); }; return { diff --git a/packages/astro/src/core/routing/astro-designed-error-pages.ts b/packages/astro/src/core/routing/astro-designed-error-pages.ts index 1dbe7ed60..6a047f6d5 100644 --- a/packages/astro/src/core/routing/astro-designed-error-pages.ts +++ b/packages/astro/src/core/routing/astro-designed-error-pages.ts @@ -1,20 +1,38 @@ -import type { ManifestData } from '../../@types/astro.js'; +import type { ManifestData, RouteData } from '../../@types/astro.js'; +import notFoundTemplate from '../../template/4xx.js'; import { DEFAULT_404_COMPONENT } from '../constants.js'; +export const DEFAULT_404_ROUTE: RouteData = { + component: DEFAULT_404_COMPONENT, + generate: () => '', + params: [], + pattern: /\/404/, + prerender: false, + pathname: '/404', + segments: [[{ content: '404', dynamic: false, spread: false }]], + type: 'page', + route: '/404', + fallbackRoutes: [], + isIndex: false, +}; + export function ensure404Route(manifest: ManifestData) { if (!manifest.routes.some((route) => route.route === '/404')) { - manifest.routes.push({ - component: DEFAULT_404_COMPONENT, - generate: () => '', - params: [], - pattern: /\/404/, - prerender: false, - segments: [[{ content: '404', dynamic: false, spread: false }]], - type: 'page', - route: '/404', - fallbackRoutes: [], - isIndex: false, - }); + manifest.routes.push(DEFAULT_404_ROUTE); } return manifest; } + +export async function default404Page({ pathname }: { pathname: string }) { + return new Response( + notFoundTemplate({ + statusCode: 404, + title: 'Not found', + tabTitle: '404: Not Found', + pathname, + }), + { status: 404, headers: { 'Content-Type': 'text/html; charset=utf-8' } } + ); +} +// mark the function as an AstroComponentFactory for the rendering internals +default404Page.isAstroComponentFactory = true; diff --git a/packages/astro/src/vite-plugin-astro-server/pipeline.ts b/packages/astro/src/vite-plugin-astro-server/pipeline.ts index 260f7fa1f..66cce8961 100644 --- a/packages/astro/src/vite-plugin-astro-server/pipeline.ts +++ b/packages/astro/src/vite-plugin-astro-server/pipeline.ts @@ -11,20 +11,20 @@ import type { SSRManifest, } from '../@types/astro.js'; import { getInfoOutput } from '../cli/info/index.js'; -import type { HeadElements } from '../core/base-pipeline.js'; +import { type HeadElements } from '../core/base-pipeline.js'; import { ASTRO_VERSION, DEFAULT_404_COMPONENT } from '../core/constants.js'; import { enhanceViteSSRError } from '../core/errors/dev/index.js'; -import { RouteNotFound } from '../core/errors/errors-data.js'; +import { InvalidRewrite404, RewriteEncounteredAnError } from '../core/errors/errors-data.js'; import { AggregateError, AstroError, CSSError, MarkdownError } from '../core/errors/index.js'; import type { Logger } from '../core/logger/core.js'; import type { ModuleLoader } from '../core/module-loader/index.js'; import { Pipeline, loadRenderer } from '../core/render/index.js'; +import { DEFAULT_404_ROUTE, default404Page } from '../core/routing/astro-designed-error-pages.js'; import { isPage, isServerLikeOutput, resolveIdToUrl, viteID } from '../core/util.js'; import { PAGE_SCRIPT_ID } from '../vite-plugin-scripts/index.js'; import { getStylesForURL } from './css.js'; import { getComponentMetadata } from './metadata.js'; import { createResolve } from './resolve.js'; -import { default404Page } from './response.js'; import { getScriptsForURL } from './scripts.js'; export class DevPipeline extends Pipeline { @@ -193,7 +193,8 @@ export class DevPipeline extends Pipeline { async tryRewrite( payload: RewritePayload, - request: Request + request: Request, + sourceRoute: RouteData ): Promise<[RouteData, ComponentInstance]> { let foundRoute; if (!this.manifestData) { @@ -201,35 +202,52 @@ export class DevPipeline extends Pipeline { } for (const route of this.manifestData.routes) { + let finalUrl: URL | undefined = undefined; + if (payload instanceof URL) { - if (route.pattern.test(payload.pathname)) { - foundRoute = route; - break; - } + finalUrl = payload; } else if (payload instanceof Request) { - const url = new URL(payload.url); - if (route.pattern.test(url.pathname)) { - foundRoute = route; - break; - } + finalUrl = new URL(payload.url); } else { - const newUrl = new URL(payload, new URL(request.url).origin); - if (route.pattern.test(decodeURI(newUrl.pathname))) { - foundRoute = route; - break; - } + finalUrl = new URL(payload, new URL(request.url).origin); + } + + if (route.pattern.test(decodeURI(finalUrl.pathname))) { + foundRoute = route; + break; + } else if (finalUrl.pathname === '/404') { + foundRoute = DEFAULT_404_ROUTE; + break; } } if (foundRoute) { - const componentInstance = await this.getComponentByRoute(foundRoute); - return [foundRoute, componentInstance]; + if (foundRoute.pathname === '/404') { + const componentInstance = this.rewriteKnownRoute(foundRoute.pathname, sourceRoute); + return [foundRoute, componentInstance]; + } else { + const componentInstance = await this.getComponentByRoute(foundRoute); + return [foundRoute, componentInstance]; + } } else { - throw new AstroError(RouteNotFound); + throw new AstroError({ + ...RewriteEncounteredAnError, + message: RewriteEncounteredAnError.message(payload.toString()), + }); } } setManifestData(manifestData: ManifestData) { this.manifestData = manifestData; } + + rewriteKnownRoute(route: string, sourceRoute: RouteData): ComponentInstance { + if (isServerLikeOutput(this.config) && sourceRoute.prerender) { + if (route === '/404') { + return { default: default404Page } as any as ComponentInstance; + } + } + + throw new AstroError(InvalidRewrite404); + } } diff --git a/packages/astro/src/vite-plugin-astro-server/response.ts b/packages/astro/src/vite-plugin-astro-server/response.ts index bdf0e3c15..c6e034aef 100644 --- a/packages/astro/src/vite-plugin-astro-server/response.ts +++ b/packages/astro/src/vite-plugin-astro-server/response.ts @@ -23,20 +23,6 @@ export async function handle404Response( writeHtmlResponse(res, 404, html); } -export async function default404Page({ pathname }: { pathname: string }) { - return new Response( - notFoundTemplate({ - statusCode: 404, - title: 'Not found', - tabTitle: '404: Not Found', - pathname, - }), - { status: 404, headers: { 'Content-Type': 'text/html; charset=utf-8' } } - ); -} -// mark the function as an AstroComponentFactory for the rendering internals -default404Page.isAstroComponentFactory = true; - export async function handle500Response( loader: ModuleLoader, res: http.ServerResponse, diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts index 95dd98135..b451a8ff6 100644 --- a/packages/astro/src/vite-plugin-astro-server/route.ts +++ b/packages/astro/src/vite-plugin-astro-server/route.ts @@ -11,11 +11,12 @@ import { loadMiddleware } from '../core/middleware/loadMiddleware.js'; import { RenderContext } from '../core/render-context.js'; import { type SSROptions, getProps } from '../core/render/index.js'; import { createRequest } from '../core/request.js'; +import { default404Page } from '../core/routing/astro-designed-error-pages.js'; import { matchAllRoutes } from '../core/routing/index.js'; import { normalizeTheLocale } from '../i18n/index.js'; import { getSortedPreloadedMatches } from '../prerender/routing.js'; import type { DevPipeline } from './pipeline.js'; -import { default404Page, handle404Response, writeSSRResult, writeWebResponse } from './response.js'; +import { handle404Response, writeSSRResult, writeWebResponse } from './response.js'; type AsyncReturnType<T extends (...args: any) => Promise<any>> = T extends ( ...args: any diff --git a/packages/astro/test/fixtures/reroute/src/pages/blog/oops.astro b/packages/astro/test/fixtures/reroute/src/pages/blog/oops.astro deleted file mode 100644 index df1f1f76a..000000000 --- a/packages/astro/test/fixtures/reroute/src/pages/blog/oops.astro +++ /dev/null @@ -1,11 +0,0 @@ ---- -return Astro.rewrite("/404") ---- -<html> -<head> - <title>Blog hello</title> -</head> -<body> -<h1>Blog hello</h1> -</body> -</html> diff --git a/packages/astro/test/fixtures/rewrite-404-invalid/astro.config.mjs b/packages/astro/test/fixtures/rewrite-404-invalid/astro.config.mjs new file mode 100644 index 000000000..af13ef19b --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-404-invalid/astro.config.mjs @@ -0,0 +1,9 @@ +import { defineConfig } from 'astro/config'; + +// https://astro.build/config +export default defineConfig({ + experimental: { + rewriting: true + }, + site: "https://example.com" +}); diff --git a/packages/astro/test/fixtures/rewrite-404-invalid/package.json b/packages/astro/test/fixtures/rewrite-404-invalid/package.json new file mode 100644 index 000000000..994743ef2 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-404-invalid/package.json @@ -0,0 +1,8 @@ +{ + "name": "@test/rewrite-404-invalid", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*" + } +} diff --git a/packages/astro/test/fixtures/rewrite-404-invalid/src/pages/rewrite-404.astro b/packages/astro/test/fixtures/rewrite-404-invalid/src/pages/rewrite-404.astro new file mode 100644 index 000000000..536eb3501 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-404-invalid/src/pages/rewrite-404.astro @@ -0,0 +1,3 @@ +--- +return Astro.rewrite("/404") +--- diff --git a/packages/astro/test/fixtures/rewrite-runtime-error/astro.config.mjs b/packages/astro/test/fixtures/rewrite-runtime-error/astro.config.mjs new file mode 100644 index 000000000..af13ef19b --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error/astro.config.mjs @@ -0,0 +1,9 @@ +import { defineConfig } from 'astro/config'; + +// https://astro.build/config +export default defineConfig({ + experimental: { + rewriting: true + }, + site: "https://example.com" +}); diff --git a/packages/astro/test/fixtures/rewrite-runtime-error/package.json b/packages/astro/test/fixtures/rewrite-runtime-error/package.json new file mode 100644 index 000000000..af8c42709 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error/package.json @@ -0,0 +1,8 @@ +{ + "name": "@test/rewrite-runtime-errror", + "version": "0.0.0", + "private": true, + "dependencies": { + "astro": "workspace:*" + } +} diff --git a/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/from.astro b/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/from.astro new file mode 100644 index 000000000..057629c36 --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/from.astro @@ -0,0 +1,4 @@ +--- +return Astro.rewrite('/errors/to'); +--- +<div></div> diff --git a/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/to.astro b/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/to.astro new file mode 100644 index 000000000..65be84e8b --- /dev/null +++ b/packages/astro/test/fixtures/rewrite-runtime-error/src/pages/errors/to.astro @@ -0,0 +1,4 @@ +--- +throw new Error('Custom error') +--- +<div></div> diff --git a/packages/astro/test/rewrite.test.js b/packages/astro/test/rewrite.test.js index 3808a9181..d66badaac 100644 --- a/packages/astro/test/rewrite.test.js +++ b/packages/astro/test/rewrite.test.js @@ -55,11 +55,10 @@ describe('Dev reroute', () => { assert.equal($('h1').text(), 'Index'); }); - it('should render the 404 built-in page', async () => { - const html = await fixture.fetch('/blog/oops').then((res) => res.text()); - const $ = cheerioLoad(html); + it('should return a 404', async () => { + const response = await fixture.fetch('/blog/oops'); - assert.equal($('h1').text(), '404: Not found'); + assert.equal(response.status, 404); }); }); @@ -120,6 +119,21 @@ describe('Build reroute', () => { }); }); +describe('SSR route', () => { + it("should not build if a user tries to use rewrite('/404') in static pages", async () => { + try { + const fixture = await loadFixture({ + root: './fixtures/rewrite-404-invalid/', + }); + await fixture.build(); + assert.fail('It should fail.'); + } catch { + // it passes + assert.equal(true, true); + } + }); +}); + describe('SSR reroute', () => { /** @type {import('./test-utils').Fixture} */ let fixture; @@ -184,8 +198,7 @@ describe('SSR reroute', () => { it('should render the 404 built-in page', async () => { const request = new Request('http://example.com/blog/oops'); const response = await app.render(request); - const html = await response.text(); - assert.equal(html, 'Not found'); + assert.equal(response.status, 404); }); it('should pass the POST data from one page to another', async () => { @@ -246,3 +259,27 @@ describe('Middleware', () => { assert.match($('h1').text(), /Index/); }); }); + +describe('Runtime error', () => { + /** @type {import('./test-utils').Fixture} */ + let fixture; + let devServer; + + before(async () => { + fixture = await loadFixture({ + root: './fixtures/rewrite-runtime-error/', + }); + devServer = await fixture.startDevServer(); + }); + + after(async () => { + await devServer.stop(); + }); + + it('should render the index page when navigating /reroute ', async () => { + const html = await fixture.fetch('/errors/from').then((res) => res.text()); + const $ = cheerioLoad(html); + + assert.equal($('title').text(), 'Error'); + }); +}); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 765d42d62..02bc89d0d 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -3466,6 +3466,18 @@ importers: specifier: workspace:* version: link:../../.. + packages/astro/test/fixtures/rewrite-404-invalid: + dependencies: + astro: + specifier: workspace:* + version: link:../../.. + + packages/astro/test/fixtures/rewrite-runtime-error: + dependencies: + astro: + specifier: workspace:* + version: link:../../.. + packages/astro/test/fixtures/root-srcdir-css: dependencies: astro: |