From d26afd87102578eff1498251121845b89174842f Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 18 Jan 2024 14:29:49 -0300 Subject: [PATCH 1/3] fix: Fix regression in the routing priority of index routes --- .../astro/src/core/routing/manifest/create.ts | 101 ++++++++++-------- .../astro/test/units/routing/manifest.test.js | 51 ++++++++- 2 files changed, 106 insertions(+), 46 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 2aeec9e4b6bf..874cfadc23eb 100644 --- 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([ '.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; +type PrioritizedRoutesData = Record; 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(); + // A map like: locale => ManifestRouteData[] + const routesByLocale = new Map(); // 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 index bcf387e5853b..55724d2c9ec7 100644 --- 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': `

test

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

test

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

test

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

test

`, + }, + 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', }, ]); From 37349e511e91c58ff673afaabbc319ca3b458d22 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 18 Jan 2024 14:36:58 -0300 Subject: [PATCH 2/3] chore: Add changeset --- .changeset/smart-rules-train.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/smart-rules-train.md diff --git a/.changeset/smart-rules-train.md b/.changeset/smart-rules-train.md new file mode 100644 index 000000000000..e6f7b9b39601 --- /dev/null +++ b/.changeset/smart-rules-train.md @@ -0,0 +1,5 @@ +--- +"astro": patch +--- + +Fix regression in routing priority between `index.astro` routes and dynamic routes with rest parameters From a638233164e6e474c0aeba6729b198580b2d491e Mon Sep 17 00:00:00 2001 From: Matthew Phillips Date: Thu, 18 Jan 2024 12:40:41 -0500 Subject: [PATCH 3/3] Update .changeset/smart-rules-train.md Co-authored-by: Florian Lefebvre --- .changeset/smart-rules-train.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/smart-rules-train.md b/.changeset/smart-rules-train.md index e6f7b9b39601..4d396762976f 100644 --- a/.changeset/smart-rules-train.md +++ b/.changeset/smart-rules-train.md @@ -2,4 +2,4 @@ "astro": patch --- -Fix regression in routing priority between `index.astro` routes and dynamic routes with rest parameters +Fixes a regression in routing priority between `index.astro` and dynamic routes with rest parameters