refactor: dynamically import middleware (#11550)

* refactor: dynamically import middleware * update tests * chore: fix code merge * address linting * remove changeset * feedback * feedback * fix test failing * add suggestion * change order of resolution to cache the middleware before inserting a route * address comment
withastro · Sep 23, 2024 · 25a104b · 25a104b
1 parent 4c47c44
commit 25a104b
Show file tree

Hide file tree

Showing 16 changed files with 86 additions and 42 deletions.
diff --git a/packages/astro/src/container/index.ts b/packages/astro/src/container/index.ts
@@ -2,6 +2,7 @@ import './polyfill.js';
 import { posix } from 'node:path';
 import type {
 	AstroConfig,
+	AstroMiddlewareInstance,
 	AstroUserConfig,
 	ComponentInstance,
 	ContainerImportRendererFn,
@@ -21,6 +22,7 @@ import { validateConfig } from '../core/config/validate.js';
 import { createKey } from '../core/encryption.js';
 import { Logger } from '../core/logger/core.js';
 import { nodeLogDestination } from '../core/logger/node.js';
+import { NOOP_MIDDLEWARE_FN } from '../core/middleware/noop-middleware.js';
 import { removeLeadingForwardSlash } from '../core/path.js';
 import { RenderContext } from '../core/render-context.js';
 import { getParts, validateSegment } from '../core/routing/manifest/create.js';
@@ -109,9 +111,11 @@ function createManifest(
 	renderers?: SSRLoadedRenderer[],
 	middleware?: MiddlewareHandler,
 ): SSRManifest {
-	const defaultMiddleware: MiddlewareHandler = (_, next) => {
-		return next();
-	};
+	function middlewareInstance(): AstroMiddlewareInstance {
+		return {
+			onRequest: middleware ?? NOOP_MIDDLEWARE_FN,
+		};
+	}
 
 	return {
 		hrefRoot: import.meta.url,
@@ -130,7 +134,7 @@ function createManifest(
 		inlinedScripts: manifest?.inlinedScripts ?? new Map(),
 		i18n: manifest?.i18n,
 		checkOrigin: false,
-		middleware: manifest?.middleware ?? middleware ?? defaultMiddleware,
+		middleware: manifest?.middleware ?? middlewareInstance,
 		experimentalEnvGetSecretEnabled: false,
 		key: createKey(),
 	};
@@ -476,11 +480,10 @@ export class experimental_AstroContainer {
 			params: options.params,
 			type: routeType,
 		});
-		const renderContext = RenderContext.create({
+		const renderContext = await RenderContext.create({
 			pipeline: this.#pipeline,
 			routeData,
 			status: 200,
-			middleware: this.#pipeline.middleware,
 			request,
 			pathname: url.pathname,
 			locals: options?.locals ?? {},

diff --git a/packages/astro/src/container/pipeline.ts b/packages/astro/src/container/pipeline.ts
@@ -88,7 +88,7 @@ export class ContainerPipeline extends Pipeline {
 				return Promise.resolve(componentInstance);
 			},
 			renderers: this.manifest.renderers,
-			onRequest: this.manifest.middleware,
+			onRequest: this.resolvedMiddleware,
 		});
 	}
 

diff --git a/packages/astro/src/core/app/common.ts b/packages/astro/src/core/app/common.ts
@@ -1,6 +1,7 @@
 import { decodeKey } from '../encryption.js';
 import { deserializeRouteData } from '../routing/manifest/serialization.js';
 import type { RouteInfo, SSRManifest, SerializedSSRManifest } from './types.js';
+import { NOOP_MIDDLEWARE_FN } from '../middleware/noop-middleware.js';
 
 export function deserializeManifest(serializedManifest: SerializedSSRManifest): SSRManifest {
 	const routes: RouteInfo[] = [];
@@ -23,8 +24,8 @@ export function deserializeManifest(serializedManifest: SerializedSSRManifest):
 
 	return {
 		// in case user middleware exists, this no-op middleware will be reassigned (see plugin-ssr.ts)
-		middleware(_, next) {
-			return next();
+		middleware() {
+			return { onRequest: NOOP_MIDDLEWARE_FN };
 		},
 		...serializedManifest,
 		assets,

diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts
@@ -11,7 +11,6 @@ import { getSetCookiesFromResponse } from '../cookies/index.js';
 import { AstroError, AstroErrorData } from '../errors/index.js';
 import { consoleLogDestination } from '../logger/console.js';
 import { AstroIntegrationLogger, Logger } from '../logger/core.js';
-import { sequence } from '../middleware/index.js';
 import {
 	appendForwardSlash,
 	joinPaths,
@@ -22,8 +21,8 @@ import { RenderContext } from '../render-context.js';
 import { createAssetLink } from '../render/ssr-element.js';
 import { createDefaultRoutes, injectDefaultRoutes } from '../routing/default.js';
 import { matchRoute } from '../routing/match.js';
-import { createOriginCheckMiddleware } from './middlewares.js';
 import { AppPipeline } from './pipeline.js';
+import { NOOP_MIDDLEWARE_FN } from '../middleware/noop-middleware.js';
 
 export { deserializeManifest } from './common.js';
 
@@ -111,13 +110,6 @@ export class App {
 	 * @private
 	 */
 	#createPipeline(manifestData: ManifestData, streaming = false) {
-		if (this.#manifest.checkOrigin) {
-			this.#manifest.middleware = sequence(
-				createOriginCheckMiddleware(),
-				this.#manifest.middleware,
-			);
-		}
-
 		return AppPipeline.create(manifestData, {
 			logger: this.#logger,
 			manifest: this.#manifest,
@@ -323,7 +315,7 @@ export class App {
 			// Load route module. We also catch its error here if it fails on initialization
 			const mod = await this.#pipeline.getModuleForRoute(routeData);
 
-			const renderContext = RenderContext.create({
+			const renderContext = await RenderContext.create({
 				pipeline: this.#pipeline,
 				locals,
 				pathname,
@@ -428,10 +420,10 @@ export class App {
 			}
 			const mod = await this.#pipeline.getModuleForRoute(errorRouteData);
 			try {
-				const renderContext = RenderContext.create({
+				const renderContext = await RenderContext.create({
 					locals,
 					pipeline: this.#pipeline,
-					middleware: skipMiddleware ? (_, next) => next() : undefined,
+					middleware: skipMiddleware ? NOOP_MIDDLEWARE_FN : undefined,
 					pathname: this.#getPathnameFromRequest(request),
 					request,
 					routeData: errorRouteData,

diff --git a/packages/astro/src/core/app/types.ts b/packages/astro/src/core/app/types.ts
@@ -1,12 +1,12 @@
 import type {
 	ComponentInstance,
 	Locales,
-	MiddlewareHandler,
 	RouteData,
 	SSRComponentMetadata,
 	SSRLoadedRenderer,
 	SSRResult,
 	SerializedRouteData,
+	AstroMiddlewareInstance,
 } from '../../@types/astro.js';
 import type { RoutingStrategies } from '../../i18n/utils.js';
 import type { SinglePageBuiltModule } from '../build/types.js';
@@ -68,7 +68,7 @@ export type SSRManifest = {
 	serverIslandNameMap?: Map<string, string>;
 	key: Promise<CryptoKey>;
 	i18n: SSRManifestI18n | undefined;
-	middleware: MiddlewareHandler;
+	middleware: () => Promise<AstroMiddlewareInstance> | AstroMiddlewareInstance;
 	checkOrigin: boolean;
 	// TODO: remove experimental prefix
 	experimentalEnvGetSecretEnabled: boolean;

diff --git a/packages/astro/src/core/base-pipeline.ts b/packages/astro/src/core/base-pipeline.ts
@@ -15,6 +15,9 @@ import { AstroErrorData } from './errors/index.js';
 import type { Logger } from './logger/core.js';
 import { RouteCache } from './render/route-cache.js';
 import { createDefaultRoutes } from './routing/default.js';
+import {NOOP_MIDDLEWARE_FN} from "./middleware/noop-middleware.js";
+import {sequence} from "./middleware/index.js";
+import {createOriginCheckMiddleware} from "./app/middlewares.js";
 
 /**
  * The `Pipeline` represents the static parts of rendering that do not change between requests.
@@ -24,7 +27,8 @@ import { createDefaultRoutes } from './routing/default.js';
  */
 export abstract class Pipeline {
 	readonly internalMiddleware: MiddlewareHandler[];
-
+	resolvedMiddleware: MiddlewareHandler | undefined = undefined;
+
 	constructor(
 		readonly logger: Logger,
 		readonly manifest: SSRManifest,
@@ -97,6 +101,25 @@ export abstract class Pipeline {
 	 * @param routeData
 	 */
 	abstract getComponentByRoute(routeData: RouteData): Promise<ComponentInstance>;
+
+	/**
+	 * Resolves the middleware from the manifest, and returns the `onRequest` function. If `onRequest` isn't there,
+	 * it returns a no-op function
+	 */
+	async getMiddleware(): Promise<MiddlewareHandler> {
+		if (this.resolvedMiddleware) {
+			return this.resolvedMiddleware;
+		} else {
+			const middlewareInstance = await this.middleware();
+			const onRequest = middlewareInstance.onRequest ?? NOOP_MIDDLEWARE_FN;
+			if (this.manifest.checkOrigin) {
+				this.resolvedMiddleware = sequence(createOriginCheckMiddleware(), onRequest);
+			} else {
+				this.resolvedMiddleware = onRequest;
+			}
+			return this.resolvedMiddleware;
+		}
+	}
 }
 
 // eslint-disable-next-line @typescript-eslint/no-empty-object-type

diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts
@@ -443,7 +443,12 @@ async function generatePath(
 		logger,
 		staticLike: true,
 	});
-	const renderContext = RenderContext.create({ pipeline, pathname, request, routeData: route });
+	const renderContext = await RenderContext.create({
+		pipeline,
+		pathname,
+		request,
+		routeData: route,
+	});
 
 	let body: string | Uint8Array;
 	let response: Response;
@@ -552,7 +557,11 @@ function createBuildManifest(
 		componentMetadata: internals.componentMetadata,
 		i18n: i18nManifest,
 		buildFormat: settings.config.build.format,
-		middleware,
+		middleware() {
+			return {
+				onRequest: middleware,
+			};
+		},
 		checkOrigin: settings.config.security?.checkOrigin ?? false,
 		key,
 		experimentalEnvGetSecretEnabled: false,

diff --git a/packages/astro/src/core/build/pipeline.ts b/packages/astro/src/core/build/pipeline.ts
@@ -138,7 +138,11 @@ export class BuildPipeline extends Pipeline {
 		const renderers = await import(renderersEntryUrl.toString());
 
 		const middleware = await import(new URL('middleware.mjs', baseDirectory).toString())
-			.then((mod) => mod.onRequest)
+			.then((mod) => {
+				return function () {
+					return { onRequest: mod.onRequest };
+				};
+			})
 			// middleware.mjs is not emitted if there is no user middleware
 			// in which case the import fails with ERR_MODULE_NOT_FOUND, and we fall back to a no-op middleware
 			.catch(() => manifest.middleware);

diff --git a/packages/astro/src/core/build/plugins/plugin-ssr.ts b/packages/astro/src/core/build/plugins/plugin-ssr.ts
@@ -291,12 +291,11 @@ function generateSSRCode(settings: AstroSettings, adapter: AstroAdapter, middlew
 
 	const contents = [
 		settings.config.experimental.serverIslands ? '' : `const serverIslandMap = new Map()`,
-		edgeMiddleware ? `const middleware = (_, next) => next()` : '',
 		`const _manifest = Object.assign(defaultManifest, {`,
 		`    ${pageMap},`,
 		`    serverIslandMap,`,
 		`    renderers,`,
-		`    middleware`,
+		`    middleware: ${edgeMiddleware ? 'undefined' : `() => import("${middlewareId}")`}`,
 		`});`,
 		`const _args = ${adapter.args ? JSON.stringify(adapter.args, null, 4) : 'undefined'};`,
 		adapter.exports

diff --git a/packages/astro/src/core/middleware/noop-middleware.ts b/packages/astro/src/core/middleware/noop-middleware.ts
@@ -0,0 +1,3 @@
+import type { MiddlewareHandler } from '../../@types/astro.js';
+
+export const NOOP_MIDDLEWARE_FN: MiddlewareHandler = (_, next) => next();
diff --git a/packages/astro/src/core/render-context.ts b/packages/astro/src/core/render-context.ts
@@ -67,7 +67,7 @@ export class RenderContext {
 	 */
 	counter = 0;
 
-	static create({
+	static async create({
 		locals = {},
 		middleware,
 		pathname,
@@ -77,11 +77,14 @@ export class RenderContext {
 		status = 200,
 		props,
 	}: Pick<RenderContext, 'pathname' | 'pipeline' | 'request' | 'routeData'> &
-		Partial<Pick<RenderContext, 'locals' | 'middleware' | 'status' | 'props'>>): RenderContext {
+		Partial<
+			Pick<RenderContext, 'locals' | 'middleware' | 'status' | 'props'>
+		>): Promise<RenderContext> {
+		const pipelineMiddleware = await pipeline.getMiddleware();
 		return new RenderContext(
 			pipeline,
 			locals,
-			sequence(...pipeline.internalMiddleware, middleware ?? pipeline.middleware),
+			sequence(...pipeline.internalMiddleware, middleware ?? pipelineMiddleware),
 			pathname,
 			request,
 			routeData,

diff --git a/packages/astro/src/vite-plugin-astro-server/plugin.ts b/packages/astro/src/vite-plugin-astro-server/plugin.ts
@@ -19,6 +19,7 @@ import { recordServerError } from './error.js';
 import { DevPipeline } from './pipeline.js';
 import { handleRequest } from './request.js';
 import { setRouteError } from './server-state.js';
+import { NOOP_MIDDLEWARE_FN }	from "../core/middleware/noop-middleware.js";
 
 export interface AstroPluginOptions {
 	settings: AstroSettings;
@@ -152,8 +153,10 @@ export function createDevelopmentManifest(settings: AstroSettings): SSRManifest
 		checkOrigin: settings.config.security?.checkOrigin ?? false,
 		experimentalEnvGetSecretEnabled: false,
 		key: createKey(),
-		middleware(_, next) {
-			return next();
+		middleware() {
+			return {
+				onRequest: NOOP_MIDDLEWARE_FN
+			};
 		},
 	};
 }
diff --git a/packages/astro/src/vite-plugin-astro-server/route.ts b/packages/astro/src/vite-plugin-astro-server/route.ts
@@ -198,7 +198,7 @@ export async function handleRoute({
 		matchedRoute.route.prerender &&
 		matchedRoute.route.component === DEFAULT_404_COMPONENT;
 
-	renderContext = RenderContext.create({
+	renderContext = await RenderContext.create({
 		locals,
 		pipeline,
 		pathname,
@@ -235,7 +235,7 @@ export async function handleRoute({
 			req({
 				url: pathname,
 				method: incomingRequest.method,
-				statusCode: isRewrite ? response.status : status ?? response.status,
+				statusCode: isRewrite ? response.status : (status ?? response.status),
 				isRewrite,
 				reqTime: timeEnd - timeStart,
 			}),

diff --git a/packages/astro/test/units/render/head.test.js b/packages/astro/test/units/render/head.test.js
@@ -103,7 +103,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.astro',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(PageModule);
 
 			const html = await response.text();
@@ -184,7 +184,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.astro',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(PageModule);
 
 			const html = await response.text();
@@ -232,7 +232,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.astro',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(PageModule);
 
 			const html = await response.text();

diff --git a/packages/astro/test/units/render/jsx.test.js b/packages/astro/test/units/render/jsx.test.js
@@ -50,7 +50,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.mdx',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(mod);
 
 			assert.equal(response.status, 200);
@@ -97,7 +97,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.mdx',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(mod);
 
 			assert.equal(response.status, 200);
@@ -128,7 +128,7 @@ describe('core/render', () => {
 				component: 'src/pages/index.mdx',
 				params: {},
 			};
-			const renderContext = RenderContext.create({ pipeline, request, routeData });
+			const renderContext = await RenderContext.create({ pipeline, request, routeData });
 			const response = await renderContext.render(mod);
 
 			try {

diff --git a/packages/astro/test/units/test-utils.js b/packages/astro/test/units/test-utils.js
@@ -13,6 +13,7 @@ import { nodeLogDestination } from '../../dist/core/logger/node.js';
 import { Pipeline } from '../../dist/core/render/index.js';
 import { RouteCache } from '../../dist/core/render/route-cache.js';
 import { unixify } from './correct-path.js';
+import {NOOP_MIDDLEWARE_FN} from "../../dist/core/middleware/noop-middleware.js";
 
 /** @type {import('../../src/core/logger/core').Logger} */
 export const defaultLogger = new Logger({
@@ -207,6 +208,9 @@ export function createBasicPipeline(options = {}) {
 	);
 	pipeline.headElements = () => ({ scripts: new Set(), styles: new Set(), links: new Set() });
 	pipeline.componentMetadata = () => new Map();
+	pipeline.getMiddleware = () => {
+		return NOOP_MIDDLEWARE_FN;
+	}
 	return pipeline;
 }
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,3 @@
		import type { MiddlewareHandler } from '../../@types/astro.js';

		export const NOOP_MIDDLEWARE_FN: MiddlewareHandler = (_, next) => next();