From 9f6a7e99da7e3efdf2ddb89539a8c72a5f722660 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 14 Dec 2023 21:20:45 -0300 Subject: [PATCH 01/32] Implement priority overrides for injected routes and redirects --- packages/astro/src/@types/astro.ts | 11 + packages/astro/src/core/config/schema.ts | 1 + .../astro/src/core/routing/manifest/create.ts | 334 +++++++-------- .../astro/test/units/routing/manifest.test.js | 383 +++++++++++++++++- 4 files changed, 554 insertions(+), 175 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index fa8c33920ac0..4deee304a33e 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1555,10 +1555,20 @@ export interface AstroUserConfig { */ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | 'page-ssr'; +// TODO: Reconsider the names +/** + * IDs for different priorities of injected routes and redirects: + * - "above-project": Override any project route in case of conflict. + * - "same-as-project": Behave the same as if the route was defined in the project, following the same priority rules. + * - "below-project": Only match if no project route matches. + */ +export type RoutePriorityOverride = 'above-project' | 'same-as-project' | 'below-project'; + export interface InjectedRoute { pattern: string; entrypoint: string; prerender?: boolean; + priority?: RoutePriorityOverride; } export interface ResolvedInjectedRoute extends InjectedRoute { @@ -2385,6 +2395,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 08910720a13e..949447dad6af 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -173,6 +173,7 @@ export const AstroConfigSchema = z.object({ z.literal(308), ]), destination: z.string(), + priority: z.enum(['above-project', 'same-as-project', 'below-project']).optional(), }), ]) ) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 19e6e3006d16..1367175f85c9 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, @@ -115,11 +115,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; @@ -137,77 +132,39 @@ 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; +function comparator(a: RouteData, b: RouteData) { + 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)); - return isSpread(b.file) ? -1 : 1; + // static routes go first + if (aIsStatic !== bIsStatic) { + return aIsStatic ? -1 : 1; } - const max = Math.max(a.parts.length, b.parts.length); - - 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; + const aHasSpread = a.segments.some((segment) => segment.some((part) => part.spread)); + const bHasSpread = b.segments.some((segment) => segment.some((part) => part.spread)); - 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) - ); - } + // rest parameters go last + if (aHasSpread !== bHasSpread) { + return aHasSpread ? 1 : -1; } - // endpoints are prioritized over pages - if (a.isPage !== b.isPage) { - return a.isPage ? 1 : -1; + // prerendered routes go first + if (a.prerender !== b.prerender) { + return a.prerender ? -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)); + // endpoints take precedence over pages + if ((a.type === 'endpoint') !== (b.type === 'endpoint')) { + return a.type === 'endpoint' ? -1 : 1; } - const ext = path.extname(pattern); - - const type = resolved.endsWith('.astro') ? 'page' : 'endpoint'; - const isPage = type === 'page'; + // more specific routes go first + if (a.segments.length !== b.segments.length) { + return a.segments.length > b.segments.length ? -1 : 1; + } - return { - basename: pattern, - ext, - parts: getParts(pattern, resolved), - file: resolved, - isDir: false, - isIndex: true, - isPage, - routeSuffix: pattern.slice(pattern.indexOf('.'), -ext.length), - }; + return a.route.localeCompare(b.route); } export interface CreateRouteManifestParams { @@ -219,11 +176,10 @@ export interface CreateRouteManifestParams { fsMod?: typeof nodeFs; } -/** Create manifest of all static routes */ -export function createRouteManifest( - { settings, cwd, fsMod }: CreateRouteManifestParams, - logger: Logger -): ManifestData { +function createProjectRoutes( + {settings, cwd, fsMod}: CreateRouteManifestParams, + logger: Logger, +): RouteData[] { const components: string[] = []; const routes: RouteData[] = []; const validPageExtensions = new Set([ @@ -239,7 +195,7 @@ export function createRouteManifest( fs: typeof nodeFs, dir: string, parentSegments: RoutePart[][], - parentParams: string[] + parentParams: string[], ) { let items: Item[] = []; fs.readdirSync(dir).forEach((basename) => { @@ -261,8 +217,8 @@ export function createRouteManifest( logger.warn( null, `Unsupported file type ${bold( - resolved - )} found. Prefix filename with an underscore (\`_\`) to ignore.` + resolved, + )} found. Prefix filename with an underscore (\`_\`) to ignore.`, ); return; @@ -286,7 +242,6 @@ export function createRouteManifest( routeSuffix, }); }); - items = items.sort(comparator); items.forEach((item) => { const segments = parentSegments.slice(); @@ -335,7 +290,7 @@ export function createRouteManifest( ? `/${segments.map((segment) => segment[0].content).join('/')}` : null; const route = `/${segments - .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) + .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); routes.push({ route, @@ -353,7 +308,7 @@ export function createRouteManifest( }); } - const { config } = settings; + const {config} = settings; const pages = resolvePages(config); if (localFs.existsSync(pages)) { @@ -363,69 +318,83 @@ 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)); - - const segments = removeLeadingForwardSlash(name) - .split(path.posix.sep) - .filter(Boolean) - .map((s: string) => { - validateSegment(s); - return getParts(s, component); - }); + return routes; +} - 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(); - - 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}".` - ); - } +type PrioritizedRoutesData = Record; + +function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): PrioritizedRoutesData { + const {config} = settings; + const prerender = getPrerenderDefault(config); + + const routes: PrioritizedRoutesData = { + 'above-project': [], + 'same-as-project': [], + 'below-project': [], + }; + + settings.injectedRoutes.forEach(({pattern: name, entrypoint, prerender: prerenderInjected, priority}) => { + 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)); - // 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: [], + 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 ?? 'above-project'].push({ + type, + route, + pattern, + segments, + params, + component, + generate, + pathname: pathname || void 0, + prerender: prerenderInjected ?? prerender, + fallbackRoutes: [], }); + }); + + return routes; +} + +function createRedirectRoutes( + {settings}: CreateRouteManifestParams, + routeMap: Map, + logger: Logger, +): PrioritizedRoutesData { + const {config} = settings; + const routes: PrioritizedRoutesData = { + 'above-project': [], + 'same-as-project': [], + 'below-project': [], + }; Object.entries(settings.config.redirects).forEach(([from, to]) => { const trailingSlash = config.trailingSlash; @@ -448,7 +417,7 @@ export function createRouteManifest( .filter((p) => p.dynamic) .map((p) => p.content); const route = `/${segments - .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) + .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); { @@ -461,12 +430,15 @@ export function createRouteManifest( if (/^https?:\/\//.test(destination)) { logger.warn( 'redirects', - `Redirecting to an external URL is not officially supported: ${from} -> ${to}` + `Redirecting to an external URL is not officially supported: ${from} -> ${to}`, ); } } - const routeData: RouteData = { + const destination = typeof to === 'string' ? to : to.destination; + const priority = typeof to === 'string' ? undefined : to.priority; + + routes[priority ?? 'below-project'].push({ type: 'redirect', route, pattern, @@ -476,35 +448,73 @@ export function createRouteManifest( generate, pathname: pathname || void 0, prerender: false, - redirect: to, - redirectRoute: routes.find((r) => r.route === to), + redirect: typeof to === 'string' ? to : { + status: to.status, + destination: to.destination, + }, + 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; - } +/** Create manifest of all static routes */ +export function createRouteManifest( + params: CreateRouteManifestParams, + logger: Logger, +): ManifestData { + // Create a map of all routes so redirects can refer to any route + const routeMap = new Map(); - // 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++; - } + const projectRoutes = createProjectRoutes(params, logger); + projectRoutes.forEach((route) => { + routeMap.set(route.route, route); + }); + + const injectedRoutes = createInjectedRoutes(params); + Object.entries(injectedRoutes).forEach(([, routes]) => { + routes.forEach((route) => { + routeMap.set(route.route, route); + }); + }); - // Didn't find a good place, insert last - routes.push(routeData); + const redirectRoutes = createRedirectRoutes(params, routeMap, logger); + + const routes: RouteData[] = [ + ...[ + ...injectedRoutes['above-project'], + ...redirectRoutes['above-project'], + ].sort(comparator), + ...[ + ...projectRoutes, + ...injectedRoutes['same-as-project'], + ...redirectRoutes['same-as-project'], + ].sort(comparator), + ...[ + ...injectedRoutes['below-project'], + ...redirectRoutes['below-project'], + ].sort(comparator), + ]; + + // Detect route collisions + routes.forEach((route, index) => { + const collision = routes.find( + (other, otherIndex) => (index !== otherIndex && other.route === route.route), + ); + + if (collision) { + throw new Error( + `Colliding routes detected in the project: "${route.route}" at "${route.component}".\n` + + `This route collides with: "${collision.component}".`, + ); + } }); + + const {settings} = params; + const {config} = settings; + const i18n = settings.config.i18n; if (i18n) { // In this block of code we group routes based on their locale diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index a7014d853152..0c6bf91042a8 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -6,6 +6,13 @@ import { createBasicSettings, createFs, defaultLogger } from '../test-utils.js'; const root = new URL('../../fixtures/alias/', import.meta.url); +function getManifestRoutes(manifest) { + return manifest.routes.map(route => ({ + type: route.type, + route: route.route, + })); +} + describe('routing - createRouteManifest', () => { it('using trailingSlash: "never" does not match the index route when it contains a trailing slash', async () => { const fs = createFs( @@ -29,7 +36,278 @@ describe('routing - createRouteManifest', () => { expect(pattern.test('/')).to.equal(false); }); - it('redirects are sorted alongside the filesystem routes', async () => { + 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', + }); + + settings.injectedRoutes = [ + { + pattern: '/about', + entrypoint: '@lib/override/static.astro', + }, + { + pattern: '/api', + entrypoint: '@lib/override/static.ts', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/api', + type: 'endpoint', + }, + { + route: '/about', + type: 'page', + }, + { + route: '/sendcontact', + type: 'endpoint', + }, + { + route: '/contact-me', + type: 'page', + }, + ]); + }); + + it('injected routes are sorted as specified 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), + base: '/search', + trailingSlash: 'never', + }); + + settings.injectedRoutes = [ + { + pattern: '/contributing', + entrypoint: '@lib/override/static.astro', + priority: 'above-project', + }, + { + pattern: '/[...slug]', + entrypoint: '@lib/override/dynamic.astro', + // Don's specify a priority to test that it defaults to above-project + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/contributing', + type: 'page', + }, + { + route: '/[...slug]', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + { + route: '/blog/[...slug]', + type: 'page', + }, + ]); + }); + + it('injected routes are sorted as specified below 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), + base: '/search', + trailingSlash: 'never', + }); + + settings.injectedRoutes = [ + { + pattern: '/contributing', + entrypoint: '@lib/override/static.astro', + priority: 'below-project', + }, + { + pattern: '/[...slug]', + entrypoint: '@lib/override/dynamic.astro', + priority: 'below-project', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/', + type: 'page', + }, + { + route: '/blog/[...slug]', + type: 'page', + }, + { + route: '/contributing', + type: 'page', + }, + { + route: '/[...slug]', + type: 'page', + }, + ]); + }); + + it('injected routes are sorted as specified 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), + base: '/search', + trailingSlash: 'never', + integrations: [ + { + name: '@test', + hooks: { + 'astro:config:setup': ({injectRoute}) => { + }, + }, + }, + ], + }); + + settings.injectedRoutes = [ + { + pattern: '/contributing', + entrypoint: '@lib/override/static.astro', + priority: 'same-as-project', + }, + { + pattern: '/[...slug]', + entrypoint: '@lib/override/dynamic.astro', + priority: 'same-as-project', + }, + ]; + + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/contributing', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + { + route: '/blog/[...slug]', + type: 'page', + }, + { + route: '/[...slug]', + type: 'page', + }, + ]); + }); + + it('redirects are sorted as specified above 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), + base: '/search', + trailingSlash: 'never', + redirects: { + // Do not specify a priority to test that it defaults to below-project + '/blog/[...slug]': { + status: 302, + destination: '/', + priority: 'above-project', + }, + '/blog/about': { + status: 302, + destination: '/another', + priority: 'above-project', + } + }, + }); + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/about', + type: 'redirect', + }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, + { + route: '/blog/contributing', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + ]); + }); + + it('redirects are sorted as specified below the filesystem routes', async () => { const fs = createFs( { '/src/pages/index.astro': `

test

`, @@ -42,8 +320,13 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', redirects: { + // Do not specify a priority to test that it defaults to below-project '/blog/[...slug]': '/', - '/blog/contributing': '/another', + '/blog/about': { + status: 302, + destination: '/another', + priority: 'below-project', + } }, }); const manifest = createRouteManifest({ @@ -52,34 +335,108 @@ 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/contributing', + type: 'page', + }, + { + route: '/', + type: 'page', + }, + { + route: '/blog/about', + type: 'redirect', + }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, + ]); }); - it('static redirect route is prioritized over dynamic file route', async () => { + it('redirects are sorted as specified alongside the filesystem routes', async () => { const fs = createFs( { - '/src/pages/[...slug].astro': `

test

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

test

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

test

`, }, root ); const settings = await createBasicSettings({ root: fileURLToPath(root), + base: '/search', trailingSlash: 'never', redirects: { - '/foo': '/bar', + '/blog/[...slug]': { + status: 302, + destination: '/', + priority: 'same-as-project', + }, + '/blog/about': { + status: 302, + destination: '/another', + priority: 'same-as-project', + } + }, + }); + const manifest = createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + + expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/about', + type: 'redirect', + }, + { + route: '/blog/contributing', + type: 'page', + }, + { + route: '/', + type: 'page', }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, + ]); + }); + + it('detects colision between routes', async () => { + const fs = createFs( + { + '/src/pages/blog/[...slug].astro': `

test

`, + }, + root, + ); + const settings = await createBasicSettings({ + root: fileURLToPath(root), + base: '/search', + trailingSlash: 'never', }); - const manifest = createRouteManifest( + + settings.injectedRoutes = [ { + pattern: '/blog/[...slug]', + entrypoint: '@lib/override/blog.astro', + }, + ]; + + const testFn = () => { + createRouteManifest({ cwd: fileURLToPath(root), settings, fsMod: fs, - }, - defaultLogger - ); + }); + }; - expect(manifest.routes[0].route).to.equal('/foo'); + expect(testFn).to.throw( + 'Colliding routes detected in the project: "/blog/[...slug]" at "@lib/override/blog.astro".\n' + + 'This route collides with: "src/pages/blog/[...slug].astro".' + ); }); }); From a6671a8df0545782d460f8cb24acf42204db6529 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 14 Dec 2023 22:23:36 -0300 Subject: [PATCH 02/32] Fix ordering for route specificity --- .../astro/src/core/routing/manifest/create.ts | 10 ++++---- .../astro/test/units/routing/manifest.test.js | 24 +++++++++---------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 1367175f85c9..09870b77ed5d 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -133,6 +133,11 @@ function validateSegment(segment: string, file = '') { } function comparator(a: RouteData, b: RouteData) { + // more specific routes go first + if (a.segments.length !== b.segments.length) { + return a.segments.length > b.segments.length ? -1 : 1; + } + 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)); @@ -159,11 +164,6 @@ function comparator(a: RouteData, b: RouteData) { return a.type === 'endpoint' ? -1 : 1; } - // more specific routes go first - if (a.segments.length !== b.segments.length) { - return a.segments.length > b.segments.length ? -1 : 1; - } - return a.route.localeCompare(b.route); } diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 0c6bf91042a8..7714d9d36e35 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -130,11 +130,11 @@ describe('routing - createRouteManifest', () => { type: 'page', }, { - route: '/', + route: '/blog/[...slug]', type: 'page', }, { - route: '/blog/[...slug]', + route: '/', type: 'page', }, ]); @@ -175,11 +175,11 @@ describe('routing - createRouteManifest', () => { expect(getManifestRoutes(manifest)).to.deep.equal([ { - route: '/', + route: '/blog/[...slug]', type: 'page', }, { - route: '/blog/[...slug]', + route: '/', type: 'page', }, { @@ -237,19 +237,19 @@ describe('routing - createRouteManifest', () => { expect(getManifestRoutes(manifest)).to.deep.equal([ { - route: '/contributing', + route: '/blog/[...slug]', type: 'page', }, { - route: '/', + route: '/contributing', type: 'page', }, { - route: '/blog/[...slug]', + route: '/[...slug]', type: 'page', }, { - route: '/[...slug]', + route: '/', type: 'page', }, ]); @@ -395,14 +395,14 @@ describe('routing - createRouteManifest', () => { route: '/blog/contributing', type: 'page', }, - { - route: '/', - type: 'page', - }, { route: '/blog/[...slug]', type: 'redirect', }, + { + route: '/', + type: 'page', + }, ]); }); From bf8a33308c64a34aab65741fa866ca1dab2a3b07 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 14 Dec 2023 23:47:37 -0300 Subject: [PATCH 03/32] Don't mix rules on tests --- .../astro/test/units/routing/manifest.test.js | 40 +++---------------- 1 file changed, 6 insertions(+), 34 deletions(-) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 7714d9d36e35..411978d056f4 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -97,6 +97,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', }); @@ -150,6 +151,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', }); @@ -203,6 +205,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', integrations: [ @@ -265,6 +268,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', redirects: { @@ -317,6 +321,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', redirects: { @@ -365,6 +370,7 @@ describe('routing - createRouteManifest', () => { ); const settings = await createBasicSettings({ root: fileURLToPath(root), + output: 'server', base: '/search', trailingSlash: 'never', redirects: { @@ -405,38 +411,4 @@ describe('routing - createRouteManifest', () => { }, ]); }); - - it('detects colision between routes', async () => { - const fs = createFs( - { - '/src/pages/blog/[...slug].astro': `

test

`, - }, - root, - ); - const settings = await createBasicSettings({ - root: fileURLToPath(root), - base: '/search', - trailingSlash: 'never', - }); - - settings.injectedRoutes = [ - { - pattern: '/blog/[...slug]', - entrypoint: '@lib/override/blog.astro', - }, - ]; - - const testFn = () => { - createRouteManifest({ - cwd: fileURLToPath(root), - settings, - fsMod: fs, - }); - }; - - expect(testFn).to.throw( - 'Colliding routes detected in the project: "/blog/[...slug]" at "@lib/override/blog.astro".\n' - + 'This route collides with: "src/pages/blog/[...slug].astro".' - ); - }); }); From 31885a9bd01e835d02ebcad511914a407f01d1f3 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Thu, 14 Dec 2023 23:49:01 -0300 Subject: [PATCH 04/32] Detailed collision detection --- .../astro/src/core/routing/manifest/create.ts | 63 ++++++++++++++++++- 1 file changed, 61 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 09870b77ed5d..a389b11d2a9a 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -460,6 +460,64 @@ function createRedirectRoutes( return routes; } +function routesCollide(a: RouteData, b: RouteData) { + if (a.segments.length !== b.segments.length) { + return false; + } + + 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 false; + } + + const length = a.segments.length; + let hasDynamic = false; + + for (let i = 0; i < length; i++) { + const segmentA = a.segments[i]; + const segmentB = b.segments[i]; + + if (segmentA.length !== segmentB.length) { + return false; + } + + const segmentLength = segmentA.length; + + for (let j = 0; j < segmentLength; j++) { + const partA = segmentA[j]; + const partB = segmentB[j]; + + if ( + // If only one side has a rest parameter, they don't collide + partA.spread !== partB.spread + // If only one side is dynamic, they don't collide + || partA.dynamic !== partB.dynamic + ) { + return false; + } + + hasDynamic = hasDynamic || partA.dynamic; + + // If both sides are static and have different content, they don't collide + if (!partA.dynamic && partA.content !== partB.content) { + return false; + } + } + } + + if (hasDynamic && (a.prerender || b.prerender)) { + // If the route is dynamic and either of the routes is prerendered, + // we cannot afirm that they collide at this point. + // They will only collide if both routes are prerendered and return + // the same parameters on `getStaticPaths`, which is checked later. + return false; + } + + // If every segment and part is equivalent, they collide + return true; +} + /** Create manifest of all static routes */ export function createRouteManifest( params: CreateRouteManifestParams, @@ -501,11 +559,12 @@ export function createRouteManifest( // Detect route collisions routes.forEach((route, index) => { const collision = routes.find( - (other, otherIndex) => (index !== otherIndex && other.route === route.route), + (other, otherIndex) => (index !== otherIndex && routesCollide(route, other)), ); if (collision) { - throw new Error( + logger.warn( + 'router', `Colliding routes detected in the project: "${route.route}" at "${route.component}".\n` + `This route collides with: "${collision.component}".`, ); From 06bcec2c7372e0bcf4dbc6b07c7d67b5e5f28835 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Fri, 15 Dec 2023 00:05:12 -0300 Subject: [PATCH 05/32] Add changeset --- .changeset/smooth-cobras-help.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) create mode 100644 .changeset/smooth-cobras-help.md diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md new file mode 100644 index 000000000000..a47e2baece59 --- /dev/null +++ b/.changeset/smooth-cobras-help.md @@ -0,0 +1,20 @@ +--- +'astro': minor +--- + +Reworks route priority processing to allow for more flexible and intuitive redirects and route injection + +- Priority order for project routes, injected routes and redirects are now all the same. +- Injected routes and redirects can now specify if they should be prioritized above, + with or below project routes. This is done by adding a `priority` property to the route + object in `injectRoute` and in the `redirects` property in `astro.config.mjs`. +- Now more specific routes always take priority over less specific routes. + Example: `/blog/[...slug]` will take priority over `/[...slug]` +- Static redirects now have a lower priority than all project routed, even if the routes are dynamic, + matching the already documented behavior. + Example: `/blog (redirect)` will no longer override a `/[slug]` route by default, this can be re-enabled + using the new `priority` field. +- Collision detection between routes can now detect coliisions between similar dynamic routes + of any kind (project routes, injected routes and redirects). + Example: `/blog/[page]` will now be detected as a collision with `/blog/[slug]` +- Colision detection is now reported as a warning for backward compatibility with all the previous false negatives. From 8a61bb7eb07457be5e5a3372d37f15598fdeef3d Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Fri, 15 Dec 2023 00:12:33 -0300 Subject: [PATCH 06/32] Remove TODO --- packages/astro/src/@types/astro.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 4deee304a33e..c5aa633c1f72 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1555,7 +1555,6 @@ export interface AstroUserConfig { */ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | 'page-ssr'; -// TODO: Reconsider the names /** * IDs for different priorities of injected routes and redirects: * - "above-project": Override any project route in case of conflict. From c2117045e25e7a86bbee5855638a53834bd36d6f Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 18 Dec 2023 15:35:59 -0300 Subject: [PATCH 07/32] Add comments to clarify default values --- packages/astro/src/core/routing/manifest/create.ts | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 4ab043458bca..fe0d5e870967 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -164,6 +164,7 @@ function comparator(a: RouteData, b: RouteData) { return a.type === 'endpoint' ? -1 : 1; } + // if all else is equal, sort alphabetically return a.route.localeCompare(b.route); } @@ -367,6 +368,8 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); + // If an injected route doesn't define a priority, it will be have a + // higher priority than the project routes for backwards compatibility. routes[priority ?? 'above-project'].push({ type, route, @@ -431,6 +434,8 @@ function createRedirectRoutes( const priority = typeof to === 'string' ? undefined : to.priority; + // If a redirect doesn't define a priority, it will be have a + // lower priority than the project routes for backwards compatibility. routes[priority ?? 'below-project'].push({ type: 'redirect', route, From 7379e1dec5b1623bb3204ecc4d6c4d8c4e8bcd6c Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Wed, 20 Dec 2023 19:36:54 -0300 Subject: [PATCH 08/32] Update terminology --- packages/astro/src/@types/astro.ts | 11 ++-- packages/astro/src/assets/endpoint/config.ts | 1 + packages/astro/src/core/config/schema.ts | 8 ++- .../astro/src/core/routing/manifest/create.ts | 61 +++++++++---------- .../astro/test/units/routing/manifest.test.js | 26 ++++---- scripts/cmd/build.js | 2 +- 6 files changed, 58 insertions(+), 51 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 3c33b13e4673..b705945a2041 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1557,11 +1557,14 @@ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | /** * IDs for different priorities of injected routes and redirects: - * - "above-project": Override any project route in case of conflict. - * - "same-as-project": Behave the same as if the route was defined in the project, following the same priority rules. - * - "below-project": Only match if no project route matches. + * - "override": Override any file-based project route in case of conflict. Conflicts within override + * routes are resolved by the same rules as file-based routes. + * - "merge": Merge with discovered file-based project routes, behaving the same as if the route + * was defined as a file in the project. + * - "defer": Defer to any file-based project route in case of conflict. Conflicts within defer + * routes are resolved by the same rules as file-based routes. */ -export type RoutePriorityOverride = 'above-project' | 'same-as-project' | 'below-project'; +export type RoutePriorityOverride = 'override' | 'normal' | 'defer'; export interface InjectedRoute { pattern: string; diff --git a/packages/astro/src/assets/endpoint/config.ts b/packages/astro/src/assets/endpoint/config.ts index 07cfe8faecbb..b56d33246284 100644 --- a/packages/astro/src/assets/endpoint/config.ts +++ b/packages/astro/src/assets/endpoint/config.ts @@ -9,6 +9,7 @@ export function injectImageEndpoint(settings: AstroSettings, mode: 'dev' | 'buil pattern: '/_image', entrypoint: endpointEntrypoint, prerender: false, + priority: 'override', }); return settings; diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 949447dad6af..2188dcb5754a 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -173,9 +173,15 @@ export const AstroConfigSchema = z.object({ z.literal(308), ]), destination: z.string(), - priority: z.enum(['above-project', 'same-as-project', 'below-project']).optional(), + priority: z.enum(['override', 'normal', 'defer']) + .default('defer'), }), ]) + .transform(redirect => ( + typeof redirect === 'string' + ? {destination: redirect, priority: 'defer' as const} + : redirect + )) ) .default(ASTRO_CONFIG_DEFAULTS.redirects), prefetch: z diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index fe0d5e870967..e7c8ba823378 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -177,7 +177,7 @@ export interface CreateRouteManifestParams { fsMod?: typeof nodeFs; } -function createProjectRoutes( +function createFileBasedRoutes( {settings, cwd, fsMod}: CreateRouteManifestParams, logger: Logger, ): RouteData[] { @@ -329,9 +329,9 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior const prerender = getPrerenderDefault(config); const routes: PrioritizedRoutesData = { - 'above-project': [], - 'same-as-project': [], - 'below-project': [], + 'override': [], + 'normal': [], + 'defer': [], }; settings.injectedRoutes.forEach(({pattern: name, entrypoint, prerender: prerenderInjected, priority}) => { @@ -368,9 +368,9 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - // If an injected route doesn't define a priority, it will be have a - // higher priority than the project routes for backwards compatibility. - routes[priority ?? 'above-project'].push({ + // If an injected route doesn't define a priority, it will override + // file-based routes from the project for backwards compatibility. + routes[priority ?? 'override'].push({ type, route, pattern, @@ -394,9 +394,9 @@ function createRedirectRoutes( ): PrioritizedRoutesData { const {config} = settings; const routes: PrioritizedRoutesData = { - 'above-project': [], - 'same-as-project': [], - 'below-project': [], + 'override': [], + 'normal': [], + 'defer': [], }; Object.entries(settings.config.redirects).forEach(([from, to]) => { @@ -424,19 +424,14 @@ function createRedirectRoutes( .join('/')}`.toLowerCase(); - const destination = typeof to === 'string' ? to : to.destination; - if (/^https?:\/\//.test(destination)) { + if (/^https?:\/\//.test(to.destination)) { logger.warn( 'redirects', - `Redirecting to an external URL is not officially supported: ${from} -> ${destination}`, + `Redirecting to an external URL is not officially supported: ${from} -> ${to.destination}`, ); } - - const priority = typeof to === 'string' ? undefined : to.priority; - // If a redirect doesn't define a priority, it will be have a - // lower priority than the project routes for backwards compatibility. - routes[priority ?? 'below-project'].push({ + routes[to.priority].push({ type: 'redirect', route, pattern, @@ -446,11 +441,13 @@ function createRedirectRoutes( generate, pathname: pathname || void 0, prerender: false, - redirect: typeof to === 'string' ? to : { - status: to.status, - destination: to.destination, - }, - redirectRoute: routeMap.get(destination), + redirect: 'status' in to + ? { + status: to.status, + destination: to.destination, + } + : to.destination, + redirectRoute: routeMap.get(to.destination), fallbackRoutes: [], }); }); @@ -524,8 +521,8 @@ export function createRouteManifest( // Create a map of all routes so redirects can refer to any route const routeMap = new Map(); - const projectRoutes = createProjectRoutes(params, logger); - projectRoutes.forEach((route) => { + const fileBasedRoutes = createFileBasedRoutes(params, logger); + fileBasedRoutes.forEach((route) => { routeMap.set(route.route, route); }); @@ -540,17 +537,17 @@ export function createRouteManifest( const routes: RouteData[] = [ ...[ - ...injectedRoutes['above-project'], - ...redirectRoutes['above-project'], + ...injectedRoutes['override'], + ...redirectRoutes['override'], ].sort(comparator), ...[ - ...projectRoutes, - ...injectedRoutes['same-as-project'], - ...redirectRoutes['same-as-project'], + ...fileBasedRoutes, + ...injectedRoutes['normal'], + ...redirectRoutes['normal'], ].sort(comparator), ...[ - ...injectedRoutes['below-project'], - ...redirectRoutes['below-project'], + ...injectedRoutes['defer'], + ...redirectRoutes['defer'], ].sort(comparator), ]; diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 411978d056f4..4022ab29b702 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -106,12 +106,12 @@ describe('routing - createRouteManifest', () => { { pattern: '/contributing', entrypoint: '@lib/override/static.astro', - priority: 'above-project', + priority: 'override', }, { pattern: '/[...slug]', entrypoint: '@lib/override/dynamic.astro', - // Don's specify a priority to test that it defaults to above-project + // Don's specify a priority to test that it defaults to override }, ]; @@ -160,12 +160,12 @@ describe('routing - createRouteManifest', () => { { pattern: '/contributing', entrypoint: '@lib/override/static.astro', - priority: 'below-project', + priority: 'defer', }, { pattern: '/[...slug]', entrypoint: '@lib/override/dynamic.astro', - priority: 'below-project', + priority: 'defer', }, ]; @@ -223,12 +223,12 @@ describe('routing - createRouteManifest', () => { { pattern: '/contributing', entrypoint: '@lib/override/static.astro', - priority: 'same-as-project', + priority: 'normal', }, { pattern: '/[...slug]', entrypoint: '@lib/override/dynamic.astro', - priority: 'same-as-project', + priority: 'normal', }, ]; @@ -272,16 +272,16 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', redirects: { - // Do not specify a priority to test that it defaults to below-project + // Do not specify a priority to test that it defaults to defer '/blog/[...slug]': { status: 302, destination: '/', - priority: 'above-project', + priority: 'override', }, '/blog/about': { status: 302, destination: '/another', - priority: 'above-project', + priority: 'override', } }, }); @@ -325,12 +325,12 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', redirects: { - // Do not specify a priority to test that it defaults to below-project + // Do not specify a priority to test that it defaults to defer '/blog/[...slug]': '/', '/blog/about': { status: 302, destination: '/another', - priority: 'below-project', + priority: 'defer', } }, }); @@ -377,12 +377,12 @@ describe('routing - createRouteManifest', () => { '/blog/[...slug]': { status: 302, destination: '/', - priority: 'same-as-project', + priority: 'normal', }, '/blog/about': { status: 302, destination: '/another', - priority: 'same-as-project', + priority: 'normal', } }, }); diff --git a/scripts/cmd/build.js b/scripts/cmd/build.js index b516e6b5105f..564dfd62afd6 100644 --- a/scripts/cmd/build.js +++ b/scripts/cmd/build.js @@ -13,7 +13,7 @@ const defaultConfig = { format: 'esm', platform: 'node', target: 'node18', - sourcemap: false, + sourcemap: 'linked', sourcesContent: false, }; From a064b4e98977e042592708914554aacdda26f244 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Wed, 20 Dec 2023 22:01:58 -0300 Subject: [PATCH 09/32] Revert unrelated changes --- scripts/cmd/build.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/scripts/cmd/build.js b/scripts/cmd/build.js index 564dfd62afd6..b516e6b5105f 100644 --- a/scripts/cmd/build.js +++ b/scripts/cmd/build.js @@ -13,7 +13,7 @@ const defaultConfig = { format: 'esm', platform: 'node', target: 'node18', - sourcemap: 'linked', + sourcemap: false, sourcesContent: false, }; From cea5439ae7a5619318d4b280d080a3b3c132209c Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Fri, 12 Jan 2024 22:52:27 -0300 Subject: [PATCH 10/32] WIP --- packages/astro/src/@types/astro.ts | 10 +- packages/astro/src/core/config/schema.ts | 6 +- .../astro/src/core/routing/manifest/create.ts | 263 ++++++++++++------ .../astro/test/units/routing/manifest.test.js | 8 +- 4 files changed, 187 insertions(+), 100 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index b705945a2041..8e8dae15ee76 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1557,14 +1557,12 @@ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | /** * IDs for different priorities of injected routes and redirects: - * - "override": Override any file-based project route in case of conflict. Conflicts within override - * routes are resolved by the same rules as file-based routes. - * - "merge": Merge with discovered file-based project routes, behaving the same as if the route + * - "normal": Merge with discovered file-based project routes, behaving the same as if the route * was defined as a file in the project. - * - "defer": Defer to any file-based project route in case of conflict. Conflicts within defer - * routes are resolved by the same rules as file-based routes. + * - "legacy": Use the old ordering of routes. Inject routes will override any file-based project route, + * and redirects will be overriden by any project route on conflict. */ -export type RoutePriorityOverride = 'override' | 'normal' | 'defer'; +export type RoutePriorityOverride = 'normal' | 'legacy'; export interface InjectedRoute { pattern: string; diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 2188dcb5754a..6ce7d257bd7c 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -173,13 +173,13 @@ export const AstroConfigSchema = z.object({ z.literal(308), ]), destination: z.string(), - priority: z.enum(['override', 'normal', 'defer']) - .default('defer'), + priority: z.enum(['normal', 'legacy']) + .default('normal'), }), ]) .transform(redirect => ( typeof redirect === 'string' - ? {destination: redirect, priority: 'defer' as const} + ? {destination: redirect, priority: 'normal' as const} : redirect )) ) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index e7c8ba823378..af946fb32179 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -132,8 +132,27 @@ function validateSegment(segment: string, file = '') { } } -function comparator(a: RouteData, b: RouteData) { - // more specific routes go first +/** + * 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; } @@ -141,7 +160,7 @@ function comparator(a: RouteData, b: RouteData) { 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)); - // static routes go first + // Sort static routes before dynamic routes if (aIsStatic !== bIsStatic) { return aIsStatic ? -1 : 1; } @@ -149,22 +168,23 @@ function comparator(a: RouteData, b: RouteData) { const aHasSpread = a.segments.some((segment) => segment.some((part) => part.spread)); const bHasSpread = b.segments.some((segment) => segment.some((part) => part.spread)); - // rest parameters go last + // 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; } - // prerendered routes go first + // Sort prerendered routes before non-prerendered routes if (a.prerender !== b.prerender) { return a.prerender ? -1 : 1; } - // endpoints take precedence over pages + // Sort endpoints before pages if ((a.type === 'endpoint') !== (b.type === 'endpoint')) { return a.type === 'endpoint' ? -1 : 1; } - // if all else is equal, sort alphabetically + // Sort alphabetically return a.route.localeCompare(b.route); } @@ -199,7 +219,7 @@ function createFileBasedRoutes( parentParams: string[], ) { let items: Item[] = []; - fs.readdirSync(dir).forEach((basename) => { + for (const basename of fs.readdirSync(dir)) { const resolved = path.join(dir, basename); const file = slash(path.relative(cwd || fileURLToPath(settings.config.root), resolved)); const isDir = fs.statSync(resolved).isDirectory(); @@ -242,9 +262,10 @@ function createFileBasedRoutes( isPage, routeSuffix, }); - }); + } + ; - items.forEach((item) => { + for (const item of items) { const segments = parentSegments.slice(); if (item.isIndex) { @@ -306,7 +327,8 @@ function createFileBasedRoutes( fallbackRoutes: [], }); } - }); + } + ; } const {config} = settings; @@ -329,12 +351,12 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior const prerender = getPrerenderDefault(config); const routes: PrioritizedRoutesData = { - 'override': [], 'normal': [], - 'defer': [], + 'legacy': [], }; - settings.injectedRoutes.forEach(({pattern: name, entrypoint, prerender: prerenderInjected, priority}) => { + for (const injectedRoute of settings.injectedRoutes) { + const {pattern: name, entrypoint, prerender: prerenderInjected, priority} = injectedRoute; let resolved: string; try { resolved = require.resolve(entrypoint, {paths: [cwd || fileURLToPath(config.root)]}); @@ -368,9 +390,7 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - // If an injected route doesn't define a priority, it will override - // file-based routes from the project for backwards compatibility. - routes[priority ?? 'override'].push({ + routes[priority ?? 'normal'].push({ type, route, pattern, @@ -382,26 +402,29 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior prerender: prerenderInjected ?? prerender, fallbackRoutes: [], }); - }); + } + ; 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 = { - 'override': [], 'normal': [], - 'defer': [], + 'legacy': [], }; - Object.entries(settings.config.redirects).forEach(([from, to]) => { - const trailingSlash = config.trailingSlash; - + for (const [from, to] of Object.entries(settings.config.redirects)) { const segments = removeLeadingForwardSlash(from) .split(path.posix.sep) .filter(Boolean) @@ -423,7 +446,6 @@ function createRedirectRoutes( .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - if (/^https?:\/\//.test(to.destination)) { logger.warn( 'redirects', @@ -450,67 +472,148 @@ function createRedirectRoutes( redirectRoute: routeMap.get(to.destination), fallbackRoutes: [], }); - }); + } return routes; } -function routesCollide(a: RouteData, b: RouteData) { - if (a.segments.length !== b.segments.length) { + +/** + * Checks whether a route segment is static. + */ +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()) { + 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 routes collide and report appropriately. + * + * Routes the are statically guaranteed to collide will throw. + * Routes that may collide depending on the parameters returned by `getStaticPaths` will log a warning. + * Routes that may collide depending on whether they are prerendered will log a warning. + * + * Fallback routes are never considered to collide with any other route. + * + * Two routes are guarantted to collide, thus throwing an error, 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]`. + * + * Two routes may collide after building, thus showing a warning, in the following scenarios: + * - Both are dynamic routes with equal static parts in matching positions, + * dynamic parts in the same positions, and at least one of them is prerendered. + * For example, `/foo/[bar]` and `/foo/[baz]` or `/foo/[...bar]` and `/foo/[...baz]`. + * - Both routes share a common prefix and one of them is prerendered and follows the + * prefix with a rest parameter. + * For example, with a prefix `/foo`: + * - `/foo/[...bar]` and `/foo/baz`. + * - `/foo/[...bar]` and `/foo/baz/[qux]`. + * - `/foo/[...bar]` and `/foo/baz/[...qux]`. + */ +function detectRouteColision(a: RouteData, b: RouteData, 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 false; } - const length = a.segments.length; - let hasDynamic = false; + if (a.route === b.route && a.segments.every(isStaticSegment) && b.segments.every(isStaticSegment)) { + // If both routes are exactly the same, they collide + // @todo Make this a pretty AstroError + throw new Error(`The route ${a.route} is defined more than once`); + } - for (let i = 0; i < length; i++) { - const segmentA = a.segments[i]; - const segmentB = b.segments[i]; + // Assume the routes collide for sure and update this assumptions if any rule says otherwise. + let certainCollision = true; - if (segmentA.length !== segmentB.length) { - return false; + // Take the minimum of both length to iterate on + // If one route is longer than the other, the segments exceeding the length + // of the other route won't be paired. + const minLength = Math.min(a.segments.length, b.segments.length); + + let index = 0; + for (; index < minLength; index++) { + const segmentA = a.segments[index]; + const segmentB = b.segments[index]; + + if (isSemanticallyEqualSegment(segmentA, segmentB)) { + // Semantically equal segments don't change our assumptions + continue; } - const segmentLength = segmentA.length; + // If any segment is not semantically equal between the routes + // it is not certain that the routes collide. + certainCollision = false; - for (let j = 0; j < segmentLength; j++) { - const partA = segmentA[j]; - const partB = segmentB[j]; + if (isStaticSegment(segmentA) && isStaticSegment(segmentB)) { + // If both segments are static and not equal then the routes + // don't collide, e.g. `/foo/[bar]` and `/baz/[qux]`. + return; + } - if ( - // If only one side has a rest parameter, they don't collide - partA.spread !== partB.spread - // If only one side is dynamic, they don't collide - || partA.dynamic !== partB.dynamic - ) { - return false; - } + // From this segment on, the routes do not match one-to-one. + break; + } - hasDynamic = hasDynamic || partA.dynamic; + if (index === minLength - 1) { + const aEndsWithSpread = a.segments.at(-1)?.[0].spread === true; + const bEndsWithSpread = b.segments.at(-1)?.[0].spread === true; - // If both sides are static and have different content, they don't collide - if (!partA.dynamic && partA.content !== partB.content) { - return false; - } + // If only one of the routes ends with a spread + if (aEndsWithSpread !== bEndsWithSpread) { + const restRoute = aEndsWithSpread ? a : b; + // const possibleColisionRoute = aEndsWithSpread ? b : a; + + logger.warn('router', `The route ${restRoute.route} is defined more than once`); } } - if (hasDynamic && (a.prerender || b.prerender)) { - // If the route is dynamic and either of the routes is prerendered, - // we cannot afirm that they collide at this point. - // They will only collide if both routes are prerendered and return - // the same parameters on `getStaticPaths`, which is checked later. - return false; - } + // if (hasDynamic && (a.prerender || b.prerender)) { + // // If the route is dynamic and either of the routes is prerendered, + // // we cannot afirm that they collide at this point. + // // They will only collide if both routes are prerendered and return + // // the same parameters on `getStaticPaths`, which is checked later. + // return false; + // } // If every segment and part is equivalent, they collide - return true; + // return true; } /** Create manifest of all static routes */ @@ -522,49 +625,35 @@ export function createRouteManifest( const routeMap = new Map(); const fileBasedRoutes = createFileBasedRoutes(params, logger); - fileBasedRoutes.forEach((route) => { + for (const route of fileBasedRoutes) { routeMap.set(route.route, route); - }); + } const injectedRoutes = createInjectedRoutes(params); - Object.entries(injectedRoutes).forEach(([, routes]) => { - routes.forEach((route) => { + 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['override'], - ...redirectRoutes['override'], - ].sort(comparator), + ...injectedRoutes['legacy'].sort(routeComparator), ...[ ...fileBasedRoutes, ...injectedRoutes['normal'], ...redirectRoutes['normal'], - ].sort(comparator), - ...[ - ...injectedRoutes['defer'], - ...redirectRoutes['defer'], - ].sort(comparator), + ].sort(routeComparator), + ...redirectRoutes['legacy'].sort(routeComparator), ]; - // Detect route collisions - routes.forEach((route, index) => { - const collision = routes.find( - (other, otherIndex) => (index !== otherIndex && routesCollide(route, other)), - ); - - if (collision) { - logger.warn( - 'router', - `Colliding routes detected in the project: "${route.route}" at "${route.component}".\n` - + `This route collides with: "${collision.component}".`, - ); + // Report route collisions + for (const [index, higherRoute] of routes.entries()) { + for (const lowerRoute of routes.slice(index + 1)) { + detectRouteColision(higherRoute, lowerRoute, logger) } - }); + } const {settings} = params; const {config} = settings; diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 4022ab29b702..59192dacb223 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -345,6 +345,10 @@ describe('routing - createRouteManifest', () => { route: '/blog/contributing', type: 'page', }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, { route: '/', type: 'page', @@ -353,10 +357,6 @@ describe('routing - createRouteManifest', () => { route: '/blog/about', type: 'redirect', }, - { - route: '/blog/[...slug]', - type: 'redirect', - }, ]); }); From addd2d950404deda27f0cdd898f90b902490864f Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Sat, 13 Jan 2024 16:21:04 -0300 Subject: [PATCH 11/32] Refactor --- packages/astro/src/assets/endpoint/config.ts | 1 - packages/astro/src/core/errors/errors-data.ts | 27 +++ .../astro/src/core/routing/manifest/create.ts | 111 ++++----- .../astro/test/units/routing/manifest.test.js | 211 +++++++----------- 4 files changed, 153 insertions(+), 197 deletions(-) diff --git a/packages/astro/src/assets/endpoint/config.ts b/packages/astro/src/assets/endpoint/config.ts index b56d33246284..07cfe8faecbb 100644 --- a/packages/astro/src/assets/endpoint/config.ts +++ b/packages/astro/src/assets/endpoint/config.ts @@ -9,7 +9,6 @@ export function injectImageEndpoint(settings: AstroSettings, mode: 'dev' | 'buil pattern: '/_image', entrypoint: endpointEntrypoint, prerender: false, - priority: 'override', }); return settings; diff --git a/packages/astro/src/core/errors/errors-data.ts b/packages/astro/src/core/errors/errors-data.ts index fb463a77fe1c..a62da250a178 100644 --- a/packages/astro/src/core/errors/errors-data.ts +++ b/packages/astro/src/core/errors/errors-data.ts @@ -1034,6 +1034,33 @@ export const UnhandledRejection = { hint: 'Make sure your promises all have an `await` or a `.catch()` handler.', } satisfies ErrorData; +/** + * @docs + * @description + * A static route was defined more than once. This can happen when an Astro integration injects a static route tha is also used in the project or when a static redirect is defined for a route that has a page or endpoint associated with it. + */ +export const StaticRouteCollision = { + name: 'StaticRouteCollision', + title: 'Static route collision', + message: (route: string, sourceA: string, sourceB: string) => + `The route "${route}" is defined in both "${sourceA}" and "${sourceB}". A static route cannot be defined more than once.`, +} satisfies ErrorData; + +/** + * @docs + * @see + * - [Dynamic HTML](https://docs.astro.build/en/core-concepts/astro-syntax/#dynamic-html') + * @description + * A dynamic route was defined more than once in SSR mode. This can happen when using two names for the dynamic parameters in the same positions, like `/foo/[bar]` and `/foo/[baz]`. If you can know at build time which paths you want to render with one of them, you can opt-in to pre-rendering in one of them to resolve the conflict. If which definition should be used is not known at build time, you need to select what to show within a single definition. + */ +export const DynamicRouteCollision = { + name: 'DynamicRouteCollision', + title: 'Dynamic route collision', + message: (route: string, sourceA: string, sourceB: string) => + `The route "${route}" is defined in both "${sourceA}" and "${sourceB}" using SSR mode. A dynamic SSR route cannot be defined more than once.`, + hint: 'If some paths should be handled by one of the definitions and all other paths by the other, you can opt-in to pre-rendering in one of them to resolve the conflict. If which definition should be used is not known at build time, you need to select what to show within a single definition.\nSee https://docs.astro.build/en/core-concepts/astro-syntax/#dynamic-html', +} satisfies ErrorData; + /** * @docs * @kind heading diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 5d34609d8f15..52367d59ac7a 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -18,8 +18,12 @@ import { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from '../../constants.js'; import { removeLeadingForwardSlash, slash } from '../../path.js'; import { resolvePages } from '../../util.js'; import { getRouteGenerator } from './generator.js'; -import { AstroError } from '../../errors/index.js'; -import { MissingIndexForInternationalization } from '../../errors/errors-data.js'; +import {AstroError, AstroErrorData} from '../../errors/index.js'; +import { + MissingIndexForInternationalization, + StaticRouteCollision, + DynamicRouteCollision +} from '../../errors/errors-data.js'; const require = createRequire(import.meta.url); interface Item { @@ -500,6 +504,7 @@ function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[] } 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) { @@ -518,101 +523,67 @@ function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[] } /** - * Check whether two routes collide and report appropriately. - * - * Routes the are statically guaranteed to collide will throw. - * Routes that may collide depending on the parameters returned by `getStaticPaths` will log a warning. - * Routes that may collide depending on whether they are prerendered will log a warning. + * 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, thus throwing an error, in the following scenarios: + * 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]`. - * - * Two routes may collide after building, thus showing a warning, in the following scenarios: - * - Both are dynamic routes with equal static parts in matching positions, - * dynamic parts in the same positions, and at least one of them is prerendered. - * For example, `/foo/[bar]` and `/foo/[baz]` or `/foo/[...bar]` and `/foo/[...baz]`. - * - Both routes share a common prefix and one of them is prerendered and follows the - * prefix with a rest parameter. - * For example, with a prefix `/foo`: - * - `/foo/[...bar]` and `/foo/baz`. - * - `/foo/[...bar]` and `/foo/baz/[qux]`. - * - `/foo/[...bar]` and `/foo/baz/[...qux]`. */ -function detectRouteColision(a: RouteData, b: RouteData, logger: Logger) { +function detectRouteColision(a: RouteData, b: RouteData) { 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 false; + return; } if (a.route === b.route && a.segments.every(isStaticSegment) && b.segments.every(isStaticSegment)) { - // If both routes are exactly the same, they collide - // @todo Make this a pretty AstroError - throw new Error(`The route ${a.route} is defined more than once`); + // If both routes are the same and completely static they are guaranteed to collide + // such that one of them will never be matched. + throw new AstroError({ + ...StaticRouteCollision, + message: StaticRouteCollision.message(a.route, a.component, b.component), + }); + } + + 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 } - // Assume the routes collide for sure and update this assumptions if any rule says otherwise. - let certainCollision = true; + 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; + } - // Take the minimum of both length to iterate on - // If one route is longer than the other, the segments exceeding the length - // of the other route won't be paired. - const minLength = Math.min(a.segments.length, b.segments.length); + // Routes have the same number of segments, can use either. + const segmentCount = a.segments.length; - let index = 0; - for (; index < minLength; index++) { + for (let index = 0; index < segmentCount; index++) { const segmentA = a.segments[index]; const segmentB = b.segments[index]; - if (isSemanticallyEqualSegment(segmentA, segmentB)) { - // Semantically equal segments don't change our assumptions - continue; - } - - // If any segment is not semantically equal between the routes - // it is not certain that the routes collide. - certainCollision = false; - - if (isStaticSegment(segmentA) && isStaticSegment(segmentB)) { - // If both segments are static and not equal then the routes - // don't collide, e.g. `/foo/[bar]` and `/baz/[qux]`. + if (!isSemanticallyEqualSegment(segmentA, segmentB)) { + // If any segment is not semantically equal between the routes + // it is not certain that the routes collide. return; } - - // From this segment on, the routes do not match one-to-one. - break; } - if (index === minLength - 1) { - const aEndsWithSpread = a.segments.at(-1)?.[0].spread === true; - const bEndsWithSpread = b.segments.at(-1)?.[0].spread === true; - - // If only one of the routes ends with a spread - if (aEndsWithSpread !== bEndsWithSpread) { - const restRoute = aEndsWithSpread ? a : b; - // const possibleColisionRoute = aEndsWithSpread ? b : a; - - logger.warn('router', `The route ${restRoute.route} is defined more than once`); - } - } - - // if (hasDynamic && (a.prerender || b.prerender)) { - // // If the route is dynamic and either of the routes is prerendered, - // // we cannot afirm that they collide at this point. - // // They will only collide if both routes are prerendered and return - // // the same parameters on `getStaticPaths`, which is checked later. - // return false; - // } - - // If every segment and part is equivalent, they collide - // return true; + // Both routes are guaranteed to collide such that one will never be matched. + throw new AstroError({ + ...DynamicRouteCollision, + message: DynamicRouteCollision.message(a.route, a.component, b.component), + }); } /** Create manifest of all static routes */ diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 59192dacb223..7f18f87a635e 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -3,6 +3,7 @@ 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); @@ -53,11 +54,11 @@ describe('routing - createRouteManifest', () => { settings.injectedRoutes = [ { pattern: '/about', - entrypoint: '@lib/override/static.astro', + entrypoint: '@lib/legacy/static.astro', }, { pattern: '/api', - entrypoint: '@lib/override/static.ts', + entrypoint: '@lib/legacy/static.ts', }, ]; @@ -72,76 +73,22 @@ describe('routing - createRouteManifest', () => { route: '/api', type: 'endpoint', }, - { - route: '/about', - type: 'page', - }, { route: '/sendcontact', type: 'endpoint', }, { - route: '/contact-me', - type: 'page', - }, - ]); - }); - - it('injected routes are sorted as specified 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/override/static.astro', - priority: 'override', - }, - { - pattern: '/[...slug]', - entrypoint: '@lib/override/dynamic.astro', - // Don's specify a priority to test that it defaults to override - }, - ]; - - 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]', + route: '/about', type: 'page', }, { - route: '/', + route: '/contact-me', type: 'page', }, ]); }); - it('injected routes are sorted as specified below filesystem routes', async () => { + it('injected routes are sorted in legacy mode above filesystem routes', async () => { const fs = createFs( { '/src/pages/index.astro': `

test

`, @@ -159,13 +106,12 @@ describe('routing - createRouteManifest', () => { settings.injectedRoutes = [ { pattern: '/contributing', - entrypoint: '@lib/override/static.astro', - priority: 'defer', + entrypoint: '@lib/legacy/static.astro', }, { pattern: '/[...slug]', - entrypoint: '@lib/override/dynamic.astro', - priority: 'defer', + entrypoint: '@lib/legacy/dynamic.astro', + priority: 'legacy', }, ]; @@ -177,11 +123,11 @@ describe('routing - createRouteManifest', () => { expect(getManifestRoutes(manifest)).to.deep.equal([ { - route: '/blog/[...slug]', + route: '/[...slug]', type: 'page', }, { - route: '/', + route: '/blog/[...slug]', type: 'page', }, { @@ -189,13 +135,13 @@ describe('routing - createRouteManifest', () => { type: 'page', }, { - route: '/[...slug]', + route: '/', type: 'page', }, ]); }); - it('injected routes are sorted as specified alongside filesystem routes', async () => { + it('injected routes are sorted alongside filesystem routes', async () => { const fs = createFs( { '/src/pages/index.astro': `

test

`, @@ -222,12 +168,11 @@ describe('routing - createRouteManifest', () => { settings.injectedRoutes = [ { pattern: '/contributing', - entrypoint: '@lib/override/static.astro', - priority: 'normal', + entrypoint: '@lib/legacy/static.astro', }, { pattern: '/[...slug]', - entrypoint: '@lib/override/dynamic.astro', + entrypoint: '@lib/legacy/dynamic.astro', priority: 'normal', }, ]; @@ -258,7 +203,7 @@ describe('routing - createRouteManifest', () => { ]); }); - it('redirects are sorted as specified above the filesystem routes', async () => { + it('redirects are sorted in legacy mode below the filesystem routes', async () => { const fs = createFs( { '/src/pages/index.astro': `

test

`, @@ -272,16 +217,11 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', redirects: { - // Do not specify a priority to test that it defaults to defer - '/blog/[...slug]': { - status: 302, - destination: '/', - priority: 'override', - }, + '/blog/[...slug]': '/', '/blog/about': { status: 302, destination: '/another', - priority: 'override', + priority: 'legacy', } }, }); @@ -293,25 +233,25 @@ describe('routing - createRouteManifest', () => { expect(getManifestRoutes(manifest)).to.deep.equal([ { - route: '/blog/about', - type: 'redirect', + route: '/blog/contributing', + type: 'page', }, { route: '/blog/[...slug]', type: 'redirect', }, { - route: '/blog/contributing', + route: '/', type: 'page', }, { - route: '/', - type: 'page', + route: '/blog/about', + type: 'redirect', }, ]); }); - it('redirects are sorted as specified below the filesystem routes', async () => { + it('redirects are sorted alongside the filesystem routes', async () => { const fs = createFs( { '/src/pages/index.astro': `

test

`, @@ -325,12 +265,15 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', redirects: { - // Do not specify a priority to test that it defaults to defer - '/blog/[...slug]': '/', + '/blog/[...slug]': { + status: 302, + destination: '/', + priority: 'normal', + }, '/blog/about': { status: 302, destination: '/another', - priority: 'defer', + priority: 'normal', } }, }); @@ -341,6 +284,10 @@ describe('routing - createRouteManifest', () => { }); expect(getManifestRoutes(manifest)).to.deep.equal([ + { + route: '/blog/about', + type: 'redirect', + }, { route: '/blog/contributing', type: 'page', @@ -353,62 +300,74 @@ describe('routing - createRouteManifest', () => { route: '/', type: 'page', }, - { - route: '/blog/about', - type: 'redirect', - }, ]); }); - it('redirects are sorted as specified alongside the filesystem routes', async () => { + it('rejects colliding static routes', async () => { const fs = createFs( { - '/src/pages/index.astro': `

test

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

test

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

test

`, }, - root + root, ); const settings = await createBasicSettings({ root: fileURLToPath(root), output: 'server', base: '/search', trailingSlash: 'never', - redirects: { - '/blog/[...slug]': { - status: 302, - destination: '/', - priority: 'normal', - }, - '/blog/about': { - status: 302, - destination: '/another', - priority: 'normal', - } - }, - }); - const manifest = createRouteManifest({ - cwd: fileURLToPath(root), - settings, - fsMod: fs, + integrations: [], }); - expect(getManifestRoutes(manifest)).to.deep.equal([ - { - route: '/blog/about', - type: 'redirect', - }, - { - route: '/blog/contributing', - type: 'page', - }, + settings.injectedRoutes = [ { - route: '/blog/[...slug]', - type: 'redirect', + pattern: '/contributing', + entrypoint: '@lib/legacy/static.astro', }, + ]; + + try { + createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + expect.fail(0, 1, 'Expected createRouteManifest to throw'); + } catch (e) { + expect(e).to.be.instanceOf(Error); + expect(e.type).to.equal('AstroError'); + expect(e.name).to.equal('StaticRouteCollision'); + expect(e.message).to.equal('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.'); + } + }); + + it('rejects colliding SSR dynamic routes', async () => { + const fs = createFs( { - route: '/', - type: 'page', + '/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: [], + }); + + try { + createRouteManifest({ + cwd: fileURLToPath(root), + settings, + fsMod: fs, + }); + expect.fail(0, 1, 'Expected createRouteManifest to throw'); + } catch (e) { + expect(e).to.be.instanceOf(Error); + expect(e.type).to.equal('AstroError'); + expect(e.name).to.equal('DynamicRouteCollision'); + expect(e.message).to.equal('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.'); + } }); }); From e3cbbe949f49acf4594bb8037eca45f2b6fdd6f1 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Sun, 14 Jan 2024 18:18:06 -0300 Subject: [PATCH 12/32] Fix typo and typing --- packages/astro/src/core/routing/manifest/create.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 52367d59ac7a..0133d7306a48 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -537,7 +537,7 @@ function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[] * For example, `/foo/[bar]` and `/foo/[baz]` or `/foo/[...bar]` and `/foo/[...baz]` * but not `/foo/[bar]` and `/foo/[...baz]`. */ -function detectRouteColision(a: RouteData, b: RouteData) { +function detectRouteCollision(a: RouteData, b: RouteData) { 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. @@ -621,7 +621,7 @@ export function createRouteManifest( // Report route collisions for (const [index, higherRoute] of routes.entries()) { for (const lowerRoute of routes.slice(index + 1)) { - detectRouteColision(higherRoute, lowerRoute, logger) + detectRouteCollision(higherRoute, lowerRoute) } } From 322bb4d2417cb630d81f333624b6d354208ec91c Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 12:07:48 +0000 Subject: [PATCH 13/32] chore: default to legacy --- packages/astro/src/core/config/schema.ts | 40 ++++++++++++------------ 1 file changed, 20 insertions(+), 20 deletions(-) diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 16192e659348..072b38a578e5 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -163,28 +163,28 @@ export const AstroConfigSchema = z.object({ redirects: z .record( z.string(), - z.union([ - z.string(), - z.object({ - status: z.union([ - z.literal(300), - z.literal(301), - z.literal(302), - z.literal(303), - z.literal(304), - z.literal(307), - z.literal(308), - ]), - destination: z.string(), - priority: z.enum(['normal', 'legacy']) - .default('normal'), - }), - ]) - .transform(redirect => ( + z + .union([ + z.string(), + z.object({ + status: z.union([ + z.literal(300), + z.literal(301), + z.literal(302), + z.literal(303), + z.literal(304), + z.literal(307), + z.literal(308), + ]), + destination: z.string(), + priority: z.enum(['normal', 'legacy']).default('legacy'), + }), + ]) + .transform((redirect) => typeof redirect === 'string' - ? {destination: redirect, priority: 'normal' as const} + ? { destination: redirect, priority: 'normal' as const } : redirect - )) + ) ) .default(ASTRO_CONFIG_DEFAULTS.redirects), prefetch: z From 28df92fa1f235464376739aa100d0cfc6c586c70 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 13:23:07 +0000 Subject: [PATCH 14/32] chore: use experimental flag instead of option --- packages/astro/src/@types/astro.ts | 17 ++- packages/astro/src/core/config/schema.ts | 42 ++++--- .../astro/src/core/routing/manifest/create.ts | 110 ++++++++++-------- 3 files changed, 98 insertions(+), 71 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 9909bdd6227a..6eefb202e265 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1566,6 +1566,19 @@ export interface AstroUserConfig { * ``` */ contentCollectionCache?: boolean; + + /** + * @docs + * @name experimental.stableRoutingPriority + * @type {boolean} + * @default `false` + * @version 4.2.0 + * @description + * + * + * Merge with discovered file-based project routes, behaving the same as if the route was defined as a file in the project. + */ + stableRoutingPriority?: boolean; }; } @@ -1589,12 +1602,12 @@ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | /** * IDs for different priorities of injected routes and redirects: - * - "normal": Merge with discovered file-based project routes, behaving the same as if the route + * - "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 overriden by any project route on conflict. */ -export type RoutePriorityOverride = 'normal' | 'legacy'; +export type RoutePriorityOverride = 'normal' | 'legacy'; export interface InjectedRoute { pattern: string; diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 072b38a578e5..930050b78705 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -58,6 +58,7 @@ const ASTRO_CONFIG_DEFAULTS = { experimental: { optimizeHoistedScript: false, contentCollectionCache: false, + stableRoutingPriority: false, }, } satisfies AstroUserConfig & { server: { open: boolean } }; @@ -163,28 +164,21 @@ export const AstroConfigSchema = z.object({ redirects: z .record( z.string(), - z - .union([ - z.string(), - z.object({ - status: z.union([ - z.literal(300), - z.literal(301), - z.literal(302), - z.literal(303), - z.literal(304), - z.literal(307), - z.literal(308), - ]), - destination: z.string(), - priority: z.enum(['normal', 'legacy']).default('legacy'), - }), - ]) - .transform((redirect) => - typeof redirect === 'string' - ? { destination: redirect, priority: 'normal' as const } - : redirect - ) + z.union([ + z.string(), + z.object({ + status: z.union([ + z.literal(300), + z.literal(301), + z.literal(302), + z.literal(303), + z.literal(304), + z.literal(307), + z.literal(308), + ]), + destination: z.string(), + }), + ]) ) .default(ASTRO_CONFIG_DEFAULTS.redirects), prefetch: z @@ -400,6 +394,10 @@ export const AstroConfigSchema = z.object({ .boolean() .optional() .default(ASTRO_CONFIG_DEFAULTS.experimental.contentCollectionCache), + stableRoutingPriority: z + .boolean() + .optional() + .default(ASTRO_CONFIG_DEFAULTS.experimental.stableRoutingPriority), }) .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 0133d7306a48..ee16a3b4ceca 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -18,12 +18,13 @@ import { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from '../../constants.js'; import { removeLeadingForwardSlash, slash } from '../../path.js'; import { resolvePages } from '../../util.js'; import { getRouteGenerator } from './generator.js'; -import {AstroError, AstroErrorData} from '../../errors/index.js'; +import { AstroError, AstroErrorData } from '../../errors/index.js'; import { MissingIndexForInternationalization, StaticRouteCollision, - DynamicRouteCollision + DynamicRouteCollision, } from '../../errors/errors-data.js'; +import Astro from '../../../runtime/client/dev-toolbar/apps/astro.js'; const require = createRequire(import.meta.url); interface Item { @@ -163,8 +164,12 @@ function routeComparator(a: RouteData, b: RouteData) { return a.segments.length > b.segments.length ? -1 : 1; } - 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)); + 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) + ); // Sort static routes before dynamic routes if (aIsStatic !== bIsStatic) { @@ -204,8 +209,8 @@ export interface CreateRouteManifestParams { } function createFileBasedRoutes( - {settings, cwd, fsMod}: CreateRouteManifestParams, - logger: Logger, + { settings, cwd, fsMod }: CreateRouteManifestParams, + logger: Logger ): RouteData[] { const components: string[] = []; const routes: RouteData[] = []; @@ -222,7 +227,7 @@ function createFileBasedRoutes( fs: typeof nodeFs, dir: string, parentSegments: RoutePart[][], - parentParams: string[], + parentParams: string[] ) { let items: Item[] = []; for (const basename of fs.readdirSync(dir)) { @@ -244,8 +249,8 @@ function createFileBasedRoutes( logger.warn( null, `Unsupported file type ${bold( - resolved, - )} found. Prefix filename with an underscore (\`_\`) to ignore.`, + resolved + )} found. Prefix filename with an underscore (\`_\`) to ignore.` ); return; @@ -317,7 +322,7 @@ function createFileBasedRoutes( ? `/${segments.map((segment) => segment[0].content).join('/')}` : null; const route = `/${segments - .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) + .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); routes.push({ route, @@ -335,7 +340,7 @@ function createFileBasedRoutes( } } - const {config} = settings; + const { config } = settings; const pages = resolvePages(config); if (localFs.existsSync(pages)) { @@ -350,20 +355,20 @@ function createFileBasedRoutes( type PrioritizedRoutesData = Record; -function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): PrioritizedRoutesData { - const {config} = settings; +function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): PrioritizedRoutesData { + const { config } = settings; const prerender = getPrerenderDefault(config); const routes: PrioritizedRoutesData = { - 'normal': [], - 'legacy': [], + normal: [], + legacy: [], }; for (const injectedRoute of settings.injectedRoutes) { - const {pattern: name, entrypoint, prerender: prerenderInjected, priority} = injectedRoute; + const { pattern: name, entrypoint, prerender: prerenderInjected, priority } = injectedRoute; let resolved: string; try { - resolved = require.resolve(entrypoint, {paths: [cwd || fileURLToPath(config.root)]}); + resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(config.root)] }); } catch (e) { resolved = fileURLToPath(new URL(entrypoint, config.root)); } @@ -391,7 +396,7 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior .filter((p) => p.dynamic) .map((p) => p.content); const route = `/${segments - .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) + .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); routes[priority ?? 'normal'].push({ @@ -415,19 +420,20 @@ function createInjectedRoutes({settings, cwd}: CreateRouteManifestParams): Prior * Create route data for all configured redirects. */ function createRedirectRoutes( - {settings}: CreateRouteManifestParams, + { settings }: CreateRouteManifestParams, routeMap: Map, - logger: Logger, + logger: Logger ): PrioritizedRoutesData { - const {config} = settings; + const { config } = settings; const trailingSlash = config.trailingSlash; const routes: PrioritizedRoutesData = { - 'normal': [], - 'legacy': [], + normal: [], + legacy: [], }; for (const [from, to] of Object.entries(settings.config.redirects)) { + const priority = computeRoutePriority(settings.config); const segments = removeLeadingForwardSlash(from) .split(path.posix.sep) .filter(Boolean) @@ -446,17 +452,24 @@ function createRedirectRoutes( .filter((p) => p.dynamic) .map((p) => p.content); const route = `/${segments - .map(([{dynamic, content}]) => (dynamic ? `[${content}]` : content)) + .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - if (/^https?:\/\//.test(to.destination)) { + 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} -> ${to.destination}`, + `Redirecting to an external URL is not officially supported: ${from} -> ${destination}` ); } - routes[to.priority].push({ + routes[priority].push({ type: 'redirect', route, pattern, @@ -466,13 +479,8 @@ function createRedirectRoutes( generate, pathname: pathname || void 0, prerender: false, - redirect: 'status' in to - ? { - status: to.status, - destination: to.destination, - } - : to.destination, - redirectRoute: routeMap.get(to.destination), + redirect: to, + redirectRoute: routeMap.get(destination), fallbackRoutes: [], }); } @@ -480,7 +488,6 @@ function createRedirectRoutes( return routes; } - /** * Checks whether a route segment is static. */ @@ -512,7 +519,7 @@ function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[] } // Only compare the content on non-dynamic segments - // `/[bar]` and `/[baz]` are effectively the same route, + // `/[bar]` and `/[baz]` are effectively the same route, // only bound to a different path parameter. if (!partA.dynamic && partA.content !== partB.content) { return false; @@ -544,7 +551,11 @@ function detectRouteCollision(a: RouteData, b: RouteData) { return; } - if (a.route === b.route && a.segments.every(isStaticSegment) && b.segments.every(isStaticSegment)) { + 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. throw new AstroError({ @@ -556,7 +567,7 @@ function detectRouteCollision(a: RouteData, b: RouteData) { 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 + return; } if (a.segments.length !== b.segments.length) { @@ -589,7 +600,7 @@ function detectRouteCollision(a: RouteData, b: RouteData) { /** Create manifest of all static routes */ export function createRouteManifest( params: CreateRouteManifestParams, - logger: Logger, + logger: Logger ): ManifestData { // Create a map of all routes so redirects can refer to any route const routeMap = new Map(); @@ -610,23 +621,21 @@ export function createRouteManifest( const routes: RouteData[] = [ ...injectedRoutes['legacy'].sort(routeComparator), - ...[ - ...fileBasedRoutes, - ...injectedRoutes['normal'], - ...redirectRoutes['normal'], - ].sort(routeComparator), + ...[...fileBasedRoutes, ...injectedRoutes['normal'], ...redirectRoutes['normal']].sort( + routeComparator + ), ...redirectRoutes['legacy'].sort(routeComparator), ]; // Report route collisions for (const [index, higherRoute] of routes.entries()) { for (const lowerRoute of routes.slice(index + 1)) { - detectRouteCollision(higherRoute, lowerRoute) + detectRouteCollision(higherRoute, lowerRoute); } } - const {settings} = params; - const {config} = settings; + const { settings } = params; + const { config } = settings; const i18n = settings.config.i18n; if (i18n) { @@ -819,3 +828,10 @@ export function createRouteManifest( routes, }; } + +function computeRoutePriority(config: AstroConfig): RoutePriorityOverride { + if (config.experimental.stableRoutingPriority) { + return 'normal'; + } + return 'legacy'; +} From a368d607e5af40d6530afe49f3648850484d3527 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 13:33:13 +0000 Subject: [PATCH 15/32] fix: do not throw an error on collisions --- .../astro/src/core/routing/manifest/create.ts | 25 +++++++++++-------- 1 file changed, 14 insertions(+), 11 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index ee16a3b4ceca..6a9474dd3b86 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -21,10 +21,8 @@ import { getRouteGenerator } from './generator.js'; import { AstroError, AstroErrorData } from '../../errors/index.js'; import { MissingIndexForInternationalization, - StaticRouteCollision, DynamicRouteCollision, } from '../../errors/errors-data.js'; -import Astro from '../../../runtime/client/dev-toolbar/apps/astro.js'; const require = createRequire(import.meta.url); interface Item { @@ -544,7 +542,7 @@ function isSemanticallyEqualSegment(segmentA: RoutePart[], segmentB: RoutePart[] * 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) { +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. @@ -558,10 +556,16 @@ function detectRouteCollision(a: RouteData, b: RouteData) { ) { // If both routes are the same and completely static they are guaranteed to collide // such that one of them will never be matched. - throw new AstroError({ - ...StaticRouteCollision, - message: StaticRouteCollision.message(a.route, a.component, b.component), - }); + if (config.experimental.stableRoutingPriority) { + 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.' + ); + } } if (a.prerender || b.prerender) { @@ -602,6 +606,8 @@ 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(); @@ -630,13 +636,10 @@ export function createRouteManifest( // Report route collisions for (const [index, higherRoute] of routes.entries()) { for (const lowerRoute of routes.slice(index + 1)) { - detectRouteCollision(higherRoute, lowerRoute); + detectRouteCollision(higherRoute, lowerRoute, config, logger); } } - const { settings } = params; - const { config } = settings; - const i18n = settings.config.i18n; if (i18n) { // First we check if the user doesn't have an index page. From dc2760aa9518a1b5b22b78420ac53eb73680d102 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 14:17:33 +0000 Subject: [PATCH 16/32] chore: fix regression --- packages/astro/src/@types/astro.ts | 1 - .../astro/src/core/routing/manifest/create.ts | 8 ++-- .../astro/test/units/routing/manifest.test.js | 46 ++++++++++--------- 3 files changed, 30 insertions(+), 25 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 6eefb202e265..3175f7de3878 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1613,7 +1613,6 @@ export interface InjectedRoute { pattern: string; entrypoint: string; prerender?: boolean; - priority?: RoutePriorityOverride; } export interface ResolvedInjectedRoute extends InjectedRoute { diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 6a9474dd3b86..eb5ad3469224 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -362,8 +362,10 @@ function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): Pri legacy: [], }; + const priority = computeRoutePriority(config); + for (const injectedRoute of settings.injectedRoutes) { - const { pattern: name, entrypoint, prerender: prerenderInjected, priority } = injectedRoute; + const { pattern: name, entrypoint, prerender: prerenderInjected } = injectedRoute; let resolved: string; try { resolved = require.resolve(entrypoint, { paths: [cwd || fileURLToPath(config.root)] }); @@ -397,7 +399,7 @@ function createInjectedRoutes({ settings, cwd }: CreateRouteManifestParams): Pri .map(([{ dynamic, content }]) => (dynamic ? `[${content}]` : content)) .join('/')}`.toLowerCase(); - routes[priority ?? 'normal'].push({ + routes[priority].push({ type, route, pattern, @@ -430,8 +432,8 @@ function createRedirectRoutes( legacy: [], }; + const priority = computeRoutePriority(settings.config); for (const [from, to] of Object.entries(settings.config.redirects)) { - const priority = computeRoutePriority(settings.config); const segments = removeLeadingForwardSlash(from) .split(path.posix.sep) .filter(Boolean) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 7f18f87a635e..a076ed81f902 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -3,12 +3,12 @@ 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"; +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 => ({ + return manifest.routes.map((route) => ({ type: route.type, route: route.route, })); @@ -43,7 +43,7 @@ describe('routing - createRouteManifest', () => { '/src/pages/contact-me.astro': `

test

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

test

`, }, - root, + root ); const settings = await createBasicSettings({ root: fileURLToPath(root), @@ -94,7 +94,7 @@ describe('routing - createRouteManifest', () => { '/src/pages/index.astro': `

test

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

test

`, }, - root, + root ); const settings = await createBasicSettings({ root: fileURLToPath(root), @@ -147,7 +147,7 @@ describe('routing - createRouteManifest', () => { '/src/pages/index.astro': `

test

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

test

`, }, - root, + root ); const settings = await createBasicSettings({ root: fileURLToPath(root), @@ -158,8 +158,7 @@ describe('routing - createRouteManifest', () => { { name: '@test', hooks: { - 'astro:config:setup': ({injectRoute}) => { - }, + 'astro:config:setup': ({ injectRoute }) => {}, }, }, ], @@ -221,8 +220,7 @@ describe('routing - createRouteManifest', () => { '/blog/about': { status: 302, destination: '/another', - priority: 'legacy', - } + }, }, }); const manifest = createRouteManifest({ @@ -236,10 +234,6 @@ describe('routing - createRouteManifest', () => { route: '/blog/contributing', type: 'page', }, - { - route: '/blog/[...slug]', - type: 'redirect', - }, { route: '/', type: 'page', @@ -248,6 +242,10 @@ describe('routing - createRouteManifest', () => { route: '/blog/about', type: 'redirect', }, + { + route: '/blog/[...slug]', + type: 'redirect', + }, ]); }); @@ -268,13 +266,14 @@ describe('routing - createRouteManifest', () => { '/blog/[...slug]': { status: 302, destination: '/', - priority: 'normal', }, '/blog/about': { status: 302, destination: '/another', - priority: 'normal', - } + }, + }, + experimental: { + stableRoutingPriority: true, }, }); const manifest = createRouteManifest({ @@ -303,12 +302,13 @@ describe('routing - createRouteManifest', () => { ]); }); - it('rejects colliding static routes', async () => { + // it should not throw an error, for now + it.skip('rejects colliding static routes', async () => { const fs = createFs( { '/src/pages/contributing.astro': `

test

`, }, - root, + root ); const settings = await createBasicSettings({ root: fileURLToPath(root), @@ -336,7 +336,9 @@ describe('routing - createRouteManifest', () => { expect(e).to.be.instanceOf(Error); expect(e.type).to.equal('AstroError'); expect(e.name).to.equal('StaticRouteCollision'); - expect(e.message).to.equal('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.'); + expect(e.message).to.equal( + '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.' + ); } }); @@ -346,7 +348,7 @@ describe('routing - createRouteManifest', () => { '/src/pages/[foo].astro': `

test

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

test

`, }, - root, + root ); const settings = await createBasicSettings({ root: fileURLToPath(root), @@ -367,7 +369,9 @@ describe('routing - createRouteManifest', () => { expect(e).to.be.instanceOf(Error); expect(e.type).to.equal('AstroError'); expect(e.name).to.equal('DynamicRouteCollision'); - expect(e.message).to.equal('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.'); + expect(e.message).to.equal( + '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.' + ); } }); }); From 7cfbdbd4d1d9e02f4ecc58dac84e820eabd53337 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 16:37:04 +0000 Subject: [PATCH 17/32] chore: use `continue` instead of `return` --- packages/astro/src/core/routing/manifest/create.ts | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index eb5ad3469224..29edf232463a 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -228,19 +228,19 @@ function createFileBasedRoutes( parentParams: string[] ) { let items: Item[] = []; - for (const basename of fs.readdirSync(dir)) { + 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)) { @@ -251,7 +251,7 @@ function createFileBasedRoutes( )} found. Prefix filename with an underscore (\`_\`) to ignore.` ); - return; + continue; } const segment = isDir ? basename : name; validateSegment(segment, file); From 61fc289f39593ebcb86a8a062842d9630a32f330 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Mon, 15 Jan 2024 16:45:32 +0000 Subject: [PATCH 18/32] chore: fix tests but one --- packages/astro/test/units/routing/manifest.test.js | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index a076ed81f902..ac1c09c7dc3e 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -49,6 +49,9 @@ describe('routing - createRouteManifest', () => { root: fileURLToPath(root), base: '/search', trailingSlash: 'never', + experimental: { + stableRoutingPriority: true, + }, }); settings.injectedRoutes = [ @@ -88,6 +91,7 @@ describe('routing - createRouteManifest', () => { ]); }); + // TODO: review this test, it seems a regression? it('injected routes are sorted in legacy mode above filesystem routes', async () => { const fs = createFs( { @@ -111,7 +115,6 @@ describe('routing - createRouteManifest', () => { { pattern: '/[...slug]', entrypoint: '@lib/legacy/dynamic.astro', - priority: 'legacy', }, ]; @@ -162,6 +165,9 @@ describe('routing - createRouteManifest', () => { }, }, ], + experimental: { + stableRoutingPriority: true, + }, }); settings.injectedRoutes = [ From eb855b9475db5ba4ace611c5d919f62ebf3202aa Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 15 Jan 2024 14:45:00 -0300 Subject: [PATCH 19/32] chore: Update test --- packages/astro/test/units/routing/manifest.test.js | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index ac1c09c7dc3e..390f7520b167 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -91,7 +91,6 @@ describe('routing - createRouteManifest', () => { ]); }); - // TODO: review this test, it seems a regression? it('injected routes are sorted in legacy mode above filesystem routes', async () => { const fs = createFs( { @@ -126,15 +125,15 @@ describe('routing - createRouteManifest', () => { expect(getManifestRoutes(manifest)).to.deep.equal([ { - route: '/[...slug]', + route: '/contributing', type: 'page', }, { - route: '/blog/[...slug]', + route: '/[...slug]', type: 'page', }, { - route: '/contributing', + route: '/blog/[...slug]', type: 'page', }, { From a8686b85f45aa6112c0d560399068472636faf91 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 15 Jan 2024 15:04:42 -0300 Subject: [PATCH 20/32] chore: Change remaining new error to warning --- packages/astro/src/core/errors/errors-data.ts | 27 ------------------- .../astro/src/core/routing/manifest/create.ts | 21 ++++++++------- .../astro/test/units/routing/manifest.test.js | 2 +- 3 files changed, 13 insertions(+), 37 deletions(-) diff --git a/packages/astro/src/core/errors/errors-data.ts b/packages/astro/src/core/errors/errors-data.ts index a62da250a178..fb463a77fe1c 100644 --- a/packages/astro/src/core/errors/errors-data.ts +++ b/packages/astro/src/core/errors/errors-data.ts @@ -1034,33 +1034,6 @@ export const UnhandledRejection = { hint: 'Make sure your promises all have an `await` or a `.catch()` handler.', } satisfies ErrorData; -/** - * @docs - * @description - * A static route was defined more than once. This can happen when an Astro integration injects a static route tha is also used in the project or when a static redirect is defined for a route that has a page or endpoint associated with it. - */ -export const StaticRouteCollision = { - name: 'StaticRouteCollision', - title: 'Static route collision', - message: (route: string, sourceA: string, sourceB: string) => - `The route "${route}" is defined in both "${sourceA}" and "${sourceB}". A static route cannot be defined more than once.`, -} satisfies ErrorData; - -/** - * @docs - * @see - * - [Dynamic HTML](https://docs.astro.build/en/core-concepts/astro-syntax/#dynamic-html') - * @description - * A dynamic route was defined more than once in SSR mode. This can happen when using two names for the dynamic parameters in the same positions, like `/foo/[bar]` and `/foo/[baz]`. If you can know at build time which paths you want to render with one of them, you can opt-in to pre-rendering in one of them to resolve the conflict. If which definition should be used is not known at build time, you need to select what to show within a single definition. - */ -export const DynamicRouteCollision = { - name: 'DynamicRouteCollision', - title: 'Dynamic route collision', - message: (route: string, sourceA: string, sourceB: string) => - `The route "${route}" is defined in both "${sourceA}" and "${sourceB}" using SSR mode. A dynamic SSR route cannot be defined more than once.`, - hint: 'If some paths should be handled by one of the definitions and all other paths by the other, you can opt-in to pre-rendering in one of them to resolve the conflict. If which definition should be used is not known at build time, you need to select what to show within a single definition.\nSee https://docs.astro.build/en/core-concepts/astro-syntax/#dynamic-html', -} satisfies ErrorData; - /** * @docs * @kind heading diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 29edf232463a..3d1f9a5c9bd0 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -18,11 +18,8 @@ import { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from '../../constants.js'; import { removeLeadingForwardSlash, slash } from '../../path.js'; import { resolvePages } from '../../util.js'; import { getRouteGenerator } from './generator.js'; -import { AstroError, AstroErrorData } from '../../errors/index.js'; -import { - MissingIndexForInternationalization, - DynamicRouteCollision, -} from '../../errors/errors-data.js'; +import {AstroError} from '../../errors/index.js'; +import {MissingIndexForInternationalization} from '../../errors/errors-data.js'; const require = createRequire(import.meta.url); interface Item { @@ -597,10 +594,16 @@ function detectRouteCollision(a: RouteData, b: RouteData, config: AstroConfig, l } // Both routes are guaranteed to collide such that one will never be matched. - throw new AstroError({ - ...DynamicRouteCollision, - message: DynamicRouteCollision.message(a.route, a.component, b.component), - }); + if (config.experimental.stableRoutingPriority) { + 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 */ diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 390f7520b167..59e727742ea4 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -347,7 +347,7 @@ describe('routing - createRouteManifest', () => { } }); - it('rejects colliding SSR dynamic routes', async () => { + it.skip('rejects colliding SSR dynamic routes', async () => { const fs = createFs( { '/src/pages/[foo].astro': `

test

`, From e319eb413fa93219a9f58620e74e49b4c0e6c3ca Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 15 Jan 2024 15:34:23 -0300 Subject: [PATCH 21/32] chore: Test collision warnings --- .../astro/src/core/routing/manifest/create.ts | 45 +++++----- .../astro/test/units/routing/manifest.test.js | 89 +++++++++++++------ 2 files changed, 84 insertions(+), 50 deletions(-) diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts index 3d1f9a5c9bd0..c2a618b56e44 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -555,16 +555,15 @@ function detectRouteCollision(a: RouteData, b: RouteData, config: AstroConfig, l ) { // If both routes are the same and completely static they are guaranteed to collide // such that one of them will never be matched. - if (config.experimental.stableRoutingPriority) { - 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.' - ); - } + 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) { @@ -594,16 +593,14 @@ function detectRouteCollision(a: RouteData, b: RouteData, config: AstroConfig, l } // Both routes are guaranteed to collide such that one will never be matched. - if (config.experimental.stableRoutingPriority) { - 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.' - ); - } + 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 */ @@ -639,9 +636,11 @@ export function createRouteManifest( ]; // Report route collisions - for (const [index, higherRoute] of routes.entries()) { - for (const lowerRoute of routes.slice(index + 1)) { - detectRouteCollision(higherRoute, lowerRoute, config, logger); + if (config.experimental.stableRoutingPriority) { + for (const [index, higherRoute] of routes.entries()) { + for (const lowerRoute of routes.slice(index + 1)) { + detectRouteCollision(higherRoute, lowerRoute, config, logger); + } } } diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 59e727742ea4..1e82fa463b44 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -14,6 +14,18 @@ function getManifestRoutes(manifest) { })); } +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( @@ -307,8 +319,7 @@ describe('routing - createRouteManifest', () => { ]); }); - // it should not throw an error, for now - it.skip('rejects colliding static routes', async () => { + it('report colliding static routes', async () => { const fs = createFs( { '/src/pages/contributing.astro': `

test

`, @@ -321,6 +332,9 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', integrations: [], + experimental: { + stableRoutingPriority: true, + } }); settings.injectedRoutes = [ @@ -330,24 +344,33 @@ describe('routing - createRouteManifest', () => { }, ]; - try { - createRouteManifest({ + const manifestOptions = { cwd: fileURLToPath(root), settings, fsMod: fs, - }); - expect.fail(0, 1, 'Expected createRouteManifest to throw'); - } catch (e) { - expect(e).to.be.instanceOf(Error); - expect(e.type).to.equal('AstroError'); - expect(e.name).to.equal('StaticRouteCollision'); - expect(e.message).to.equal( - '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.' - ); - } + }; + + 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.skip('rejects colliding SSR dynamic routes', async () => { + it('report colliding SSR dynamic routes', async () => { const fs = createFs( { '/src/pages/[foo].astro': `

test

`, @@ -361,22 +384,34 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', integrations: [], + experimental: { + stableRoutingPriority: true, + } }); - try { - createRouteManifest({ + const manifestOptions = { cwd: fileURLToPath(root), settings, fsMod: fs, - }); - expect.fail(0, 1, 'Expected createRouteManifest to throw'); - } catch (e) { - expect(e).to.be.instanceOf(Error); - expect(e.type).to.equal('AstroError'); - expect(e.name).to.equal('DynamicRouteCollision'); - expect(e.message).to.equal( - '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.' - ); - } + }; + + 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, + }, + ]); }); }); From bd930475447fe9d450e28c14d1d6e0ff4ae4df70 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 15 Jan 2024 21:08:46 -0300 Subject: [PATCH 22/32] docs: Update docs of new config --- packages/astro/src/@types/astro.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 3175f7de3878..6c9f09b49fb6 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1574,9 +1574,23 @@ export interface AstroUserConfig { * @default `false` * @version 4.2.0 * @description + * + * Prioritize redirects and injected routes along with discovered file-based project routes, behaving the same as if all routes were defined as files in the project. * - * - * Merge with discovered file-based project routes, behaving the same as if the route was defined as a file in the project. + * For the following scenario in SSR mode: + * - 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 with the following routes: + * + * | Page | Current Behavior | Stable 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` | */ stableRoutingPriority?: boolean; }; From de3a8cbed86442220dd765c78d48ac21aae2a24e Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Mon, 15 Jan 2024 21:27:11 -0300 Subject: [PATCH 23/32] docs: Improve changesets --- .changeset/short-keys-bow.md | 15 +++++++++++ .changeset/smooth-cobras-help.md | 46 +++++++++++++++++++++----------- 2 files changed, 45 insertions(+), 16 deletions(-) create mode 100644 .changeset/short-keys-bow.md diff --git a/.changeset/short-keys-bow.md b/.changeset/short-keys-bow.md new file mode 100644 index 000000000000..c7bee5fff3a8 --- /dev/null +++ b/.changeset/short-keys-bow.md @@ -0,0 +1,15 @@ +--- +"astro": patch +--- + +Fix inconsistency between route priorities of file routes, redirects and injected routes + + +Now all groups are prioritized following the same rules: + +- Routes with more path segments will take precedence over less specific routes. E.g. `/blog/post/[pid].astro` takes precedence over `/blog/[...slug].astro`. +- Static routes without path parameters will take precedence over dynamic routes. E.g. `/posts/create.astro` takes precedence over all the other routes in the example. +- Dynamic routes using named parameters take precedence over rest parameters. E.g. `/posts/[page].astro` takes precedence over `/posts/[...slug].astro`. +- Pre-rendered dynamic routes take precedence over server dynamic routes. +- Endpoints take precedence over pages. E.g. `/posts/[pid].ts` takes precedence over `/posts/[pid].astro`. +- If none of the rules above decide the order, routes are sorted alphabetically based on the default locale of your Node installation. diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index a47e2baece59..49f727ee4c29 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -2,19 +2,33 @@ 'astro': minor --- -Reworks route priority processing to allow for more flexible and intuitive redirects and route injection - -- Priority order for project routes, injected routes and redirects are now all the same. -- Injected routes and redirects can now specify if they should be prioritized above, - with or below project routes. This is done by adding a `priority` property to the route - object in `injectRoute` and in the `redirects` property in `astro.config.mjs`. -- Now more specific routes always take priority over less specific routes. - Example: `/blog/[...slug]` will take priority over `/[...slug]` -- Static redirects now have a lower priority than all project routed, even if the routes are dynamic, - matching the already documented behavior. - Example: `/blog (redirect)` will no longer override a `/[slug]` route by default, this can be re-enabled - using the new `priority` field. -- Collision detection between routes can now detect coliisions between similar dynamic routes - of any kind (project routes, injected routes and redirects). - Example: `/blog/[page]` will now be detected as a collision with `/blog/[slug]` -- Colision detection is now reported as a warning for backward compatibility with all the previous false negatives. +Adds an experimental flag `stableRoutePriority` to make all routes be prioritized following the same stable rules. + +```js +// astro.config.mjs +{ + experimental: { + stableRoutePriority: true, + }, +} +``` + +Enabling this feature makes redirects and injected routes be prioritized along with discovered file-based project +routes, behaving the same as if all routes were defined as files in the project. + +For the following scenario in SSR mode: + +- 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 with the following routes: + +| Page | Current Behavior | Stable 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` | + From fdc7c902e91abed52f052a86f12cd320609f2af6 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 16 Jan 2024 14:58:44 +0000 Subject: [PATCH 24/32] chore: rename experimental flag --- packages/astro/src/@types/astro.ts | 10 +++++----- packages/astro/src/core/config/schema.ts | 6 +++--- packages/astro/src/core/routing/manifest/create.ts | 13 +++++-------- 3 files changed, 13 insertions(+), 16 deletions(-) diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 6c9f09b49fb6..85dbe0924ac6 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1569,12 +1569,12 @@ export interface AstroUserConfig { /** * @docs - * @name experimental.stableRoutingPriority + * @name experimental.globalRoutePriority * @type {boolean} * @default `false` * @version 4.2.0 * @description - * + * * Prioritize redirects and injected routes along with discovered file-based project routes, behaving the same as if all routes were defined as files in the project. * * For the following scenario in SSR mode: @@ -1583,16 +1583,16 @@ export interface AstroUserConfig { * - Injected route: `/blog/[...slug]` * - Redirect: `/blog/tags/[tag]` -> `/[tag]` * - Redirect: `/posts` -> `/blog` - * + * * URLs are handled with the following routes: - * + * * | Page | Current Behavior | Stable 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` | */ - stableRoutingPriority?: boolean; + globalRoutePriority?: boolean; }; } diff --git a/packages/astro/src/core/config/schema.ts b/packages/astro/src/core/config/schema.ts index 930050b78705..9bd9310d4896 100644 --- a/packages/astro/src/core/config/schema.ts +++ b/packages/astro/src/core/config/schema.ts @@ -58,7 +58,7 @@ const ASTRO_CONFIG_DEFAULTS = { experimental: { optimizeHoistedScript: false, contentCollectionCache: false, - stableRoutingPriority: false, + globalRoutePriority: false, }, } satisfies AstroUserConfig & { server: { open: boolean } }; @@ -394,10 +394,10 @@ export const AstroConfigSchema = z.object({ .boolean() .optional() .default(ASTRO_CONFIG_DEFAULTS.experimental.contentCollectionCache), - stableRoutingPriority: z + globalRoutePriority: z .boolean() .optional() - .default(ASTRO_CONFIG_DEFAULTS.experimental.stableRoutingPriority), + .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 c2a618b56e44..6bca9fcf8a1e 100644 --- a/packages/astro/src/core/routing/manifest/create.ts +++ b/packages/astro/src/core/routing/manifest/create.ts @@ -18,8 +18,8 @@ import { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from '../../constants.js'; import { removeLeadingForwardSlash, slash } from '../../path.js'; import { resolvePages } from '../../util.js'; import { getRouteGenerator } from './generator.js'; -import {AstroError} from '../../errors/index.js'; -import {MissingIndexForInternationalization} from '../../errors/errors-data.js'; +import { AstroError } from '../../errors/index.js'; +import { MissingIndexForInternationalization } from '../../errors/errors-data.js'; const require = createRequire(import.meta.url); interface Item { @@ -597,10 +597,7 @@ function detectRouteCollision(a: RouteData, b: RouteData, config: AstroConfig, l '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.' - ); + logger.warn('router', 'A collision will result in an hard error in following versions of Astro.'); } /** Create manifest of all static routes */ @@ -636,7 +633,7 @@ export function createRouteManifest( ]; // Report route collisions - if (config.experimental.stableRoutingPriority) { + if (config.experimental.globalRoutePriority) { for (const [index, higherRoute] of routes.entries()) { for (const lowerRoute of routes.slice(index + 1)) { detectRouteCollision(higherRoute, lowerRoute, config, logger); @@ -837,7 +834,7 @@ export function createRouteManifest( } function computeRoutePriority(config: AstroConfig): RoutePriorityOverride { - if (config.experimental.stableRoutingPriority) { + if (config.experimental.globalRoutePriority) { return 'normal'; } return 'legacy'; From 5671f416a0850c6251fd119871fc756a1df00b02 Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Tue, 16 Jan 2024 15:35:10 +0000 Subject: [PATCH 25/32] chore: update changeset and docs --- .changeset/smooth-cobras-help.md | 8 ++++---- packages/astro/src/@types/astro.ts | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index 49f727ee4c29..146220600ceb 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -6,11 +6,11 @@ Adds an experimental flag `stableRoutePriority` to make all routes be prioritize ```js // astro.config.mjs -{ +export default defineConfig({ experimental: { - stableRoutePriority: true, + globalRoutePriority: true, }, -} +}) ``` Enabling this feature makes redirects and injected routes be prioritized along with discovered file-based project @@ -26,7 +26,7 @@ For the following scenario in SSR mode: URLs are handled with the following routes: -| Page | Current Behavior | Stable Routing Priority Behavior | +| 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] | diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index 85dbe0924ac6..fc2fa096d56d 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1586,7 +1586,7 @@ export interface AstroUserConfig { * * URLs are handled with the following routes: * - * | Page | Current Behavior | Stable Routing Priority Behavior | + * | 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] | From 24c9c746ee9bcb60991888be3067672eaba6ef63 Mon Sep 17 00:00:00 2001 From: Sarah Rainsberger Date: Tue, 16 Jan 2024 11:44:07 -0400 Subject: [PATCH 26/32] Sarah editing pass --- .changeset/short-keys-bow.md | 17 +++++++++-------- .changeset/smooth-cobras-help.md | 11 +++++------ packages/astro/src/@types/astro.ts | 8 ++++---- 3 files changed, 18 insertions(+), 18 deletions(-) diff --git a/.changeset/short-keys-bow.md b/.changeset/short-keys-bow.md index c7bee5fff3a8..bfbfd6e4391c 100644 --- a/.changeset/short-keys-bow.md +++ b/.changeset/short-keys-bow.md @@ -2,14 +2,15 @@ "astro": patch --- -Fix inconsistency between route priorities of file routes, redirects and injected routes +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). -Now all groups are prioritized following the same rules: -- Routes with more path segments will take precedence over less specific routes. E.g. `/blog/post/[pid].astro` takes precedence over `/blog/[...slug].astro`. -- Static routes without path parameters will take precedence over dynamic routes. E.g. `/posts/create.astro` takes precedence over all the other routes in the example. -- Dynamic routes using named parameters take precedence over rest parameters. E.g. `/posts/[page].astro` takes precedence over `/posts/[...slug].astro`. -- Pre-rendered dynamic routes take precedence over server dynamic routes. -- Endpoints take precedence over pages. E.g. `/posts/[pid].ts` takes precedence over `/posts/[pid].astro`. -- If none of the rules above decide the order, routes are sorted alphabetically based on the default locale of your Node installation. diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index 146220600ceb..9a1c95c67bd1 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -2,7 +2,7 @@ 'astro': minor --- -Adds an experimental flag `stableRoutePriority` to make all routes be prioritized following the same stable rules. +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 @@ -13,10 +13,9 @@ export default defineConfig({ }) ``` -Enabling this feature makes redirects and injected routes be prioritized along with discovered file-based project -routes, behaving the same as if all routes were defined as files in the project. +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 locally-defined route definitions, making it impossible to override some routes locally. -For the following scenario in SSR mode: +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]` @@ -24,11 +23,11 @@ For the following scenario in SSR mode: - Redirect: `/blog/tags/[tag]` -> `/[tag]` - Redirect: `/posts` -> `/blog` -URLs are handled with the following routes: +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] | +| `/blog/post/0` | Injected route `/blog/[...slug]` | File-based route `/blog/post/[pid]` | | `/posts` | File-based route `/[page]` | Redirect to `/blog` | diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index fc2fa096d56d..a139adf0d926 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1575,21 +1575,21 @@ export interface AstroUserConfig { * @version 4.2.0 * @description * - * Prioritize redirects and injected routes along with discovered file-based project routes, behaving the same as if all routes were defined as files in the project. + * 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. * - * For the following scenario in SSR mode: + * 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 with the following routes: + * 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] | + * | `/blog/post/0` | Injected route `/blog/[...slug]` | File-based route `/blog/post/[pid]` | * | `/posts` | File-based route `/[page]` | Redirect to `/blog` | */ globalRoutePriority?: boolean; From 7fc2482a82b258800c6b11e50b807898b3a5bde2 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Tue, 16 Jan 2024 14:09:48 -0300 Subject: [PATCH 27/32] nit: Align Markdown table --- .changeset/smooth-cobras-help.md | 8 ++++---- packages/astro/src/@types/astro.ts | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index 9a1c95c67bd1..02ecfdc34269 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -25,9 +25,9 @@ The following table shows which route builds certain page URLs when file-based r 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]` | +| 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` | +| `/posts` | File-based route `/[page]` | Redirect to `/blog` | diff --git a/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index a139adf0d926..f4393e4040b8 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1586,11 +1586,11 @@ export interface AstroUserConfig { * * 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]` | + * | 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` | + * | `/posts` | File-based route `/[page]` | Redirect to `/blog` | */ globalRoutePriority?: boolean; }; From 30241b67fb1543d4ca85b047eb74ceea64bcc53a Mon Sep 17 00:00:00 2001 From: Sarah Rainsberger Date: Tue, 16 Jan 2024 13:33:28 -0400 Subject: [PATCH 28/32] defined definitions! Co-authored-by: Luiz Ferraz --- .changeset/smooth-cobras-help.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index 02ecfdc34269..4fdbcf73e566 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -13,7 +13,7 @@ export default defineConfig({ }) ``` -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 locally-defined route definitions, making it impossible to override some routes locally. +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: From fb78e034c98b3b22b3cbfb2ac5df6ee5aed41f39 Mon Sep 17 00:00:00 2001 From: Sarah Rainsberger Date: Tue, 16 Jan 2024 14:01:59 -0400 Subject: [PATCH 29/32] added logging info to docs for experimental flag --- .changeset/smooth-cobras-help.md | 1 + packages/astro/src/@types/astro.ts | 3 +++ 2 files changed, 4 insertions(+) diff --git a/.changeset/smooth-cobras-help.md b/.changeset/smooth-cobras-help.md index 4fdbcf73e566..80e368124264 100644 --- a/.changeset/smooth-cobras-help.md +++ b/.changeset/smooth-cobras-help.md @@ -31,3 +31,4 @@ URLs are handled by the following routes: | `/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 f4393e4040b8..cda3b770d186 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1591,6 +1591,9 @@ export interface AstroUserConfig { * | `/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; }; From dcf8ca491f744e004742b126108d24f9910f9227 Mon Sep 17 00:00:00 2001 From: Sarah Rainsberger Date: Tue, 16 Jan 2024 14:15:16 -0400 Subject: [PATCH 30/32] Yan final boss review Co-authored-by: Yan Thomas <61414485+Yan-Thomas@users.noreply.github.com> --- .changeset/short-keys-bow.md | 4 ++-- packages/astro/src/@types/astro.ts | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/.changeset/short-keys-bow.md b/.changeset/short-keys-bow.md index bfbfd6e4391c..bead5326ceb8 100644 --- a/.changeset/short-keys-bow.md +++ b/.changeset/short-keys-bow.md @@ -4,13 +4,13 @@ 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. +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). +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/packages/astro/src/@types/astro.ts b/packages/astro/src/@types/astro.ts index cda3b770d186..9db1eddd8ea0 100644 --- a/packages/astro/src/@types/astro.ts +++ b/packages/astro/src/@types/astro.ts @@ -1622,7 +1622,7 @@ export type InjectedScriptStage = 'before-hydration' | 'head-inline' | 'page' | * - "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 overriden by any project route on conflict. + * and redirects will be overridden by any project route on conflict. */ export type RoutePriorityOverride = 'normal' | 'legacy'; From 9882de12b1b03fb625173ec0647abc60594b7c43 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Tue, 16 Jan 2024 17:09:49 -0300 Subject: [PATCH 31/32] chore: Update flag name in tests --- packages/astro/test/units/routing/manifest.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index 1e82fa463b44..cc96c6bdf6c5 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -62,7 +62,7 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', experimental: { - stableRoutingPriority: true, + globalRoutingPriority: true, }, }); @@ -177,7 +177,7 @@ describe('routing - createRouteManifest', () => { }, ], experimental: { - stableRoutingPriority: true, + globalRoutingPriority: true, }, }); @@ -290,7 +290,7 @@ describe('routing - createRouteManifest', () => { }, }, experimental: { - stableRoutingPriority: true, + globalRoutingPriority: true, }, }); const manifest = createRouteManifest({ @@ -333,7 +333,7 @@ describe('routing - createRouteManifest', () => { trailingSlash: 'never', integrations: [], experimental: { - stableRoutingPriority: true, + globalRoutingPriority: true, } }); @@ -385,7 +385,7 @@ describe('routing - createRouteManifest', () => { trailingSlash: 'never', integrations: [], experimental: { - stableRoutingPriority: true, + globalRoutingPriority: true, } }); From a34647eec52d2470372f80ff799c8b7758eb2383 Mon Sep 17 00:00:00 2001 From: Luiz Ferraz Date: Tue, 16 Jan 2024 18:08:01 -0300 Subject: [PATCH 32/32] chore: Update flag name in tests --- packages/astro/test/units/routing/manifest.test.js | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js index cc96c6bdf6c5..5da15fad09f0 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -62,7 +62,7 @@ describe('routing - createRouteManifest', () => { base: '/search', trailingSlash: 'never', experimental: { - globalRoutingPriority: true, + globalRoutePriority: true, }, }); @@ -177,7 +177,7 @@ describe('routing - createRouteManifest', () => { }, ], experimental: { - globalRoutingPriority: true, + globalRoutePriority: true, }, }); @@ -290,7 +290,7 @@ describe('routing - createRouteManifest', () => { }, }, experimental: { - globalRoutingPriority: true, + globalRoutePriority: true, }, }); const manifest = createRouteManifest({ @@ -333,7 +333,7 @@ describe('routing - createRouteManifest', () => { trailingSlash: 'never', integrations: [], experimental: { - globalRoutingPriority: true, + globalRoutePriority: true, } }); @@ -385,7 +385,7 @@ describe('routing - createRouteManifest', () => { trailingSlash: 'never', integrations: [], experimental: { - globalRoutingPriority: true, + globalRoutePriority: true, } });