diff --git a/.changeset/young-rules-draw.md b/.changeset/young-rules-draw.md new file mode 100644 index 000000000..dad5cfc35 --- /dev/null +++ b/.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 diff --git a/.eslintrc.cjs b/.eslintrc.cjs index 911c0eab3..c55d0d05d 100644 --- a/.eslintrc.cjs +++ b/.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 }, }; diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts index f2d737179..53c1b15d0 100644 --- a/packages/astro/src/core/build/page-data.ts +++ b/packages/astro/src/core/build/page-data.ts @@ -44,12 +44,6 @@ export async function collectPagesData(opts: CollectPagesDataOptions): Promise { @@ -106,12 +100,6 @@ export async function collectPagesData(opts: CollectPagesDataOptions): Promise { +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 { 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 { 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); diff --git a/packages/astro/src/core/render/dev/index.ts b/packages/astro/src/core/render/dev/index.ts index 0a59f8157..c3bf8cb5f 100644 --- a/packages/astro/src/core/render/dev/index.ts +++ b/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 { +export async function preload({ astroConfig, filePath, viteServer }: Pick): Promise { // 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 { +export async function ssr(preloadedComponent: ComponentPreload, ssrOpts: SSROptions): Promise { 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 }); diff --git a/packages/astro/src/vite-plugin-astro-server/index.ts b/packages/astro/src/vite-plugin-astro-server/index.ts index ff6825e8f..33baeed71 100644 --- a/packages/astro/src/vite-plugin-astro-server/index.ts +++ b/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, ''); - route = manifest.routes.find((r) => r.component === relPages + '404.astro'); + log404(logging, pathname); + 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; + + 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: filePathCustom404, + logging, + mode: 'development', + origin, + pathname: rootRelativeUrl, + route: routeCustom404, + routeCache, + viteServer, + }); + return writeHtmlResponse(res, statusCode, html); + } else { + return handle404Response(origin, config, req, res); + } } - // Route successfully matched! Render it. - const html = await ssr({ + + const html = await ssr(preloadedComponent, { astroConfig: config, - filePath: new URL(`./${route.component}`, config.projectRoot), + filePath, logging, mode: 'development', origin, pathname: rootRelativeUrl, route, - routeCache: routeCache, - viteServer: viteServer, + routeCache, + viteServer, }); writeHtmlResponse(res, statusCode, html); } catch (_err: any) { diff --git a/packages/astro/test/astro-get-static-paths.test.js b/packages/astro/test/astro-get-static-paths.test.js index 13f1aa4ac..cc46dfcd5 100644 --- a/packages/astro/test/astro-get-static-paths.test.js +++ b/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()', () => { - let fixture; - +describe('getStaticPaths - build calls', () => { 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); + }); +}); diff --git a/packages/astro/test/dev-routing.test.js b/packages/astro/test/dev-routing.test.js index f30ff1abf..9e1721c2f 100644 --- a/packages/astro/test/dev-routing.test.js +++ b/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); }); }); diff --git a/packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...test].astro b/packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...calledTwiceTest].astro similarity index 63% rename from packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...test].astro rename to packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...calledTwiceTest].astro index 438a31454..19800e1ae 100644 --- a/packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...test].astro +++ b/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: {test: 'b'}}, - {params: {test: 'c'}}, + {params: {calledTwiceTest: 'a'}}, + {params: {calledTwiceTest: 'b'}}, + {params: {calledTwiceTest: 'c'}}, ]; } const { params } = Astro.request; @@ -15,7 +15,7 @@ const { params } = Astro.request; - Page {params.test} + Page {params.calledTwiceTest} diff --git a/packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[...pizza].astro b/packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[...pizza].astro new file mode 100644 index 000000000..02ef8ef47 --- /dev/null +++ b/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 +--- + + + + + {pizza ?? 'The landing page'} + + +

Welcome to {pizza ?? 'The landing page'}

+ + \ No newline at end of file diff --git a/packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[cheese]-[topping].astro b/packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[cheese]-[topping].astro new file mode 100644 index 000000000..353805c5c --- /dev/null +++ b/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 +--- + + + + + {cheese} + + +

🍕 It's pizza time

+

{cheese}-{topping}

+ + \ No newline at end of file