Fix regression in the routing priority of index routes (#9726)

* fix: Fix regression in the routing priority of index routes * chore: Add changeset * Update .changeset/smart-rules-train.md Co-authored-by: Florian Lefebvre <[email protected]> --------- Co-authored-by: Matthew Phillips <[email protected]> Co-authored-by: Florian Lefebvre <[email protected]>
withastro · Jan 18, 2024 · a4b696d · a4b696d
1 parent 259c30e
commit a4b696d
Show file tree

Hide file tree

Showing 3 changed files with 111 additions and 46 deletions.
diff --git a/.changeset/smart-rules-train.md b/.changeset/smart-rules-train.md
@@ -0,0 +1,5 @@
+---
+"astro": patch
+---
+
+Fixes a regression in routing priority between `index.astro` and dynamic routes with rest parameters
diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts
@@ -33,6 +33,10 @@ interface Item {
 	routeSuffix: string;
 }
 
+interface ManifestRouteData extends RouteData {
+	isIndex: boolean;
+}
+
 function countOccurrences(needle: string, haystack: string) {
 	let count = 0;
 	for (const hay of haystack) {
@@ -134,6 +138,40 @@ function validateSegment(segment: string, file = '') {
 	}
 }
 
+/**
+ * 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;
+		}
+
+		// 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;
+}
+
 /**
  * Comparator for sorting routes in resolution order.
  *
@@ -142,6 +180,8 @@ function validateSegment(segment: string, file = '') {
  * - 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`.
+ *   Index routes, originating from a file named `index.astro`, are considered to have one more
+ *   segment than the URL they represent.
  * - 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.
@@ -153,10 +193,14 @@ function validateSegment(segment: string, file = '') {
  *   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) {
+function routeComparator(a: ManifestRouteData, b: ManifestRouteData) {
+	// For sorting purposes, an index route is considered to have one more segment than the URL it represents.
+	const aLength = a.isIndex ? a.segments.length + 1 : a.segments.length;
+	const bLength = b.isIndex ? b.segments.length + 1 : b.segments.length;
+
 	// Sort more specific routes before less specific routes
-	if (a.segments.length !== b.segments.length) {
-		return a.segments.length > b.segments.length ? -1 : 1;
+	if (aLength !== bLength) {
+		return aLength > bLength ? -1 : 1;
 	}
 
 	const aIsStatic = a.segments.every((segment) =>
@@ -206,9 +250,9 @@ export interface CreateRouteManifestParams {
 function createFileBasedRoutes(
 	{ settings, cwd, fsMod }: CreateRouteManifestParams,
 	logger: Logger
-): RouteData[] {
+): ManifestRouteData[] {
 	const components: string[] = [];
-	const routes: RouteData[] = [];
+	const routes: ManifestRouteData[] = [];
 	const validPageExtensions = new Set<string>([
 		'.astro',
 		...SUPPORTED_MARKDOWN_FILE_EXTENSIONS,
@@ -321,6 +365,7 @@ function createFileBasedRoutes(
 					.join('/')}`.toLowerCase();
 				routes.push({
 					route,
+					isIndex: item.isIndex,
 					type: item.isPage ? 'page' : 'endpoint',
 					pattern,
 					segments,
@@ -348,7 +393,7 @@ function createFileBasedRoutes(
 	return routes;
 }
 
-type PrioritizedRoutesData = Record<RoutePriorityOverride, RouteData[]>;
+type PrioritizedRoutesData = Record<RoutePriorityOverride, ManifestRouteData[]>;
 
 function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): PrioritizedRoutesData {
 	const { config } = settings;
@@ -398,6 +443,8 @@ function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): Pri
 
 		routes[priority].push({
 			type,
+			// For backwards compatibility, an injected route is never considered an index route.
+			isIndex: false,
 			route,
 			pattern,
 			segments,
@@ -468,6 +515,8 @@ function createRedirectRoutes(
 
 		routes[priority].push({
 			type: 'redirect',
+			// For backwards compatibility, a redirect is never considered an index route.
+			isIndex: false,
 			route,
 			pattern,
 			segments,
@@ -492,40 +541,6 @@ function isStaticSegment(segment: RoutePart[]) {
 	return segment.every((part) => !part.dynamic && !part.spread);
 }
 
-/**
- * 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;
-		}
-
-		// 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.
  *
@@ -624,7 +639,7 @@ export function createRouteManifest(
 
 	const redirectRoutes = createRedirectRoutes(params, routeMap, logger);
 
-	const routes: RouteData[] = [
+	const routes: ManifestRouteData[] = [
 		...injectedRoutes['legacy'].sort(routeComparator),
 		...[...fileBasedRoutes, ...injectedRoutes['normal'], ...redirectRoutes['normal']].sort(
 			routeComparator
@@ -660,8 +675,8 @@ export function createRouteManifest(
 
 		// In this block of code we group routes based on their locale
 
-		// A map like: locale => RouteData[]
-		const routesByLocale = new Map<string, RouteData[]>();
+		// A map like: locale => ManifestRouteData[]
+		const routesByLocale = new Map<string, ManifestRouteData[]>();
 		// This type is here only as a helper. We copy the routes and make them unique, so we don't "process" the same route twice.
 		// The assumption is that a route in the file system belongs to only one locale.
 		const setRoutes = new Set(routes.filter((route) => route.type === 'page'));

diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js
@@ -103,6 +103,51 @@ describe('routing - createRouteManifest', () => {
 		]);
 	});
 
+	it('static routes are sorted before dynamic and rest routes', async () => {
+		const fs = createFs(
+			{
+				'/src/pages/[dynamic].astro': `<h1>test</h1>`,
+				'/src/pages/[...rest].astro': `<h1>test</h1>`,
+				'/src/pages/static.astro': `<h1>test</h1>`,
+				'/src/pages/index.astro': `<h1>test</h1>`,
+			},
+			root
+		);
+		const settings = await createBasicSettings({
+			root: fileURLToPath(root),
+			base: '/search',
+			trailingSlash: 'never',
+			experimental: {
+				globalRoutePriority: true,
+			},
+		});
+
+		const manifest = createRouteManifest({
+			cwd: fileURLToPath(root),
+			settings,
+			fsMod: fs,
+		});
+
+		expect(getManifestRoutes(manifest)).to.deep.equal([
+				{
+					"route": "/",
+					"type": "page",
+				},
+				{
+					"route": "/static",
+					"type": "page",
+				},
+				{
+					"route": "/[dynamic]",
+					"type": "page",
+				},
+				{
+					"route": "/[...rest]",
+					"type": "page",
+				},
+			]);
+	});
+
 	it('injected routes are sorted in legacy mode above filesystem routes', async () => {
 		const fs = createFs(
 			{
@@ -197,15 +242,15 @@ describe('routing - createRouteManifest', () => {
 				type: 'page',
 			},
 			{
-				route: '/contributing',
+				route: '/',
 				type: 'page',
 			},
 			{
-				route: '/[...slug]',
+				route: '/contributing',
 				type: 'page',
 			},
 			{
-				route: '/',
+				route: '/[...slug]',
 				type: 'page',
 			},
 		]);