Feat: show 404 when getStaticPaths doesn't match URL (#2743)

* WIP: return 404 for unmatched getStaticPaths route

* feat: regex on static paths to 404 in dev

* Revert "WIP: return 404 for unmatched getStaticPaths route"

This reverts commit 9c395a2586ca40d44c3ab18edc7ffbc1c4660ed8.

* feat: call getParamsAndProps pre-ssr to catch errs

* fix: remove unused cache regex check

* fix: revert getPattern changes

* fix: remove unused preload props

* fix: log 404 for custom 404 pages

* refactor: rename fixture for clarity

* feat: add getStaticPaths status code tests

* fix: pas rootRelativeUrl to handle subpaths

* fix: update dev-routing tests from 500 -> 404

* refactor: make error handling more explicit

* lint: use typescript no shadow to fix enum issue

* chore: add changeset

* refactor: clarify test names

* refactor: remove variable reassignment

* fix: update dev-routing tests 500 > 404

* refactor: update test file structure

* Fix: revert to old logging

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

* Chore: use `const enum` instead

Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

* chore: format

Co-authored-by: Nate Moore <nate@skypack.dev>
Co-authored-by: Nate Moore <natemoo-re@users.noreply.github.com>

.changeset/young-rules-draw.md

@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fix - show 404 for bad static paths with console message, rather than a 500

.eslintrc.cjs

@@ -14,8 +14,9 @@ module.exports = {
     '@typescript-eslint/no-var-requires': 'off',
     '@typescript-eslint/no-this-alias': 'off',
     'no-console': 'warn',
-    'no-shadow': 'error',
     'prefer-const': 'off',
+    'no-shadow': 'off',
+    '@typescript-eslint/no-shadow': ['error'],
     // 'require-jsdoc': 'error', // re-enable this to enforce JSDoc for all functions
   },
 };

packages/astro/src/core/build/page-data.ts

@@ -44,12 +44,6 @@ export async function collectPagesData(opts: CollectPagesDataOptions): Promise<C
 				preload: await ssrPreload({
 					astroConfig,
 					filePath: new URL(`./${route.component}`, astroConfig.projectRoot),
-					logging,
-					mode: 'production',
-					origin,
-					pathname: route.pathname,
-					route,
-					routeCache,
 					viteServer,
 				})
 					.then((routes) => {
@@ -106,12 +100,6 @@ export async function collectPagesData(opts: CollectPagesDataOptions): Promise<C
 			preload: await ssrPreload({
 				astroConfig,
 				filePath: new URL(`./${route.component}`, astroConfig.projectRoot),
-				logging,
-				mode: 'production',
-				origin,
-				pathname: finalPaths[0],
-				route,
-				routeCache,
 				viteServer,
 			}),
 		};

packages/astro/src/core/render/core.ts

@@ -15,7 +15,11 @@ interface GetParamsAndPropsOptions {
 	logging: LogOptions;
 }
 
-async function getParamsAndProps(opts: GetParamsAndPropsOptions): Promise<[Params, Props]> {
+export const enum GetParamsAndPropsError {
+	NoMatchingStaticPath,
+}
+
+export async function getParamsAndProps(opts: GetParamsAndPropsOptions): Promise<[Params, Props] | GetParamsAndPropsError> {
 	const { logging, mod, route, routeCache, pathname } = opts;
 	// Handle dynamic routes
 	let params: Params = {};
@@ -38,7 +42,7 @@ async function getParamsAndProps(opts: GetParamsAndPropsOptions): Promise<[Param
 		}
 		const matchedStaticPath = findPathItemByKey(routeCacheEntry.staticPaths, params);
 		if (!matchedStaticPath) {
-			throw new Error(`[getStaticPaths] route pattern matched, but no matching static path found. (${pathname})`);
+			return GetParamsAndPropsError.NoMatchingStaticPath;
 		}
 		// This is written this way for performance; instead of spreading the props
 		// which is O(n), create a new object that extends props.
@@ -68,7 +72,7 @@ interface RenderOptions {
 export async function render(opts: RenderOptions): Promise<string> {
 	const { legacyBuild, links, logging, origin, markdownRender, mod, pathname, scripts, renderers, resolve, route, routeCache, site } = opts;
 
-	const [params, pageProps] = await getParamsAndProps({
+	const paramsAndPropsRes = await getParamsAndProps({
 		logging,
 		mod,
 		route,
@@ -76,6 +80,11 @@ export async function render(opts: RenderOptions): Promise<string> {
 		pathname,
 	});
 
+	if (paramsAndPropsRes === GetParamsAndPropsError.NoMatchingStaticPath) {
+		throw new Error(`[getStaticPath] route pattern matched, but no matching static path found. (${pathname})`);
+	}
+	const [params, pageProps] = paramsAndPropsRes;
+
 	// For endpoints, render the content immediately without injecting scripts or styles
 	if (route?.type === 'endpoint') {
 		return renderEndpoint(mod as any as EndpointHandler, params);

packages/astro/src/core/render/dev/index.ts

@@ -36,7 +36,7 @@ export type ComponentPreload = [Renderer[], ComponentInstance];
 
 const svelteStylesRE = /svelte\?svelte&type=style/;
 
-export async function preload({ astroConfig, filePath, viteServer }: SSROptions): Promise<ComponentPreload> {
+export async function preload({ astroConfig, filePath, viteServer }: Pick<SSROptions, 'astroConfig' | 'filePath' | 'viteServer'>): Promise<ComponentPreload> {
 	// Important: This needs to happen first, in case a renderer provides polyfills.
 	const renderers = await resolveRenderers(viteServer, astroConfig);
 	// Load the module from the Vite SSR Runtime.
@@ -173,9 +173,9 @@ export async function render(renderers: Renderer[], mod: ComponentInstance, ssrO
 	return content;
 }
 
-export async function ssr(ssrOpts: SSROptions): Promise<string> {
+export async function ssr(preloadedComponent: ComponentPreload, ssrOpts: SSROptions): Promise<string> {
 	try {
-		const [renderers, mod] = await preload(ssrOpts);
+		const [renderers, mod] = preloadedComponent;
 		return await render(renderers, mod, ssrOpts); // note(drew): without "await", errors won’t get caught by errorHandler()
 	} catch (e: unknown) {
 		await errorHandler(e, { viteServer: ssrOpts.viteServer, filePath: ssrOpts.filePath });

packages/astro/src/vite-plugin-astro-server/index.ts

@@ -1,11 +1,12 @@
 import type * as vite from 'vite';
 import type http from 'http';
 import type { AstroConfig, ManifestData } from '../@types/astro';
-import { info, error, LogOptions } from '../core/logger.js';
+import { info, warn, error, LogOptions } from '../core/logger.js';
+import { getParamsAndProps, GetParamsAndPropsError } from '../core/render/core.js';
 import { createRouteManifest, matchRoute } from '../core/routing/index.js';
 import stripAnsi from 'strip-ansi';
 import { createSafeError } from '../core/util.js';
-import { ssr } from '../core/render/dev/index.js';
+import { ssr, preload } from '../core/render/dev/index.js';
 import * as msg from '../core/messages.js';
 
 import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js';
@@ -63,6 +64,15 @@ async function handle500Response(viteServer: vite.ViteDevServer, origin: string,
 	writeHtmlResponse(res, 500, transformedHtml);
 }
 
+function getCustom404Route(config: AstroConfig, manifest: ManifestData) {
+	const relPages = config.pages.href.replace(config.projectRoot.href, '');
+	return manifest.routes.find((r) => r.component === relPages + '404.astro');
+}
+
+function log404(logging: LogOptions, pathname: string) {
+	info(logging, 'serve', msg.req({ url: pathname, statusCode: 404 }));
+}
+
 /** The main logic to route dev server requests to pages in Astro. */
 async function handleRequest(
 	routeCache: RouteCache,
@@ -79,37 +89,73 @@ async function handleRequest(
 	const origin = `${viteServer.config.server.https ? 'https' : 'http'}://${req.headers.host}`;
 	const pathname = decodeURI(new URL(origin + req.url).pathname);
 	const rootRelativeUrl = pathname.substring(devRoot.length - 1);
+
 	try {
 		if (!pathname.startsWith(devRoot)) {
-			info(logging, 'serve', msg.req({ url: pathname, statusCode: 404 }));
+			log404(logging, pathname);
 			return handle404Response(origin, config, req, res);
 		}
 		// Attempt to match the URL to a valid page route.
 		// If that fails, switch the response to a 404 response.
 		let route = matchRoute(rootRelativeUrl, manifest);
 		const statusCode = route ? 200 : 404;
-		// If no match found, lookup a custom 404 page to render, if one exists.
+
 		if (!route) {
-			const relPages = config.pages.href.replace(config.projectRoot.href, '');
+			log404(logging, pathname);
-			route = manifest.routes.find((r) => r.component === relPages + '404.astro');
+			const custom404 = getCustom404Route(config, manifest);
+			if (custom404) {
+				route = custom404;
+			} else {
+				return handle404Response(origin, config, req, res);
 			}
-		// If still no match is found, respond with a generic 404 page.
-		if (!route) {
-			info(logging, 'serve', msg.req({ url: pathname, statusCode: 404 }));
-			handle404Response(origin, config, req, res);
-			return;
 		}
-		// Route successfully matched! Render it.
+
-		const html = await ssr({
+		const filePath = new URL(`./${route.component}`, config.projectRoot);
+		const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer });
+		const [, mod] = preloadedComponent;
+		// attempt to get static paths
+		// if this fails, we have a bad URL match!
+		const paramsAndPropsRes = await getParamsAndProps({
+			mod,
+			route,
+			routeCache,
+			pathname: rootRelativeUrl,
+			logging,
+		});
+		if (paramsAndPropsRes === GetParamsAndPropsError.NoMatchingStaticPath) {
+			warn(logging, 'getStaticPaths', `Route pattern matched, but no matching static path found. (${pathname})`);
+			log404(logging, pathname);
+			const routeCustom404 = getCustom404Route(config, manifest);
+			if (routeCustom404) {
+				const filePathCustom404 = new URL(`./${routeCustom404.component}`, config.projectRoot);
+				const preloadedCompCustom404 = await preload({ astroConfig: config, filePath: filePathCustom404, viteServer });
+				const html = await ssr(preloadedCompCustom404, {
 					astroConfig: config,
-			filePath: new URL(`./${route.component}`, config.projectRoot),
+					filePath: filePathCustom404,
+					logging,
+					mode: 'development',
+					origin,
+					pathname: rootRelativeUrl,
+					route: routeCustom404,
+					routeCache,
+					viteServer,
+				});
+				return writeHtmlResponse(res, statusCode, html);
+			} else {
+				return handle404Response(origin, config, req, res);
+			}
+		}
+
+		const html = await ssr(preloadedComponent, {
+			astroConfig: config,
+			filePath,
 			logging,
 			mode: 'development',
 			origin,
 			pathname: rootRelativeUrl,
 			route,
-			routeCache: routeCache,
+			routeCache,
-			viteServer: viteServer,
+			viteServer,
 		});
 		writeHtmlResponse(res, statusCode, html);
 	} catch (_err: any) {

packages/astro/test/astro-get-static-paths.test.js

@@ -1,11 +1,9 @@
 import { expect } from 'chai';
 import { loadFixture } from './test-utils.js';
 
-describe('getStaticPaths()', () => {
+describe('getStaticPaths - build calls', () => {
-	let fixture;
-
 	before(async () => {
-		fixture = await loadFixture({
+		const fixture = await loadFixture({
 			projectRoot: './fixtures/astro-get-static-paths/',
 			buildOptions: {
 				site: 'https://mysite.dev/blog/',
@@ -14,9 +12,42 @@ describe('getStaticPaths()', () => {
 		});
 		await fixture.build();
 	});
-
 	it('is only called once during build', () => {
 		// useless expect; if build() throws in setup then this test fails
 		expect(true).to.equal(true);
 	});
 });
+
+describe('getStaticPaths - 404 behavior', () => {
+	let fixture;
+	let devServer;
+
+	before(async () => {
+		fixture = await loadFixture({ projectRoot: './fixtures/astro-get-static-paths/' });
+		devServer = await fixture.startDevServer();
+	});
+
+	after(async () => {
+		devServer && devServer.stop();
+	});
+
+	it('resolves 200 on matching static path - named params', async () => {
+		const res = await fixture.fetch('/pizza/provolone-sausage');
+		expect(res.status).to.equal(200);
+	});
+
+	it('resolves 404 on pattern match without static path - named params', async () => {
+		const res = await fixture.fetch('/pizza/provolone-pineapple');
+		expect(res.status).to.equal(404);
+	});
+
+	it('resolves 200 on matching static path - rest params', async () => {
+		const res = await fixture.fetch('/pizza/grimaldis/new-york');
+		expect(res.status).to.equal(200);
+	});
+
+	it('resolves 404 on pattern match without static path - rest params', async () => {
+		const res = await fixture.fetch('/pizza/pizza-hut');
+		expect(res.status).to.equal(404);
+	});
+});

packages/astro/test/dev-routing.test.js

@@ -38,9 +38,9 @@ describe('Development Routing', () => {
 			expect(response.status).to.equal(200);
 		});
 
-		it('500 when loading invalid dynamic route', async () => {
+		it('404 when loading invalid dynamic route', async () => {
 			const response = await fixture.fetch('/2');
-			expect(response.status).to.equal(500);
+			expect(response.status).to.equal(404);
 		});
 	});
 
@@ -74,9 +74,9 @@ describe('Development Routing', () => {
 			expect(response.status).to.equal(200);
 		});
 
-		it('500 when loading invalid dynamic route', async () => {
+		it('404 when loading invalid dynamic route', async () => {
 			const response = await fixture.fetch('/2');
-			expect(response.status).to.equal(500);
+			expect(response.status).to.equal(404);
 		});
 	});
 
@@ -120,9 +120,9 @@ describe('Development Routing', () => {
 			expect(response.status).to.equal(200);
 		});
 
-		it('500 when loading invalid dynamic route', async () => {
+		it('404 when loading invalid dynamic route', async () => {
 			const response = await fixture.fetch('/blog/2/');
-			expect(response.status).to.equal(500);
+			expect(response.status).to.equal(404);
 		});
 	});
 
@@ -166,9 +166,9 @@ describe('Development Routing', () => {
 			expect(response.status).to.equal(200);
 		});
 
-		it('500 when loading invalid dynamic route', async () => {
+		it('404 when loading invalid dynamic route', async () => {
 			const response = await fixture.fetch('/blog/2/');
-			expect(response.status).to.equal(500);
+			expect(response.status).to.equal(404);
 		});
 	});
 

packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...calledTwiceTest].astro

@@ -5,9 +5,9 @@ export function getStaticPaths({ paginate }) {
   }
   globalThis.isCalledOnce = true;
   return [
-    {params: {test: 'a'}},
+    {params: {calledTwiceTest: 'a'}},
-    {params: {test: 'b'}},
+    {params: {calledTwiceTest: 'b'}},
-    {params: {test: 'c'}},
+    {params: {calledTwiceTest: 'c'}},
   ];
 }
 const { params } = Astro.request;
@@ -15,7 +15,7 @@ const { params } = Astro.request;
 
 <html>
   <head>
-    <title>Page {params.test}</title>
+    <title>Page {params.calledTwiceTest}</title>
   </head>
   <body></body>
 </html>

packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[...pizza].astro

@@ -0,0 +1,22 @@
+---
+export function getStaticPaths() {
+	return [{
+		params: { pizza: 'papa-johns' },
+	}, {
+		params: { pizza: 'dominos' },
+  }, {
+		params: { pizza: 'grimaldis/new-york' },
+  }]
+}
+const { pizza } = Astro.request.params
+---
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>{pizza ?? 'The landing page'}</title>
+	</head>
+	<body>
+		<h1>Welcome to {pizza ?? 'The landing page'}</h1>
+	</body>
+</html>

packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[cheese]-[topping].astro

@@ -0,0 +1,21 @@
+---
+export function getStaticPaths() {
+	return [{
+		params: { cheese: 'mozzarella', topping: 'pepperoni' },
+	}, {
+		params: { cheese: 'provolone', topping: 'sausage' },
+  }]
+}
+const { cheese, topping } = Astro.request.params
+---
+<html lang="en">
+	<head>
+		<meta charset="utf-8" />
+		<meta name="viewport" content="width=device-width" />
+		<title>{cheese}</title>
+	</head>
+	<body>
+		<h1>🍕 It's pizza time</h1>
+		<p>{cheese}-{topping}</p>
+	</body>
+</html>