diff --git a/.changeset/gold-olives-cover.md b/.changeset/gold-olives-cover.md new file mode 100644 index 000000000..783629343 --- /dev/null +++ b/.changeset/gold-olives-cover.md @@ -0,0 +1,5 @@ +--- +'astro': patch +--- + +Fixes a couple routing bugs that could lead to routing differences in `dev` vs. `build` when using multiple dynamic routes diff --git a/packages/astro/src/core/routing/index.ts b/packages/astro/src/core/routing/index.ts index 84ec1f66e..13b23d78b 100644 --- a/packages/astro/src/core/routing/index.ts +++ b/packages/astro/src/core/routing/index.ts @@ -1,5 +1,5 @@ export { createRouteManifest } from './manifest/create.js'; export { deserializeRouteData, serializeRouteData } from './manifest/serialization.js'; -export { matchRoute } from './match.js'; +export { matchRoute, matchAllRoutes } from './match.js'; export { getParams } from './params.js'; export { validateGetStaticPathsModule, validateGetStaticPathsResult } from './validation.js'; diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index ba40f3b1b..bb718dd45 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -1,4 +1,4 @@ -import type { AstroConfig, ManifestData, RouteData, RoutePart } from '../../../@types/astro'; +import type { AstroConfig, InjectedRoute, ManifestData, RouteData, RoutePart } from '../../../@types/astro'; import type { LogOptions } from '../../logger/core'; import fs from 'fs'; @@ -159,6 +159,29 @@ function comparator(a: Item, b: Item) { return a.file < b.file ? -1 : 1; } +function injectedRouteToItem( + { config, cwd }: { config: AstroConfig; cwd?: string }, + { pattern, entryPoint }: InjectedRoute +): Item { + const resolved = require.resolve(entryPoint, { paths: [cwd || fileURLToPath(config.root)] }); + + const ext = path.extname(pattern); + + const type = resolved.endsWith('.astro') ? 'page' : 'endpoint'; + const isPage = type === 'page'; + + return { + basename: pattern, + ext, + parts: getParts(pattern, resolved), + file: resolved, + isDir: false, + isIndex: true, + isPage, + routeSuffix: pattern.slice(pattern.indexOf('.'), -ext.length), + } +} + /** Create manifest of all static routes */ export function createRouteManifest( { config, cwd }: { config: AstroConfig; cwd?: string }, @@ -288,7 +311,14 @@ export function createRouteManifest( warn(logging, 'astro', `Missing pages directory: ${pagesDirRootRelative}`); } - config?._ctx?.injectedRoutes?.forEach(({ pattern: name, entryPoint }) => { + config?._ctx?.injectedRoutes?.sort((a, b) => + // sort injected routes in the same way as user-defined routes + comparator( + injectedRouteToItem({ config, cwd }, a), + injectedRouteToItem({ config, cwd}, b) + )) + .reverse() // prepend to the routes array from lowest to highest priority + .forEach(({ pattern: name, entryPoint }) => { const resolved = require.resolve(entryPoint, { paths: [cwd || fileURLToPath(config.root)] }); const component = slash(path.relative(cwd || fileURLToPath(config.root), resolved)); @@ -336,7 +366,10 @@ export function createRouteManifest( ); } - routes.push({ + // the routes array was already sorted by priority, + // pushing to the front of the list ensure that injected routes + // are given priority over all user-provided routes + routes.unshift({ type, route, pattern, diff --git a/packages/astro/src/core/routing/match.ts b/packages/astro/src/core/routing/match.ts index d2c2a8a54..bc4987926 100644 --- a/packages/astro/src/core/routing/match.ts +++ b/packages/astro/src/core/routing/match.ts @@ -4,3 +4,8 @@ import type { ManifestData, RouteData } from '../../@types/astro'; export function matchRoute(pathname: string, manifest: ManifestData): RouteData | undefined { return manifest.routes.find((route) => route.pattern.test(pathname)); } + +/** Finds all matching routes from pathname */ +export function matchAllRoutes(pathname: string, manifest: ManifestData): RouteData[] { + return manifest.routes.filter((route) => route.pattern.test(pathname)); +} diff --git a/packages/astro/src/vite-plugin-astro-server/index.ts b/packages/astro/src/vite-plugin-astro-server/index.ts index 3449b7d3a..a77ca486d 100644 --- a/packages/astro/src/vite-plugin-astro-server/index.ts +++ b/packages/astro/src/vite-plugin-astro-server/index.ts @@ -19,7 +19,7 @@ import { getParamsAndProps, GetParamsAndPropsError } from '../core/render/core.j import { preload, ssr } from '../core/render/dev/index.js'; import { RouteCache } from '../core/render/route-cache.js'; import { createRequest } from '../core/request.js'; -import { createRouteManifest, matchRoute } from '../core/routing/index.js'; +import { createRouteManifest, matchAllRoutes, matchRoute } from '../core/routing/index.js'; import { createSafeError, resolvePages } from '../core/util.js'; import notFoundTemplate, { subpathNotUsedTemplate } from '../template/4xx.js'; @@ -248,26 +248,81 @@ async function handleRequest( clientAddress: buildingToSSR ? req.socket.remoteAddress : undefined, }); - let filePath: URL | undefined; - try { - // Attempt to match the URL to a valid page route. - // If that fails, switch the response to a 404 response. - let route = matchRoute(pathname, manifest); - const statusCode = route ? 200 : 404; + async function matchRoute() { + const matches = matchAllRoutes(pathname, manifest); - if (!route) { - log404(logging, pathname); - const custom404 = getCustom404Route(config, manifest); - if (custom404) { - route = custom404; - } else { - return handle404Response(origin, config, req, res); + if (config.output === 'server' && matches.length > 1) { + throw new Error(`Found multiple matching routes for "${pathname}"! When using \`output: 'server'\`, only one route in \`src/pages\` can match a given URL. Found: + +${ + matches.map(({ component }) => `- ${component}`).join('\n') +} +`); + } + + for await (const maybeRoute of matches) { + const filePath = new URL(`./${maybeRoute.component}`, config.root); + 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: maybeRoute, + routeCache, + pathname: pathname, + logging, + ssr: config.output === 'server', + }); + + if (paramsAndPropsRes !== GetParamsAndPropsError.NoMatchingStaticPath) { + return { + route: maybeRoute, + filePath, + preloadedComponent, + mod, + } } } - filePath = new URL(`./${route.component}`, config.root); - const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer }); - const [, mod] = preloadedComponent; + if (matches.length) { + warn( + logging, + 'getStaticPaths', + `Route pattern matched, but no matching static path found. (${pathname})` + ); + } + + log404(logging, pathname); + const custom404 = getCustom404Route(config, manifest); + + if (custom404) { + const filePath = new URL(`./${custom404.component}`, config.root); + const preloadedComponent = await preload({ astroConfig: config, filePath, viteServer }); + const [, mod] = preloadedComponent; + + return { + route: custom404, + filePath, + preloadedComponent, + mod + }; + } + + return undefined; + } + + let filePath: URL | undefined; + try { + const matchedRoute = await matchRoute(); + + if (!matchedRoute) { + return handle404Response(origin, config, req, res); + } + + const { route, preloadedComponent, mod } = matchedRoute; + filePath = matchedRoute.filePath; + // attempt to get static paths // if this fails, we have a bad URL match! const paramsAndPropsRes = await getParamsAndProps({ @@ -278,38 +333,6 @@ async function handleRequest( logging, ssr: config.output === 'server', }); - 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.root); - const preloadedCompCustom404 = await preload({ - astroConfig: config, - filePath: filePathCustom404, - viteServer, - }); - const result = await ssr(preloadedCompCustom404, { - astroConfig: config, - filePath: filePathCustom404, - logging, - mode: 'development', - origin, - pathname: pathname, - request, - route: routeCustom404, - routeCache, - viteServer, - }); - return await writeSSRResult(result, res); - } else { - return handle404Response(origin, config, req, res); - } - } const options: SSROptions = { astroConfig: config, diff --git a/packages/astro/test/astro-get-static-paths.test.js b/packages/astro/test/astro-get-static-paths.test.js index ffb3660d9..6d98446a1 100644 --- a/packages/astro/test/astro-get-static-paths.test.js +++ b/packages/astro/test/astro-get-static-paths.test.js @@ -4,6 +4,9 @@ import * as cheerio from 'cheerio'; describe('getStaticPaths - build calls', () => { before(async () => { + // reset the flag used by [...calledTwiceTest].astro between each test + globalThis.isCalledOnce = false; + const fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/', site: 'https://mysite.dev/', @@ -11,17 +14,49 @@ describe('getStaticPaths - build calls', () => { }); 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 - dev calls', () => { + let fixture; + let devServer; + + before(async () => { + // reset the flag used by [...calledTwiceTest].astro between each test + globalThis.isCalledOnce = false; + + fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' }); + devServer = await fixture.startDevServer(); + }); + + after(async () => { + devServer.stop(); + }); + + it('only calls getStaticPaths once', async () => { + let res = await fixture.fetch('/a'); + expect(res.status).to.equal(200); + + res = await fixture.fetch('/b'); + expect(res.status).to.equal(200); + + res = await fixture.fetch('/c'); + expect(res.status).to.equal(200); + }); +}); + describe('getStaticPaths - 404 behavior', () => { let fixture; let devServer; before(async () => { + // reset the flag used by [...calledTwiceTest].astro between each test + globalThis.isCalledOnce = false; + fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' }); devServer = await fixture.startDevServer(); }); @@ -55,6 +90,9 @@ describe('getStaticPaths - route params type validation', () => { let fixture, devServer; before(async () => { + // reset the flag used by [...calledTwiceTest].astro between each test + globalThis.isCalledOnce = false; + fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/' }); devServer = await fixture.startDevServer(); }); @@ -81,6 +119,9 @@ describe('getStaticPaths - numeric route params', () => { let devServer; before(async () => { + // reset the flag used by [...calledTwiceTest].astro between each test + globalThis.isCalledOnce = false; + fixture = await loadFixture({ root: './fixtures/astro-get-static-paths/', site: 'https://mysite.dev/', diff --git a/packages/astro/test/fixtures/routing-priority/astro.config.mjs b/packages/astro/test/fixtures/routing-priority/astro.config.mjs index 882e6515a..a5103a7f4 100644 --- a/packages/astro/test/fixtures/routing-priority/astro.config.mjs +++ b/packages/astro/test/fixtures/routing-priority/astro.config.mjs @@ -1,4 +1,7 @@ import { defineConfig } from 'astro/config'; +import integration from './integration.mjs'; // https://astro.build/config -export default defineConfig({}); +export default defineConfig({ + integrations: [integration()] +}); diff --git a/packages/astro/test/fixtures/routing-priority/integration.mjs b/packages/astro/test/fixtures/routing-priority/integration.mjs new file mode 100644 index 000000000..213812790 --- /dev/null +++ b/packages/astro/test/fixtures/routing-priority/integration.mjs @@ -0,0 +1,21 @@ +export default function() { + return { + name: '@astrojs/test-integration', + hooks: { + 'astro:config:setup': ({ injectRoute }) => { + injectRoute({ + pattern: '/injected', + entryPoint: './src/to-inject.astro' + }); + injectRoute({ + pattern: '/_injected', + entryPoint: './src/_to-inject.astro' + }); + injectRoute({ + pattern: '/[id]', + entryPoint: './src/[id].astro' + }); + } + } + } +} diff --git a/packages/astro/test/fixtures/routing-priority/src/[id].astro b/packages/astro/test/fixtures/routing-priority/src/[id].astro new file mode 100644 index 000000000..af235c9a8 --- /dev/null +++ b/packages/astro/test/fixtures/routing-priority/src/[id].astro @@ -0,0 +1,21 @@ +--- +export async function getStaticPaths() { + return [ + { params: { id: 'injected-1' } }, + { params: { id: 'injected-2' } } + ]; +} + +const { id } = Astro.params; +--- + +
+ + +{id}
+ + diff --git a/packages/astro/test/fixtures/routing-priority/src/_to-inject.astro b/packages/astro/test/fixtures/routing-priority/src/_to-inject.astro new file mode 100644 index 000000000..4c5b6428f --- /dev/null +++ b/packages/astro/test/fixtures/routing-priority/src/_to-inject.astro @@ -0,0 +1,12 @@ +--- +--- + + + + +{Astro.params.page}
+ + + diff --git a/packages/astro/test/fixtures/routing-priority/src/pages/[slug].astro b/packages/astro/test/fixtures/routing-priority/src/pages/[slug].astro new file mode 100644 index 000000000..409942844 --- /dev/null +++ b/packages/astro/test/fixtures/routing-priority/src/pages/[slug].astro @@ -0,0 +1,27 @@ +--- + export async function getStaticPaths() { + return [ + { + params: { slug: "slug-1" }, + }, + { + params: { slug: "slug-2" }, + } + ] + } +--- + + + + + + +{Astro.params.slug}
+ + + diff --git a/packages/astro/test/fixtures/routing-priority/src/to-inject.astro b/packages/astro/test/fixtures/routing-priority/src/to-inject.astro new file mode 100644 index 000000000..4c5b6428f --- /dev/null +++ b/packages/astro/test/fixtures/routing-priority/src/to-inject.astro @@ -0,0 +1,12 @@ +--- +--- + + + + +