diff options
| -rw-r--r-- | .changeset/young-rules-draw.md | 5 | ||||
| -rw-r--r-- | .eslintrc.cjs | 3 | ||||
| -rw-r--r-- | packages/astro/src/core/build/page-data.ts | 12 | ||||
| -rw-r--r-- | packages/astro/src/core/render/core.ts | 15 | ||||
| -rw-r--r-- | packages/astro/src/core/render/dev/index.ts | 6 | ||||
| -rw-r--r-- | packages/astro/src/vite-plugin-astro-server/index.ts | 78 | ||||
| -rw-r--r-- | packages/astro/test/astro-get-static-paths.test.js | 41 | ||||
| -rw-r--r-- | packages/astro/test/dev-routing.test.js | 16 | ||||
| -rw-r--r-- | packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...calledTwiceTest].astro (renamed from packages/astro/test/fixtures/astro-get-static-paths/src/pages/[...test].astro) | 8 | ||||
| -rw-r--r-- | packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[...pizza].astro | 22 | ||||
| -rw-r--r-- | packages/astro/test/fixtures/astro-get-static-paths/src/pages/pizza/[cheese]-[topping].astro | 21 |
11 files changed, 175 insertions, 52 deletions
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<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, }), }; diff --git a/packages/astro/src/core/render/core.ts b/packages/astro/src/core/render/core.ts index 9e837b3a3..32493ead1 100644 --- a/packages/astro/src/core/render/core.ts +++ b/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); 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<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 }); 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 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; <html> <head> - <title>Page {params.test}</title> + <title>Page {params.calledTwiceTest}</title> </head> <body></body> </html> 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 +--- +<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>
\ 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 +--- +<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>
\ No newline at end of file |