Infrastructure/astro
0
Fork 0
mirror of https://github.com/withastro/astro.git synced 2025-01-20 22:12:38 -05:00
Code Issues Activity

Fixes regression on routing priority for multi-layer index pages (#10096)

Browse source 
* Reproduce regression

* Simplify sorting algorithm

* Add changeset

* Fix changeset typo

* Rename assertion utility function

* Fix index detection

* Add changeset for index fix

---------

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>
This commit is contained in:
Luiz Ferraz 2024-02-14 07:06:38 -03:00 • committed by GitHub
parent e24db1d8a6
commit 227cd83a51
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
4 changed files with 151 additions and 104 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
.changeset/early-windows-accept.md Normal file
View file

@ -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.

8
.changeset/lemon-cobras-swim.md Normal file
View file

@ -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 `/`.

74
packages/astro/src/core/routing/manifest/create.ts
View file

@ -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 (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;
}
if (a.segments.length > b.segments.length && !bEndsInRest) {
return 1;
if (bLength > aLength && bEndsInRest) {
// a: /foo
// b: /foo/[...bar]
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
);
return followingBSegmentIsStatic ? 1 : -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 = '/';

150
packages/astro/test/units/routing/manifest.nodetest.js
View file

@ -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), [
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: '/',
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: '/static',
type: 'page',
},
{
route: '/[dynamic]',
type: 'page',
},
{
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',
},
{