feat: route manifest refactor (#12597)

Co-authored-by: Emanuele Stoppa <my.burning@gmail.com>

13 files changed, 80 insertions, 69 deletions

diff --git a/.changeset/empty-crews-scream.md b/.changeset/empty-crews-scream.md
new file mode 100644
index 000000000..c52237ab8
--- /dev/null
+++ b/.changeset/empty-crews-scream.md
@@ -0,0 +1,5 @@
+---
+'astro': patch
+---
+
+Fixes an issue where image and server islands routes would not be passed to the `astro:routes:resolved` hook during builds
diff --git a/packages/astro/src/assets/endpoint/config.ts b/packages/astro/src/assets/endpoint/config.ts
index cce5cde5f..b59bdc626 100644
--- a/packages/astro/src/assets/endpoint/config.ts
+++ b/packages/astro/src/assets/endpoint/config.ts
@@ -16,17 +16,6 @@ export function injectImageEndpoint(
 	manifest.routes.unshift(getImageEndpointData(settings, mode, cwd));
 }
 
-export function ensureImageEndpointRoute(
-	settings: AstroSettings,
-	manifest: ManifestData,
-	mode: 'dev' | 'build',
-	cwd?: string,
-) {
-	if (!manifest.routes.some((route) => route.route === settings.config.image.endpoint.route)) {
-		manifest.routes.unshift(getImageEndpointData(settings, mode, cwd));
-	}
-}
-
 function getImageEndpointData(
 	settings: AstroSettings,
 	mode: 'dev' | 'build',
diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts
index d23383133..a3d08bb64 100644
--- a/packages/astro/src/core/app/index.ts
+++ b/packages/astro/src/core/app/index.ts
@@ -20,7 +20,8 @@ import {
 } from '../path.js';
 import { RenderContext } from '../render-context.js';
 import { createAssetLink } from '../render/ssr-element.js';
-import { createDefaultRoutes, injectDefaultRoutes } from '../routing/default.js';
+import { ensure404Route } from '../routing/astro-designed-error-pages.js';
+import { createDefaultRoutes } from '../routing/default.js';
 import { matchRoute } from '../routing/match.js';
 import { AppPipeline } from './pipeline.js';
 
@@ -88,9 +89,12 @@ export class App {
 
 	constructor(manifest: SSRManifest, streaming = true) {
 		this.#manifest = manifest;
-		this.#manifestData = injectDefaultRoutes(manifest, {
+		this.#manifestData = {
 			routes: manifest.routes.map((route) => route.routeData),
-		});
+		};
+		// This is necessary to allow running middlewares for 404 in SSR. There's special handling
+		// to return the host 404 if the user doesn't provide a custom 404
+		ensure404Route(this.#manifestData);
 		this.#baseWithoutTrailingSlash = removeTrailingForwardSlash(this.#manifest.base);
 		this.#pipeline = this.#createPipeline(this.#manifestData, streaming);
 		this.#adapterLogger = new AstroIntegrationLogger(
diff --git a/packages/astro/src/core/build/index.ts b/packages/astro/src/core/build/index.ts
index 68cf650ac..bcc7fb96d 100644
--- a/packages/astro/src/core/build/index.ts
+++ b/packages/astro/src/core/build/index.ts
@@ -3,7 +3,6 @@ import { performance } from 'node:perf_hooks';
 import { fileURLToPath } from 'node:url';
 import { blue, bold, green } from 'kleur/colors';
 import type * as vite from 'vite';
-import { injectImageEndpoint } from '../../assets/endpoint/config.js';
 import { telemetry } from '../../events/index.js';
 import { eventCliSession } from '../../events/session.js';
 import {
@@ -24,7 +23,6 @@ import type { Logger } from '../logger/core.js';
 import { levels, timerMessage } from '../logger/core.js';
 import { apply as applyPolyfill } from '../polyfill.js';
 import { createRouteManifest } from '../routing/index.js';
-import { getServerIslandRouteData } from '../server-islands/endpoint.js';
 import { clearContentLayerCache } from '../sync/index.js';
 import { ensureProcessNodeEnv } from '../util.js';
 import { collectPagesData } from './page-data.js';
@@ -123,10 +121,6 @@ class AstroBuilder {
 
 		this.manifest = await createRouteManifest({ settings: this.settings }, this.logger);
 
-		if (this.settings.buildOutput === 'server') {
-			injectImageEndpoint(this.settings, this.manifest, 'build');
-		}
-
 		await runHookConfigDone({ settings: this.settings, logger: logger, command: 'build' });
 
 		// If we're building for the server, we need to ensure that an adapter is installed.
@@ -239,8 +233,7 @@ class AstroBuilder {
 			pages: pageNames,
 			routes: Object.values(allPages)
 				.flat()
-				.map((pageData) => pageData.route)
-				.concat(hasServerIslands ? getServerIslandRouteData(this.settings.config) : []),
+				.map((pageData) => pageData.route),
 			logging: this.logger,
 		});
 
diff --git a/packages/astro/src/core/build/page-data.ts b/packages/astro/src/core/build/page-data.ts
index c36fe0d57..59806be58 100644
--- a/packages/astro/src/core/build/page-data.ts
+++ b/packages/astro/src/core/build/page-data.ts
@@ -5,6 +5,7 @@ import type { AllPagesData } from './types.js';
 import * as colors from 'kleur/colors';
 import { debug } from '../logger/core.js';
 import { makePageDataKey } from './plugins/util.js';
+import { DEFAULT_COMPONENTS } from '../routing/default.js';
 
 export interface CollectPagesDataOptions {
 	settings: AstroSettings;
@@ -29,6 +30,11 @@ export function collectPagesData(opts: CollectPagesDataOptions): CollectPagesDat
 	// and is then cached across all future SSR builds. In the past, we've had trouble
 	// with parallelized builds without guaranteeing that this is called first.
 	for (const route of manifest.routes) {
+		// There's special handling in SSR
+		if (DEFAULT_COMPONENTS.some((component) => route.component === component)) {
+			continue;
+		}
+
 		// Generate a unique key to identify each page in the build process.
 		const key = makePageDataKey(route.route, route.component);
 		// static route:
diff --git a/packages/astro/src/core/build/plugins/plugin-manifest.ts b/packages/astro/src/core/build/plugins/plugin-manifest.ts
index f0589868f..a3bd6239a 100644
--- a/packages/astro/src/core/build/plugins/plugin-manifest.ts
+++ b/packages/astro/src/core/build/plugins/plugin-manifest.ts
@@ -21,6 +21,7 @@ import { type BuildInternals, cssOrder, mergeInlineCss } from '../internal.js';
 import type { AstroBuildPlugin } from '../plugin.js';
 import type { StaticBuildOptions } from '../types.js';
 import { makePageDataKey } from './util.js';
+import { DEFAULT_COMPONENTS } from '../../routing/default.js';
 
 const manifestReplace = '@@ASTRO_MANIFEST_REPLACE@@';
 const replaceExp = new RegExp(`['"]${manifestReplace}['"]`, 'g');
@@ -172,6 +173,22 @@ function buildManifest(
 		}
 	};
 
+	// Default components follow a special flow during build. We prevent their processing earlier
+	// in the build. As a result, they are not present on `internals.pagesByKeys` and not serialized
+	// in the manifest file. But we need them in the manifest, so we handle them here
+	for (const route of opts.manifest.routes) {
+		if (!DEFAULT_COMPONENTS.find((component) => route.component === component)) {
+			continue;
+		}
+		routes.push({
+			file: '',
+			links: [],
+			scripts: [],
+			styles: [],
+			routeData: serializeRouteData(route, settings.config.trailingSlash),
+		});
+	}
+
 	for (const route of opts.manifest.routes) {
 		if (!route.prerender) continue;
 		if (!route.pathname) continue;
diff --git a/packages/astro/src/core/dev/container.ts b/packages/astro/src/core/dev/container.ts
index 063681f27..3484227af 100644
--- a/packages/astro/src/core/dev/container.ts
+++ b/packages/astro/src/core/dev/container.ts
@@ -7,7 +7,6 @@ import * as vite from 'vite';
 import {
 	runHookConfigDone,
 	runHookConfigSetup,
-	runHookRoutesResolved,
 	runHookServerDone,
 	runHookServerStart,
 } from '../../integrations/hooks.js';
@@ -16,7 +15,6 @@ import { createDevelopmentManifest } from '../../vite-plugin-astro-server/plugin
 import { createVite } from '../create-vite.js';
 import type { Logger } from '../logger/core.js';
 import { apply as applyPolyfill } from '../polyfill.js';
-import { injectDefaultDevRoutes } from '../routing/dev-default.js';
 import { createRouteManifest } from '../routing/index.js';
 import { syncInternal } from '../sync/index.js';
 import { warnMissingAdapter } from './adapter-validation.js';
@@ -84,12 +82,9 @@ export async function createContainer({
 		.filter(Boolean) as string[];
 
 	// Create the route manifest already outside of Vite so that `runHookConfigDone` can use it to inform integrations of the build output
-	let manifest = await createRouteManifest({ settings, fsMod: fs }, logger, { dev: true });
+	const manifest = await createRouteManifest({ settings, fsMod: fs }, logger, { dev: true });
 	const devSSRManifest = createDevelopmentManifest(settings);
 
-	manifest = injectDefaultDevRoutes(settings, devSSRManifest, manifest);
-	await runHookRoutesResolved({ settings, logger, routes: manifest.routes });
-
 	await runHookConfigDone({ settings, logger, command: 'dev' });
 
 	warnMissingAdapter(logger, settings);
diff --git a/packages/astro/src/core/routing/default.ts b/packages/astro/src/core/routing/default.ts
index 8bcd473d0..d255b1327 100644
--- a/packages/astro/src/core/routing/default.ts
+++ b/packages/astro/src/core/routing/default.ts
@@ -1,23 +1,12 @@
-import type { ComponentInstance, ManifestData } from '../../types/astro.js';
+import type { ComponentInstance } from '../../types/astro.js';
 import type { SSRManifest } from '../app/types.js';
 import { DEFAULT_404_COMPONENT } from '../constants.js';
 import {
 	SERVER_ISLAND_COMPONENT,
 	SERVER_ISLAND_ROUTE,
 	createEndpoint as createServerIslandEndpoint,
-	ensureServerIslandRoute,
 } from '../server-islands/endpoint.js';
-import {
-	DEFAULT_404_ROUTE,
-	default404Instance,
-	ensure404Route,
-} from './astro-designed-error-pages.js';
-
-export function injectDefaultRoutes(ssrManifest: SSRManifest, routeManifest: ManifestData) {
-	ensure404Route(routeManifest);
-	ensureServerIslandRoute(ssrManifest, routeManifest);
-	return routeManifest;
-}
+import { DEFAULT_404_ROUTE, default404Instance } from './astro-designed-error-pages.js';
 
 type DefaultRouteParams = {
 	instance: ComponentInstance;
@@ -26,6 +15,8 @@ type DefaultRouteParams = {
 	component: string;
 };
 
+export const DEFAULT_COMPONENTS = [DEFAULT_404_COMPONENT, SERVER_ISLAND_COMPONENT];
+
 export function createDefaultRoutes(manifest: SSRManifest): DefaultRouteParams[] {
 	const root = new URL(manifest.hrefRoot);
 	return [
diff --git a/packages/astro/src/core/routing/dev-default.ts b/packages/astro/src/core/routing/dev-default.ts
deleted file mode 100644
index ac2ea1d3b..000000000
--- a/packages/astro/src/core/routing/dev-default.ts
+++ /dev/null
@@ -1,14 +0,0 @@
-import { ensureImageEndpointRoute } from '../../assets/endpoint/config.js';
-import type { AstroSettings, ManifestData } from '../../types/astro.js';
-import type { SSRManifest } from '../app/types.js';
-import { injectDefaultRoutes } from './default.js';
-
-export function injectDefaultDevRoutes(
-	settings: AstroSettings,
-	ssrManifest: SSRManifest,
-	routeManifest: ManifestData,
-) {
-	ensureImageEndpointRoute(settings, routeManifest, 'dev');
-	injectDefaultRoutes(ssrManifest, routeManifest);
-	return routeManifest;
-}
diff --git a/packages/astro/src/core/routing/manifest/create.ts b/packages/astro/src/core/routing/manifest/create.ts
index 6dbcf18ae..15d137286 100644
--- a/packages/astro/src/core/routing/manifest/create.ts
+++ b/packages/astro/src/core/routing/manifest/create.ts
@@ -21,6 +21,9 @@ import { routeComparator } from '../priority.js';
 import { getRouteGenerator } from './generator.js';
 import { getPattern } from './pattern.js';
 import { getRoutePrerenderOption } from './prerender.js';
+import { ensure404Route } from '../astro-designed-error-pages.js';
+import { injectImageEndpoint } from '../../../assets/endpoint/config.js';
+import { injectServerIslandRoute } from '../../server-islands/endpoint.js';
 const require = createRequire(import.meta.url);
 
 interface Item {
@@ -732,9 +735,22 @@ export async function createRouteManifest(
 		}
 	}
 
-	if (!dev) {
-		await runHookRoutesResolved({ routes, settings, logger });
+	if (dev) {
+		// In SSR, a 404 route is injected in the App directly for some special handling,
+		// it must not appear in the manifest
+		ensure404Route({ routes });
 	}
+	if (dev || settings.buildOutput === 'server') {
+		injectImageEndpoint(settings, { routes }, dev ? 'dev' : 'build');
+		// Ideally we would only inject the server islands route if server islands are used in the project.
+		// Unforunately, there is a "circular dependency": to know if server islands are used, we need to run
+		// the build but the build relies on the routes manifest.
+		// This situation also means we cannot update the buildOutput based on wether or not server islands
+		// are used in the project. If server islands are detected after the build but the buildOutput is
+		// static, we fail the build.
+		injectServerIslandRoute(settings.config, { routes });
+	}
+	await runHookRoutesResolved({ routes, settings, logger });
 
 	return {
 		routes,
diff --git a/packages/astro/src/core/server-islands/endpoint.ts b/packages/astro/src/core/server-islands/endpoint.ts
index ca24b54af..5afdde651 100644
--- a/packages/astro/src/core/server-islands/endpoint.ts
+++ b/packages/astro/src/core/server-islands/endpoint.ts
@@ -37,11 +37,7 @@ export function getServerIslandRouteData(config: ConfigFields) {
 	return route;
 }
 
-export function ensureServerIslandRoute(config: ConfigFields, routeManifest: ManifestData) {
-	if (routeManifest.routes.some((route) => route.route === '/_server-islands/[name]')) {
-		return;
-	}
-
+export function injectServerIslandRoute(config: ConfigFields, routeManifest: ManifestData) {
 	routeManifest.routes.unshift(getServerIslandRouteData(config));
 }
 
diff --git a/packages/astro/src/vite-plugin-astro-server/plugin.ts b/packages/astro/src/vite-plugin-astro-server/plugin.ts
index b706f967d..7e4a7169d 100644
--- a/packages/astro/src/vite-plugin-astro-server/plugin.ts
+++ b/packages/astro/src/vite-plugin-astro-server/plugin.ts
@@ -13,7 +13,6 @@ import { patchOverlay } from '../core/errors/overlay.js';
 import type { Logger } from '../core/logger/core.js';
 import { NOOP_MIDDLEWARE_FN } from '../core/middleware/noop-middleware.js';
 import { createViteLoader } from '../core/module-loader/index.js';
-import { injectDefaultDevRoutes } from '../core/routing/dev-default.js';
 import { createRouteManifest } from '../core/routing/index.js';
 import { getRoutePrerenderOption } from '../core/routing/manifest/prerender.js';
 import { toFallbackType, toRoutingStrategy } from '../i18n/utils.js';
@@ -74,15 +73,11 @@ export default function createVitePluginAstroServer({
 					try {
 						const content = await fsMod.promises.readFile(routePath, 'utf-8');
 						await getRoutePrerenderOption(content, route, settings, logger);
+						await runHookRoutesResolved({ routes: routeManifest.routes, settings, logger });
 					} catch (_) {}
 				} else {
-					routeManifest = injectDefaultDevRoutes(
-						settings,
-						devSSRManifest,
-						await createRouteManifest({ settings, fsMod }, logger, { dev: true }),
-					);
+					routeManifest = await createRouteManifest({ settings, fsMod }, logger, { dev: true });
 				}
-				await runHookRoutesResolved({ routes: routeManifest.routes, settings, logger });
 
 				warnMissingAdapter(logger, settings);
 				pipeline.manifest.checkOrigin =
diff --git a/packages/astro/test/units/routing/manifest.test.js b/packages/astro/test/units/routing/manifest.test.js
index d4f9f2514..a981b8719 100644
--- a/packages/astro/test/units/routing/manifest.test.js
+++ b/packages/astro/test/units/routing/manifest.test.js
@@ -261,6 +261,14 @@ describe('routing - createRouteManifest', () => {
 
 		assert.deepEqual(getManifestRoutes(manifest), [
 			{
+				route: '/_server-islands/[name]',
+				type: 'page',
+			},
+			{
+				route: '/_image',
+				type: 'endpoint',
+			},
+			{
 				route: '/blog/[...slug]',
 				type: 'page',
 			},
@@ -307,6 +315,14 @@ describe('routing - createRouteManifest', () => {
 
 		assert.deepEqual(getManifestRoutes(manifest), [
 			{
+				route: '/_server-islands/[name]',
+				type: 'page',
+			},
+			{
+				route: '/_image',
+				type: 'endpoint',
+			},
+			{
 				route: '/blog/about',
 				type: 'redirect',
 			},
@@ -441,6 +457,8 @@ describe('routing - createRouteManifest', () => {
 		});
 
 		assert.deepEqual(getManifestRoutes(manifest), [
+			{ type: 'page', route: '/_server-islands/[name]' },
+			{ type: 'endpoint', route: '/_image' },
 			{ type: 'endpoint', route: '/blog/a-[b].233' },
 			{ type: 'redirect', route: '/posts/a-[b].233' },
 			{ type: 'page', route: '/[c]-d' },