From 564ac6c2f2d77ee34f8519f1e5a4db2c6e194f65 Mon Sep 17 00:00:00 2001 From: Florian Lefebvre Date: Mon, 16 Dec 2024 15:52:54 +0100 Subject: [PATCH] feat: route manifest refactor (#12597) Co-authored-by: Emanuele Stoppa --- .changeset/empty-crews-scream.md | 5 +++++ packages/astro/src/assets/endpoint/config.ts | 11 ---------- packages/astro/src/core/app/index.ts | 10 +++++++--- packages/astro/src/core/build/index.ts | 9 +-------- packages/astro/src/core/build/page-data.ts | 6 ++++++ .../src/core/build/plugins/plugin-manifest.ts | 17 ++++++++++++++++ packages/astro/src/core/dev/container.ts | 7 +------ packages/astro/src/core/routing/default.ts | 17 ++++------------ .../astro/src/core/routing/dev-default.ts | 14 ------------- .../astro/src/core/routing/manifest/create.ts | 20 +++++++++++++++++-- .../astro/src/core/server-islands/endpoint.ts | 6 +----- .../src/vite-plugin-astro-server/plugin.ts | 9 ++------- .../astro/test/units/routing/manifest.test.js | 18 +++++++++++++++++ 13 files changed, 80 insertions(+), 69 deletions(-) create mode 100644 .changeset/empty-crews-scream.md delete mode 100644 packages/astro/src/core/routing/dev-default.ts diff --git a/.changeset/empty-crews-scream.md b/.changeset/empty-crews-scream.md new file mode 100644 index 000000000000..c52237ab8cc3 --- /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 cce5cde5fc37..b59bdc626b9a 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 d23383133e05..a3d08bb642e9 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 68cf650acd0c..bcc7fb96d56d 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 c36fe0d57b29..59806be58422 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 f0589868f72c..a3bd6239a4ed 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 063681f2717c..3484227afb9b 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 8bcd473d0081..d255b13275f2 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 ac2ea1d3b03b..000000000000 --- 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 6dbcf18aee68..15d137286955 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 ca24b54af4b9..5afdde651b7d 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 b706f967d3e8..7e4a7169d5b8 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 d4f9f2514901..a981b8719c0d 100644 --- a/packages/astro/test/units/routing/manifest.test.js +++ b/packages/astro/test/units/routing/manifest.test.js @@ -260,6 +260,14 @@ describe('routing - createRouteManifest', () => { }); assert.deepEqual(getManifestRoutes(manifest), [ + { + route: '/_server-islands/[name]', + type: 'page', + }, + { + route: '/_image', + type: 'endpoint', + }, { route: '/blog/[...slug]', type: 'page', @@ -306,6 +314,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' },