From fd17f4a40b83d14350dce691aeb79d87e8fcaf40 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Wed, 17 Jan 2024 10:28:18 -0300 Subject: [PATCH] Implement priority overrides for injected routes and redirects (#9439) * Implement priority overrides for injected routes and redirects * Fix ordering for route specificity * Don't mix rules on tests * Detailed collision detection * Add changeset * Remove TODO * Add comments to clarify default values * Update terminology * Revert unrelated changes * WIP * Refactor * Fix typo and typing * chore: default to legacy * chore: use experimental flag instead of option * fix: do not throw an error on collisions * chore: fix regression * chore: use `continue` instead of `return` * chore: fix tests but one * chore: Update test * chore: Change remaining new error to warning * chore: Test collision warnings * docs: Update docs of new config * docs: Improve changesets * chore: rename experimental flag * chore: update changeset and docs * Sarah editing pass * nit: Align Markdown table * defined definitions! Co-authored-by: Luiz Ferraz * added logging info to docs for experimental flag * Yan final boss review Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com> * chore: Update flag name in tests * chore: Update flag name in tests --------- Co-authored-by: Emanuele Stoppa Co-authored-by: Sarah Rainsberger Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com> --- .changeset/short-keys-bow.md | 16 + .changeset/smooth-cobras-help.md | 34 ++ packages/astro/src/@types/astro.ts | 40 ++ packages/astro/src/core/config/schema.ts | 5 + .../astro/src/core/routing/manifest/create.ts | 493 +++++++++++------- .../astro/test/units/routing/manifest.test.js | 362 ++++++++++++- 6 files changed, 756 insertions(+), 194 deletions(-) create mode 100644 .changeset/short-keys-bow.md create mode 100644 .changeset/smooth-cobras-help.md diff --git a/.changeset/short-keys-bow.md b/.changeset/short-keys-bow.md new file mode 100644 index 0000000000..bead5326ce --- /dev/null +++ b/.changeset/short-keys-bow.md @@ -0,0 +1,16 @@ +--- +"astro": patch +--- + +Updates [Astro's routing priority rules](https://docs.astro.build/en/core-concepts/routing/#route-priority-order) to prioritize the most specifically-defined routes. + +Now, routes with **more defined path segments** will take precedence over less specific routes. + +For example, `/blog/posts/[pid].astro` (3 path segments) takes precedence over `/blog/[...slug].astro` (2 path segments). This means that: + +- `/pages/blog/posts/[id].astro` will build routes of the form `/blog/posts/1` and `/blog/posts/a` +- `/pages/blog/[...slug].astro` will build routes of a variety of forms, including `blog/1` and `/blog/posts/1/a`, but will not build either of the previous routes. + +For a complete list of Astro's routing priority rules, please see the [routing guide](https://docs.astro.build/en/core-concepts/routing/#route-priority-order). This should not be a breaking change, but you may wish to inspect your built routes to ensure that your project is unaffected. + + diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md new file mode 100644 index 0000000000..80e3681242 --- /dev/null +++ b/.changeset/smooth-cobras-help.md @@ -0,0 +1,34 @@ +--- +'astro': minor +--- + +Adds an experimental flag `globalRoutePriority` to prioritize redirects and injected routes equally alongside file-based project routes, following the same [route priority order rules](https://docs.astro.build/en/core-concepts/routing/#route-priority-order) for all routes. + +```js +// astro.config.mjs +export default defineConfig({ + experimental: { + globalRoutePriority: true, + }, +}) +``` + +Enabling this feature ensures that all routes in your project follow the same, predictable route priority order rules. In particular, this avoids an issue where redirects or injected routes (e.g. from an integration) would always take precedence over local route definitions, making it impossible to override some routes locally. + +The following table shows which route builds certain page URLs when file-based routes, injected routes, and redirects are combined as shown below: + +- File-based route: `/blog/post/[pid]` +- File-based route: `/[page]` +- Injected route: `/blog/[...slug]` +- Redirect: `/blog/tags/[tag]` -> `/[tag]` +- Redirect: `/posts` -> `/blog` + +URLs are handled by the following routes: + +| Page | Current Behavior | Global Routing Priority Behavior | +|--------------------|----------------------------------|-------------------------------------| +| `/blog/tags/astro` | Injected route `/blog/[...slug]` | Redirect to `/tags/[tag]` | +| `/blog/post/0` | Injected route `/blog/[...slug]` | File-based route `/blog/post/[pid]` | +| `/posts` | File-based route `/[page]` | Redirect to `/blog` | + +In the event of route collisions, where two routes of equal route priority attempt to build the same URL, Astro will log a warning identifying the conflicting routes. diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index a7dbd14ac5..f8d0d5ab6c 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1631,6 +1631,36 @@ export interface AstroUserConfig { * See the [Prefetch Guide](https://docs.astro.build/en/guides/prefetch/) for more `prefetch` options and usage. */ clientPrerender?: boolean; + + /** + * @docs + * @name experimental.globalRoutePriority + * @type {boolean} + * @default `false` + * @version 4.2.0 + * @description + * + * Prioritizes redirects and injected routes equally alongside file-based project routes, following the same [route priority order rules](https://docs.astro.build/en/core-concepts/routing/#route-priority-order) for all routes. + * + * The following table shows which route builds certain page URLs when file-based routes, injected routes, and redirects are combined as shown below: + * - File-based route: `/blog/post/[pid]` + * - File-based route: `/[page]` + * - Injected route: `/blog/[...slug]` + * - Redirect: `/blog/tags/[tag]` -> `/[tag]` + * - Redirect: `/posts` -> `/blog` + * + * URLs are handled by the following routes: + * + * | Page | Current Behavior | Global Routing Priority Behavior | + * | ------------------ | -------------------------------- | ----------------------------------- | + * | `/blog/tags/astro` | Injected route `/blog/[...slug]` | Redirect to `/tags/[tag]` | + * | `/blog/post/0` | Injected route `/blog/[...slug]` | File-based route `/blog/post/[pid]` | + * | `/posts` | File-based route `/[page]` | Redirect to `/blog` | + * + * + * In the event of route collisions, where two routes of equal route priority attempt to build the same URL, Astro will log a warning identifying the conflicting routes. + */ + globalRoutePriority?: boolean; }; } @@ -1652,6 +1682,15 @@ export interface AstroUserConfig { */ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | 'page-ssr'; +/** + * IDs for different priorities of injected routes and redirects: + * - "normal": Merge with discovered file-based project routes, behaving the same as if the route + * was defined as a file in the project. + * - "legacy": Use the old ordering of routes. Inject routes will override any file-based project route, + * and redirects will be overridden by any project route on conflict. + */ +export type RoutePriorityOverride = 'normal' | 'legacy'; + export interface InjectedRoute { pattern: string; entrypoint: string; @@ -2496,6 +2535,7 @@ type RedirectConfig = | { status: ValidRedirectStatus; destination: string; + priority?: RoutePriorityOverride; }; export interface RouteData { diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 47ed11aaf6..655db8ed8f 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -61,6 +61,7 @@ const ASTRO_CONFIG_DEFAULTS = { optimizeHoistedScript: false, contentCollectionCache: false, clientPrerender: false, + globalRoutePriority: false, }, } satisfies AstroUserConfig & { server: { open: boolean } }; @@ -421,6 +422,10 @@ export const AstroConfigSchema = z.object({ .boolean() .optional() .default(ASTRO_CONFIG_DEFAULTS.experimental.clientPrerender), + globalRoutePriority: z + .boolean() + .optional() + .default(ASTRO_CONFIG_DEFAULTS.experimental.globalRoutePriority), }) .strict( `Invalid or outdated experimental feature.\nCheck for incorrect spelling or outdated Astro version.\nSee https://docs.astro.build/en/reference/configuration-reference/#experimental-flags for a list of all current experiments.` diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index e74df1eed4..2aeec9e4b6 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -1,7 +1,7 @@ import type { AstroConfig, AstroSettings, - InjectedRoute, + RoutePriorityOverride, ManifestData, RouteData, RoutePart, @@ -117,11 +117,6 @@ function getTrailingSlashPattern(addTrailingSlash: AstroConfig['trailingSlash']) return '\\/?$'; } -function isSpread(str: string) { - const spreadPattern = /\[\.{3}/g; - return spreadPattern.test(str); -} - function validateSegment(segment: string, file = '') { if (!file) file = segment; @@ -139,77 +134,64 @@ function validateSegment(segment: string, file = '') { } } -function comparator(a: Item, b: Item) { - if (a.isIndex !== b.isIndex) { - if (a.isIndex) return isSpread(a.file) ? 1 : -1; - - return isSpread(b.file) ? -1 : 1; +/** + * Comparator for sorting routes in resolution order. + * + * The routes are sorted in by the following rules in order, following the first rule that + * applies: + * - More specific routes are sorted before less specific routes. Here, "specific" means + * the number of segments in the route, so a parent route is always sorted after its children. + * For example, `/foo/bar` is sorted before `/foo`. + * - Static routes are sorted before dynamic routes. + * For example, `/foo/bar` is sorted before `/foo/[bar]`. + * - Dynamic routes with single parameters are sorted before dynamic routes with rest parameters. + * For example, `/foo/[bar]` is sorted before `/foo/[...bar]`. + * - Prerendered routes are sorted before non-prerendered routes. + * - Endpoints are sorted before pages. + * For example, a file `/foo.ts` is sorted before `/bar.astro`. + * - If both routes are equal regarding all previosu conditions, they are sorted alphabetically. + * For example, `/bar` is sorted before `/foo`. + * The definition of "alphabetically" is dependent on the default locale of the running system. + */ +function routeComparator(a: RouteData, b: RouteData) { + // Sort more specific routes before less specific routes + if (a.segments.length !== b.segments.length) { + return a.segments.length > b.segments.length ? -1 : 1; } - const max = Math.max(a.parts.length, b.parts.length); + const aIsStatic = a.segments.every((segment) => + segment.every((part) => !part.dynamic && !part.spread) + ); + const bIsStatic = b.segments.every((segment) => + segment.every((part) => !part.dynamic && !part.spread) + ); - for (let i = 0; i < max; i += 1) { - const aSubPart = a.parts[i]; - const bSubPart = b.parts[i]; - - if (!aSubPart) return 1; // b is more specific, so goes first - if (!bSubPart) return -1; - - // if spread && index, order later - if (aSubPart.spread && bSubPart.spread) { - return a.isIndex ? 1 : -1; - } - - // If one is ...spread order it later - if (aSubPart.spread !== bSubPart.spread) return aSubPart.spread ? 1 : -1; - - if (aSubPart.dynamic !== bSubPart.dynamic) { - return aSubPart.dynamic ? 1 : -1; - } - - if (!aSubPart.dynamic && aSubPart.content !== bSubPart.content) { - return ( - bSubPart.content.length - aSubPart.content.length || - (aSubPart.content < bSubPart.content ? -1 : 1) - ); - } + // Sort static routes before dynamic routes + if (aIsStatic !== bIsStatic) { + return aIsStatic ? -1 : 1; } - // endpoints are prioritized over pages - if (a.isPage !== b.isPage) { - return a.isPage ? 1 : -1; + const aHasSpread = a.segments.some((segment) => segment.some((part) => part.spread)); + const bHasSpread = b.segments.some((segment) => segment.some((part) => part.spread)); + + // Sort dynamic routes with rest parameters after dynamic routes with single parameters + // (also after static, but that is already covered by the previous condition) + if (aHasSpread !== bHasSpread) { + return aHasSpread ? 1 : -1; } - // otherwise sort alphabetically - return a.file < b.file ? -1 : 1; -} - -function injectedRouteToItem( - { config, cwd }: { config: AstroConfig; cwd?: string }, - { pattern, entrypoint }: InjectedRoute -): Item { - let resolved: string; - try { - resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(config.root)] }); - } catch (e) { - resolved = fileURLToPath(new URL(entrypoint, config.root)); + // Sort prerendered routes before non-prerendered routes + if (a.prerender !== b.prerender) { + return a.prerender ? -1 : 1; } - const ext = path.extname(pattern); + // Sort endpoints before pages + if ((a.type === 'endpoint') !== (b.type === 'endpoint')) { + return a.type === 'endpoint' ? -1 : 1; + } - 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), - }; + // Sort alphabetically + return a.route.localeCompare(b.route); } export interface CreateRouteManifestParams { @@ -221,11 +203,10 @@ export interface CreateRouteManifestParams { fsMod?: typeof nodeFs; } -/** Create manifest of all static routes */ -export function createRouteManifest( +function createFileBasedRoutes( { settings, cwd, fsMod }: CreateRouteManifestParams, logger: Logger -): ManifestData { +): RouteData[] { const components: string[] = []; const routes: RouteData[] = []; const validPageExtensions = new Set([ @@ -244,19 +225,19 @@ export function createRouteManifest( parentParams: string[] ) { let items: Item[] = []; - fs.readdirSync(dir).forEach((basename) => { + const files = fs.readdirSync(dir); + for (const basename of files) { const resolved = path.join(dir, basename); const file = slash(path.relative(cwd || fileURLToPath(settings.config.root), resolved)); const isDir = fs.statSync(resolved).isDirectory(); const ext = path.extname(basename); const name = ext ? basename.slice(0, -ext.length) : basename; - if (name[0] === '_') { - return; + continue; } if (basename[0] === '.' && basename !== '.well-known') { - return; + continue; } // filter out "foo.astro_tmp" files, etc if (!isDir && !validPageExtensions.has(ext) && !validEndpointExtensions.has(ext)) { @@ -267,7 +248,7 @@ export function createRouteManifest( )} found. Prefix filename with an underscore (\`_\`) to ignore.` ); - return; + continue; } const segment = isDir ? basename : name; validateSegment(segment, file); @@ -287,10 +268,9 @@ export function createRouteManifest( isPage, routeSuffix, }); - }); - items = items.sort(comparator); + } - items.forEach((item) => { + for (const item of items) { const segments = parentSegments.slice(); if (item.isIndex) { @@ -352,7 +332,7 @@ export function createRouteManifest( fallbackRoutes: [], }); } - }); + } } const { config } = settings; @@ -365,73 +345,92 @@ export function createRouteManifest( logger.warn(null, `Missing pages directory: ${pagesDirRootRelative}`); } - settings.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, prerender: prerenderInjected }) => { - let resolved: string; - try { - resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(config.root)] }); - } catch (e) { - resolved = fileURLToPath(new URL(entrypoint, config.root)); - } - const component = slash(path.relative(cwd || fileURLToPath(config.root), resolved)); + return routes; +} - const segments = removeLeadingForwardSlash(name) - .split(path.posix.sep) - .filter(Boolean) - .map((s: string) => { - validateSegment(s); - return getParts(s, component); - }); +type PrioritizedRoutesData = Record; - const type = resolved.endsWith('.astro') ? 'page' : 'endpoint'; - const isPage = type === 'page'; - const trailingSlash = isPage ? config.trailingSlash : 'never'; +function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): PrioritizedRoutesData { + const { config } = settings; + const prerender = getPrerenderDefault(config); - const pattern = getPattern(segments, settings.config, trailingSlash); - const generate = getRouteGenerator(segments, trailingSlash); - const pathname = segments.every((segment) => segment.length === 1 && !segment[0].dynamic) - ? `/${segments.map((segment) => segment[0].content).join('/')}` - : null; - const params = segments - .flat() - .filter((p) => p.dynamic) - .map((p) => p.content); - const route = `/${segments - .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) - .join('/')}`.toLowerCase(); + const routes: PrioritizedRoutesData = { + normal: [], + legacy: [], + }; - const collision = routes.find(({ route: r }) => r === route); - if (collision) { - throw new Error( - `An integration attempted to inject a route that is already used in your project: "${route}" at "${component}". \nThis route collides with: "${collision.component}".` - ); - } + const priority = computeRoutePriority(config); - // 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, - segments, - params, - component, - generate, - pathname: pathname || void 0, - prerender: prerenderInjected ?? prerender, - fallbackRoutes: [], + for (const injectedRoute of settings.injectedRoutes) { + const { pattern: name, entrypoint, prerender: prerenderInjected } = injectedRoute; + let resolved: string; + try { + resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(config.root)] }); + } catch (e) { + resolved = fileURLToPath(new URL(entrypoint, config.root)); + } + const component = slash(path.relative(cwd || fileURLToPath(config.root), resolved)); + + const segments = removeLeadingForwardSlash(name) + .split(path.posix.sep) + .filter(Boolean) + .map((s: string) => { + validateSegment(s); + return getParts(s, component); }); + + const type = resolved.endsWith('.astro') ? 'page' : 'endpoint'; + const isPage = type === 'page'; + const trailingSlash = isPage ? config.trailingSlash : 'never'; + + const pattern = getPattern(segments, settings.config, trailingSlash); + const generate = getRouteGenerator(segments, trailingSlash); + const pathname = segments.every((segment) => segment.length === 1 && !segment[0].dynamic) + ? `/${segments.map((segment) => segment[0].content).join('/')}` + : null; + const params = segments + .flat() + .filter((p) => p.dynamic) + .map((p) => p.content); + const route = `/${segments + .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) + .join('/')}`.toLowerCase(); + + routes[priority].push({ + type, + route, + pattern, + segments, + params, + component, + generate, + pathname: pathname || void 0, + prerender: prerenderInjected ?? prerender, + fallbackRoutes: [], }); + } - Object.entries(settings.config.redirects).forEach(([from, to]) => { - const trailingSlash = config.trailingSlash; + return routes; +} +/** + * Create route data for all configured redirects. + */ +function createRedirectRoutes( + { settings }: CreateRouteManifestParams, + routeMap: Map, + logger: Logger +): PrioritizedRoutesData { + const { config } = settings; + const trailingSlash = config.trailingSlash; + + const routes: PrioritizedRoutesData = { + normal: [], + legacy: [], + }; + + const priority = computeRoutePriority(settings.config); + for (const [from, to] of Object.entries(settings.config.redirects)) { const segments = removeLeadingForwardSlash(from) .split(path.posix.sep) .filter(Boolean) @@ -453,22 +452,21 @@ export function createRouteManifest( .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - { - let destination: string; - if (typeof to === 'string') { - destination = to; - } else { - destination = to.destination; - } - if (/^https?:\/\//.test(destination)) { - logger.warn( - 'redirects', - `Redirecting to an external URL is not officially supported: ${from} -> ${destination}` - ); - } + let destination: string; + if (typeof to === 'string') { + destination = to; + } else { + destination = to.destination; } - const routeData: RouteData = { + if (/^https?:\/\//.test(destination)) { + logger.warn( + 'redirects', + `Redirecting to an external URL is not officially supported: ${from} -> ${destination}` + ); + } + + routes[priority].push({ type: 'redirect', route, pattern, @@ -479,40 +477,170 @@ export function createRouteManifest( pathname: pathname || void 0, prerender: false, redirect: to, - redirectRoute: routes.find((r) => { - if (typeof to === 'object') { - return r.route === to.destination; - } else { - return r.route === to; - } - }), + redirectRoute: routeMap.get(destination), fallbackRoutes: [], - }; + }); + } - const lastSegmentIsDynamic = (r: RouteData) => !!r.segments.at(-1)?.at(-1)?.dynamic; + return routes; +} - const redirBase = path.posix.dirname(route); - const dynamicRedir = lastSegmentIsDynamic(routeData); - let i = 0; - for (const existingRoute of routes) { - // An exact match, prefer the page/endpoint. This matches hosts. - if (existingRoute.route === route) { - routes.splice(i + 1, 0, routeData); - return; - } +/** + * Checks whether a route segment is static. + */ +function isStaticSegment(segment: RoutePart[]) { + return segment.every((part) => !part.dynamic && !part.spread); +} - // If the existing route is dynamic, prefer the static redirect. - const base = path.posix.dirname(existingRoute.route); - if (base === redirBase && !dynamicRedir && lastSegmentIsDynamic(existingRoute)) { - routes.splice(i, 0, routeData); - return; - } - i++; +/** + * Checks whether two route segments are semantically equivalent. + * + * Two segments are equivalent if they would match the same paths. This happens when: + * - They have the same length. + * - Each part in the same position is either: + * - Both static and with the same content (e.g. `/foo` and `/foo`). + * - Both dynamic, regardless of the content (e.g. `/[bar]` and `/[baz]`). + * - Both rest parameters, regardless of the content (e.g. `/[...bar]` and `/[...baz]`). + */ +function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[]) { + if (segmentA.length !== segmentB.length) { + return false; + } + + for (const [index, partA] of segmentA.entries()) { + // Safe to use the index of one segment for the other because the segments have the same length + const partB = segmentB[index]; + + if (partA.dynamic !== partB.dynamic || partA.spread !== partB.spread) { + return false; } - // Didn't find a good place, insert last - routes.push(routeData); - }); + // Only compare the content on non-dynamic segments + // `/[bar]` and `/[baz]` are effectively the same route, + // only bound to a different path parameter. + if (!partA.dynamic && partA.content !== partB.content) { + return false; + } + } + + return true; +} + +/** + * Check whether two are sure to collide in clearly unintended ways report appropriately. + * + * Fallback routes are never considered to collide with any other route. + * Routes that may collide depending on the parameters returned by their `getStaticPaths` + * are not reported as collisions at this stage. + * + * Two routes are guarantted to collide in the following scenarios: + * - Both are the exact same static route. + * For example, `/foo` from an injected route and `/foo` from a file in the project. + * - Both are non-prerendered dynamic routes with equal static parts in matching positions + * and dynamic parts of same type in the same positions. + * For example, `/foo/[bar]` and `/foo/[baz]` or `/foo/[...bar]` and `/foo/[...baz]` + * but not `/foo/[bar]` and `/foo/[...baz]`. + */ +function detectRouteCollision(a: RouteData, b: RouteData, config: AstroConfig, logger: Logger) { + if (a.type === 'fallback' || b.type === 'fallback') { + // If either route is a fallback route, they don't collide. + // Fallbacks are always added below other routes exactly to avoid collisions. + return; + } + + if ( + a.route === b.route && + a.segments.every(isStaticSegment) && + b.segments.every(isStaticSegment) + ) { + // If both routes are the same and completely static they are guaranteed to collide + // such that one of them will never be matched. + logger.warn( + 'router', + `The route "${a.route}" is defined in both "${a.component}" and "${b.component}". A static route cannot be defined more than once.` + ); + logger.warn( + 'router', + 'A collision will result in an hard error in following versions of Astro.' + ); + return; + } + + if (a.prerender || b.prerender) { + // If either route is prerendered, it is impossible to know if they collide + // at this stage because it depends on the parameters returned by `getStaticPaths`. + return; + } + + if (a.segments.length !== b.segments.length) { + // If the routes have different number of segments, they cannot perfectly overlap + // each other, so a collision is either not guaranteed or may be intentional. + return; + } + + // Routes have the same number of segments, can use either. + const segmentCount = a.segments.length; + + for (let index = 0; index < segmentCount; index++) { + const segmentA = a.segments[index]; + const segmentB = b.segments[index]; + + if (!isSemanticallyEqualSegment(segmentA, segmentB)) { + // If any segment is not semantically equal between the routes + // it is not certain that the routes collide. + return; + } + } + + // Both routes are guaranteed to collide such that one will never be matched. + logger.warn( + 'router', + `The route "${a.route}" is defined in both "${a.component}" and "${b.component}" using SSR mode. A dynamic SSR route cannot be defined more than once.` + ); + logger.warn('router', 'A collision will result in an hard error in following versions of Astro.'); +} + +/** Create manifest of all static routes */ +export function createRouteManifest( + params: CreateRouteManifestParams, + logger: Logger +): ManifestData { + const { settings } = params; + const { config } = settings; + // Create a map of all routes so redirects can refer to any route + const routeMap = new Map(); + + const fileBasedRoutes = createFileBasedRoutes(params, logger); + for (const route of fileBasedRoutes) { + routeMap.set(route.route, route); + } + + const injectedRoutes = createInjectedRoutes(params); + for (const [, routes] of Object.entries(injectedRoutes)) { + for (const route of routes) { + routeMap.set(route.route, route); + } + } + + const redirectRoutes = createRedirectRoutes(params, routeMap, logger); + + const routes: RouteData[] = [ + ...injectedRoutes['legacy'].sort(routeComparator), + ...[...fileBasedRoutes, ...injectedRoutes['normal'], ...redirectRoutes['normal']].sort( + routeComparator + ), + ...redirectRoutes['legacy'].sort(routeComparator), + ]; + + // Report route collisions + if (config.experimental.globalRoutePriority) { + for (const [index, higherRoute] of routes.entries()) { + for (const lowerRoute of routes.slice(index + 1)) { + detectRouteCollision(higherRoute, lowerRoute, config, logger); + } + } + } + const i18n = settings.config.i18n; if (i18n) { // First we check if the user doesn't have an index page. @@ -704,3 +832,10 @@ export function createRouteManifest( routes, }; } + +function computeRoutePriority(config: AstroConfig): RoutePriorityOverride { + if (config.experimental.globalRoutePriority) { + return 'normal'; + } + return 'legacy'; +} diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index a7014d8531..5da15fad09 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -3,9 +3,29 @@ import { expect } from 'chai'; import { fileURLToPath } from 'node:url'; import { createRouteManifest } from '../../../dist/core/routing/manifest/create.js'; import { createBasicSettings, createFs, defaultLogger } from '../test-utils.js'; +import { Logger } from '../../../dist/core/logger/core.js'; const root = new URL('../../fixtures/alias/', import.meta.url); +function getManifestRoutes(manifest) { + return manifest.routes.map((route) => ({ + type: route.type, + route: route.route, + })); +} + +function getLogger() { + const logs = []; + + return { + logger: new Logger({ + dest: {write: (msg) => logs.push(msg)}, + level: 'debug', + }), + logs, + } +} + describe('routing - createRouteManifest', () => { it('using trailingSlash: "never" does not match the index route when it contains a trailing slash', async () => { const fs = createFs( @@ -29,6 +49,223 @@ describe('routing - createRouteManifest', () => { expect(pattern.test('/')).to.equal(false); }); + it('endpoint routes are sorted before page routes', async () => { + const fs = createFs( + { + '/src/pages/contact-me.astro': `

test

`, + '/src/pages/sendContact.ts': `

test

`, + }, + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + base: '/search', + trailingSlash: 'never', + experimental: { + globalRoutePriority: true, + }, + }); + + settings.injectedRoutes = [ + { + pattern: '/about', + entrypoint: '@lib/legacy/static.astro', + }, + { + pattern: '/api', + entrypoint: '@lib/legacy/static.ts', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/api', + type: 'endpoint', + }, + { + route: '/sendcontact', + type: 'endpoint', + }, + { + route: '/about', + type: 'page', + }, + { + route: '/contact-me', + type: 'page', + }, + ]); + }); + + it('injected routes are sorted in legacy mode above filesystem routes', async () => { + const fs = createFs( + { + '/src/pages/index.astro': `

test

`, + '/src/pages/blog/[...slug].astro': `

test

`, + }, + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + output: 'server', + base: '/search', + trailingSlash: 'never', + }); + + settings.injectedRoutes = [ + { + pattern: '/contributing', + entrypoint: '@lib/legacy/static.astro', + }, + { + pattern: '/[...slug]', + entrypoint: '@lib/legacy/dynamic.astro', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/contributing', + type: 'page', + }, + { + route: '/[...slug]', + type: 'page', + }, + { + route: '/blog/[...slug]', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + ]); + }); + + it('injected routes are sorted alongside filesystem routes', async () => { + const fs = createFs( + { + '/src/pages/index.astro': `

test

`, + '/src/pages/blog/[...slug].astro': `

test

`, + }, + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + output: 'server', + base: '/search', + trailingSlash: 'never', + integrations: [ + { + name: '@test', + hooks: { + 'astro:config:setup': ({ injectRoute }) => {}, + }, + }, + ], + experimental: { + globalRoutePriority: true, + }, + }); + + settings.injectedRoutes = [ + { + pattern: '/contributing', + entrypoint: '@lib/legacy/static.astro', + }, + { + pattern: '/[...slug]', + entrypoint: '@lib/legacy/dynamic.astro', + priority: 'normal', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/[...slug]', + type: 'page', + }, + { + route: '/contributing', + type: 'page', + }, + { + route: '/[...slug]', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + ]); + }); + + it('redirects are sorted in legacy mode below the filesystem routes', async () => { + const fs = createFs( + { + '/src/pages/index.astro': `

test

`, + '/src/pages/blog/contributing.astro': `

test

`, + }, + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + output: 'server', + base: '/search', + trailingSlash: 'never', + redirects: { + '/blog/[...slug]': '/', + '/blog/about': { + status: 302, + destination: '/another', + }, + }, + }); + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/contributing', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + { + route: '/blog/about', + type: 'redirect', + }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, + ]); + }); + it('redirects are sorted alongside the filesystem routes', async () => { const fs = createFs( { @@ -39,11 +276,21 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', redirects: { - '/blog/[...slug]': '/', - '/blog/contributing': '/another', + '/blog/[...slug]': { + status: 302, + destination: '/', + }, + '/blog/about': { + status: 302, + destination: '/another', + }, + }, + experimental: { + globalRoutePriority: true, }, }); const manifest = createRouteManifest({ @@ -52,34 +299,119 @@ describe('routing - createRouteManifest', () => { fsMod: fs, }); - expect(manifest.routes[1].route).to.equal('/blog/contributing'); - expect(manifest.routes[1].type).to.equal('page'); - expect(manifest.routes[3].route).to.equal('/blog/[...slug]'); + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/about', + type: 'redirect', + }, + { + route: '/blog/contributing', + type: 'page', + }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, + { + route: '/', + type: 'page', + }, + ]); }); - it('static redirect route is prioritized over dynamic file route', async () => { + it('report colliding static routes', async () => { const fs = createFs( { - '/src/pages/[...slug].astro': `

test

`, + '/src/pages/contributing.astro': `

test

`, }, root ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', + base: '/search', trailingSlash: 'never', - redirects: { - '/foo': '/bar', - }, + integrations: [], + experimental: { + globalRoutePriority: true, + } }); - const manifest = createRouteManifest( + + settings.injectedRoutes = [ { + pattern: '/contributing', + entrypoint: '@lib/legacy/static.astro', + }, + ]; + + const manifestOptions = { cwd: fileURLToPath(root), settings, fsMod: fs, - }, - defaultLogger - ); + }; - expect(manifest.routes[0].route).to.equal('/foo'); + const {logger, logs} = getLogger(); + + createRouteManifest(manifestOptions, logger); + + expect(logs).to.deep.equal([ + { + "label": "router", + "level": "warn", + "message": 'The route "/contributing" is defined in both "src/pages/contributing.astro" and "@lib/legacy/static.astro". A static route cannot be defined more than once.', + "newLine": true, + }, + { + "label": "router", + "level": "warn", + "message": "A collision will result in an hard error in following versions of Astro.", + "newLine": true, + }, + ]); + }); + + it('report colliding SSR dynamic routes', async () => { + const fs = createFs( + { + '/src/pages/[foo].astro': `

test

`, + '/src/pages/[bar].astro': `

test

`, + }, + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + output: 'server', + base: '/search', + trailingSlash: 'never', + integrations: [], + experimental: { + globalRoutePriority: true, + } + }); + + const manifestOptions = { + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }; + + const {logger, logs} = getLogger(); + + createRouteManifest(manifestOptions, logger); + + expect(logs).to.deep.equal([ + { + "label": "router", + "level": "warn", + "message": 'The route "/[bar]" is defined in both "src/pages/[bar].astro" and "src/pages/[foo].astro" using SSR mode. A dynamic SSR route cannot be defined more than once.', + "newLine": true, + }, + { + "label": "router", + "level": "warn", + "message": "A collision will result in an hard error in following versions of Astro.", + "newLine": true, + }, + ]); }); });