diff options
| -rw-r--r-- | .changeset/early-windows-accept.md | 23 | ||||
| -rw-r--r-- | .changeset/lemon-cobras-swim.md | 8 | ||||
| -rw-r--r-- | packages/astro/src/core/routing/manifest/create.ts | 74 | ||||
| -rw-r--r-- | packages/astro/test/units/routing/manifest.nodetest.js | 150 |
4 files changed, 151 insertions, 104 deletions
diff --git a/.changeset/early-windows-accept.md b/.changeset/early-windows-accept.md new file mode 100644 index 000000000..de3f17723 --- /dev/null +++ b/.changeset/early-windows-accept.md @@ -0,0 +1,23 @@ +--- +"astro": patch +--- + +Fixes regression on routing priority for multi-layer index pages + +The sorting algorithm positions more specific routes before less specific routes, and considers index pages to be more specific than a dynamic route with a rest parameter inside of it. +This means that `/blog` is considered more specific than `/blog/[...slug]`. + +But this special case was being applied incorrectly to indexes, which could cause a problem in scenarios like the following: +- `/` +- `/blog` +- `/blog/[...slug]` + +The algorithm would make the following comparisons: +- `/` is more specific than `/blog` (incorrect) +- `/blog/[...slug]` is more specific than `/` (correct) +- `/blog` is more specific than `/blog/[...slug]` (correct) + +Although the incorrect first comparison is not a problem by itself, it could cause the algorithm to make the wrong decision. +Depending on the other routes in the project, the sorting could perform just the last two comparisons and by transitivity infer the inverse of the third (`/blog/[...slug` > `/` > `/blog`), which is incorrect. + +Now the algorithm doesn't have a special case for index pages and instead does the comparison soleley for rest parameter segments and their immediate parents, which is consistent with the transitivity property. diff --git a/.changeset/lemon-cobras-swim.md b/.changeset/lemon-cobras-swim.md new file mode 100644 index 000000000..58eef28fd --- /dev/null +++ b/.changeset/lemon-cobras-swim.md @@ -0,0 +1,8 @@ +--- +"astro": patch +--- + +Fixes edge case on i18n fallback routes + +Previously index routes deeply nested in the default locale, like `/some/nested/index.astro` could be mistaked as the root index for the default locale, resulting in an incorrect redirect on `/`. + diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 6a1064c05..a3170a2ce 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -227,55 +227,33 @@ function routeComparator(a: RouteData, b: RouteData) { } } - // Special case to have `/[foo].astro` be equivalent to `/[foo]/index.astro` - // when compared against `/[foo]/[...rest].astro`. - if (Math.abs(a.segments.length - b.segments.length) === 1) { + const aLength = a.segments.length; + const bLength = b.segments.length; + + if (aLength !== bLength) { const aEndsInRest = a.segments.at(-1)?.some((part) => part.spread); const bEndsInRest = b.segments.at(-1)?.some((part) => part.spread); - // Routes with rest parameters are less specific than their parent route. - // For example, `/foo/[...bar]` is sorted after `/foo`. - - if (a.segments.length > b.segments.length && !bEndsInRest) { - return 1; - } - - if (b.segments.length > a.segments.length && !aEndsInRest) { - return -1; - } - } - - if (a.isIndex !== b.isIndex) { - // Index pages are lower priority than other static segments in the same prefix. - // They match the path up to their parent, but are more specific than the parent. - // For example: - // - `/foo/index.astro` is sorted before `/foo` - // - `/foo/index.astro` is sorted before `/foo/[bar].astro` - // - `/[...foo]/index.astro` is sorted after `/[...foo]/bar.astro` - - if (a.isIndex) { - const followingBSegment = b.segments.at(a.segments.length); - const followingBSegmentIsStatic = followingBSegment?.every( - (part) => !part.dynamic && !part.spread - ); + if (aEndsInRest !== bEndsInRest && Math.abs(aLength - bLength) === 1) { + // If only one of the routes ends in a rest parameter + // and the difference in length is exactly 1 + // and the shorter route is the one that ends in a rest parameter + // the shorter route is considered more specific. + // I.e. `/foo` is considered more specific than `/foo/[...bar]` + if (aLength > bLength && aEndsInRest) { + // b: /foo + // a: /foo/[...bar] + return 1; + } - return followingBSegmentIsStatic ? 1 : -1; + if (bLength > aLength && bEndsInRest) { + // a: /foo + // b: /foo/[...bar] + return -1; + } } - const followingASegment = a.segments.at(b.segments.length); - const followingASegmentIsStatic = followingASegment?.every( - (part) => !part.dynamic && !part.spread - ); - - return followingASegmentIsStatic ? -1 : 1; - } - - // 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; - - if (aLength !== bLength) { - // Routes are equal up to the smaller of the two lengths, so the longer route is more specific + // Sort routes by length return aLength > bLength ? -1 : 1; } @@ -781,10 +759,12 @@ export function createRouteManifest( // we attempt to retrieve the index page of the default locale const defaultLocaleRoutes = routesByLocale.get(i18n.defaultLocale); if (defaultLocaleRoutes) { - const indexDefaultRoute = defaultLocaleRoutes.find((routeData) => { - // it should be safe to assume that an index page has "index" in their name - return routeData.component.includes('index'); - }); + // The index for the default locale will be either already at the root path + // or at the root of the locale. + const indexDefaultRoute = defaultLocaleRoutes + .find(({route}) => route === '/') + ?? defaultLocaleRoutes.find(({route}) => route === `/${i18n.defaultLocale}`); + if (indexDefaultRoute) { // we found the index of the default locale, now we create a root index that will redirect to the index of the default locale const pathname = '/'; diff --git a/packages/astro/test/units/routing/manifest.nodetest.js b/packages/astro/test/units/routing/manifest.nodetest.js index d7bbc8c0f..110582bfb 100644 --- a/packages/astro/test/units/routing/manifest.nodetest.js +++ b/packages/astro/test/units/routing/manifest.nodetest.js @@ -26,6 +26,19 @@ function getLogger() { }; } +function assertRouteRelations(routes, relations) { + const routePaths = routes.map((route) => route.route); + + for (const [before, after] of relations) { + const beforeIndex = routePaths.indexOf(before); + const afterIndex = routePaths.indexOf(after); + + if (beforeIndex > afterIndex) { + assert.fail(`${before} should be higher priority than ${after}`); + } + } +} + describe('routing - createRouteManifest', () => { it('using trailingSlash: "never" does not match the index route when it contains a trailing slash', async () => { const fs = createFs( @@ -128,23 +141,58 @@ describe('routing - createRouteManifest', () => { fsMod: fs, }); - assert.deepEqual(getManifestRoutes(manifest), [ - { - route: '/', - type: 'page', - }, - { - route: '/static', - type: 'page', - }, + assertRouteRelations(getManifestRoutes(manifest), [ + ['/', '/[...rest]'], + ['/static', '/[dynamic]'], + ['/static', '/[...rest]'], + ['/[dynamic]', '/[...rest]'], + ]); + }); + + it('route sorting with multi-layer index page conflict', async () => { + // Reproducing regression from https://github.com/withastro/astro/issues/10071 + const fs = createFs( { - route: '/[dynamic]', - type: 'page', + '/src/pages/a/1.astro': `<h1>test</h1>`, + '/src/pages/a/2.astro': `<h1>test</h1>`, + '/src/pages/a/3.astro': `<h1>test</h1>`, + '/src/pages/modules/[...slug].astro': `<h1>test</h1>`, + '/src/pages/modules/index.astro': `<h1>test</h1>`, + '/src/pages/test/[...slug].astro': `<h1>test</h1>`, + '/src/pages/test/index.astro': `<h1>test</h1>`, + '/src/pages/index.astro': `<h1>test</h1>`, }, - { - route: '/[...rest]', - type: 'page', + root + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + base: '/search', + trailingSlash: 'never', + experimental: { + globalRoutePriority: true, }, + }); + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + assertRouteRelations(getManifestRoutes(manifest), [ + // Parent route should come before rest parameters + ['/test', '/test/[...slug]'], + ['/modules', '/modules/[...slug]'], + + // More specific routes should come before less specific routes + ['/a/1', '/'], + ['/a/2', '/'], + ['/a/3', '/'], + ['/test', '/'], + ['/modules', '/'], + + // Alphabetical order + ['/modules', '/test'], ]); }); @@ -157,6 +205,7 @@ describe('routing - createRouteManifest', () => { '/src/pages/[...rest]/static.astro': `<h1>test</h1>`, '/src/pages/[...rest]/index.astro': `<h1>test</h1>`, '/src/pages/blog/index.astro': `<h1>test</h1>`, + '/src/pages/blog/[...slug].astro': `<h1>test</h1>`, '/src/pages/[dynamic_file].astro': `<h1>test</h1>`, '/src/pages/[...other].astro': `<h1>test</h1>`, '/src/pages/static.astro': `<h1>test</h1>`, @@ -179,47 +228,34 @@ describe('routing - createRouteManifest', () => { fsMod: fs, }); - assert.deepEqual(getManifestRoutes(manifest), [ - { - route: '/', - type: 'page', - }, - { - route: '/blog', - type: 'page', - }, - { - route: '/static', - type: 'page', - }, - { - route: '/[dynamic_folder]', - type: 'page', - }, - { - route: '/[dynamic_file]', - type: 'page', - }, - { - route: '/[dynamic_folder]/static', - type: 'page', - }, - { - route: '/[dynamic_folder]/[...rest]', - type: 'page', - }, - { - route: '/[...rest]/static', - type: 'page', - }, - { - route: '/[...rest]', - type: 'page', - }, - { - route: '/[...other]', - type: 'page', - }, + assertRouteRelations(getManifestRoutes(manifest), [ + // Parent route should come before rest parameters + ['/', '/[...rest]'], + ['/', '/[...other]'], + ['/blog', '/blog/[...slug]'], + ['/[dynamic_file]', '/[dynamic_folder]/[...rest]'], + ['/[dynamic_folder]', '/[dynamic_folder]/[...rest]'], + + // Static should come before dynamic + ['/static', '/[dynamic_folder]'], + ['/static', '/[dynamic_file]'], + + // Static should come before rest parameters + ['/blog', '/[...rest]'], + ['/blog', '/[...other]'], + ['/static', '/[...rest]'], + ['/static', '/[...other]'], + ['/static', '/[...rest]/static'], + ['/[dynamic_folder]/static', '/[dynamic_folder]/[...rest]'], + + // Dynamic should come before rest parameters + ['/[dynamic_file]', '/[dynamic_folder]/[...rest]'], + + // More specific routes should come before less specific routes + ['/[dynamic_folder]/[...rest]', '/[...rest]'], + ['/[dynamic_folder]/[...rest]', '/[...other]'], + ['/blog/[...slug]', '/[...rest]'], + ['/blog/[...slug]', '/[...other]'], ]); }); @@ -317,11 +353,11 @@ describe('routing - createRouteManifest', () => { type: 'page', }, { - route: '/', + route: '/contributing', type: 'page', }, { - route: '/contributing', + route: '/', type: 'page', }, { |