From 46d0a0b006044eb393a13720ab9600f7f643fcea Mon Sep 17 00:00:00 2001 From: Emanuele Stoppa Date: Fri, 25 Aug 2023 12:23:58 +0100 Subject: [PATCH] fix(ssr): correctly call the middleware when rendering error pages (#8200) --- packages/astro/src/core/app/index.ts | 23 +++++++++++++++---- packages/astro/src/core/app/types.ts | 1 + .../astro/src/core/build/buildPipeline.ts | 7 ++++++ packages/astro/src/core/build/generate.ts | 9 ++++---- .../src/core/build/plugins/plugin-manifest.ts | 14 +++++++++-- .../core/build/plugins/plugin-middleware.ts | 6 +++-- packages/astro/src/core/build/types.ts | 2 -- .../astro/src/core/redirects/component.ts | 1 - .../src/vite-plugin-astro-server/plugin.ts | 1 + .../fixtures/middleware-dev/src/middleware.js | 8 ++++--- .../middleware-dev/src/pages/404.astro | 6 +++++ packages/astro/test/middleware.test.js | 17 ++++++++++---- packages/astro/test/test-adapter.js | 12 ++++++++-- 13 files changed, 81 insertions(+), 26 deletions(-) create mode 100644 packages/astro/test/fixtures/middleware-dev/src/pages/404.astro diff --git a/packages/astro/src/core/app/index.ts b/packages/astro/src/core/app/index.ts index 566d0c60d..6b92bed92 100644 --- a/packages/astro/src/core/app/index.ts +++ b/packages/astro/src/core/app/index.ts @@ -9,7 +9,7 @@ import type { import type { SinglePageBuiltModule } from '../build/types'; import { getSetCookiesFromResponse } from '../cookies/index.js'; import { consoleLogDestination } from '../logger/console.js'; -import { error, type LogOptions } from '../logger/core.js'; +import { error, type LogOptions, warn } from '../logger/core.js'; import { collapseDuplicateSlashes, prependForwardSlash, @@ -56,6 +56,8 @@ export class App { }; #baseWithoutTrailingSlash: string; #pipeline: SSRRoutePipeline; + #onRequest: MiddlewareEndpointHandler | undefined; + #middlewareLoaded: boolean; constructor(manifest: SSRManifest, streaming = true) { this.#manifest = manifest; @@ -65,6 +67,7 @@ export class App { this.#routeDataToRouteInfo = new Map(manifest.routes.map((route) => [route.routeData, route])); this.#baseWithoutTrailingSlash = removeTrailingForwardSlash(this.#manifest.base); this.#pipeline = new SSRRoutePipeline(this.#createEnvironment(streaming)); + this.#middlewareLoaded = false; } set setManifest(newManifest: SSRManifest) { @@ -128,7 +131,21 @@ export class App { if (!routeData || routeData.prerender) return undefined; return routeData; } + + async #getOnRequest() { + if (this.#manifest.middlewareEntryPoint && !this.#middlewareLoaded) { + try { + const middleware = await import(this.#manifest.middlewareEntryPoint); + this.#pipeline.setMiddlewareFunction(middleware.onRequest as MiddlewareEndpointHandler); + } catch (e) { + warn(this.#logging, 'SSR', "Couldn't load the middleware entry point"); + } + } + this.#middlewareLoaded = true; + } + async render(request: Request, routeData?: RouteData, locals?: object): Promise { + await this.#getOnRequest(); // Handle requests with duplicate slashes gracefully by cloning with a cleaned-up request URL if (request.url !== collapseDuplicateSlashes(request.url)) { request = new Request(collapseDuplicateSlashes(request.url), request); @@ -156,10 +173,6 @@ export class App { ); let response; try { - // NOTE: ideally we could set the middleware function just once, but we don't have the infrastructure to that yet - if (mod.onRequest) { - this.#pipeline.setMiddlewareFunction(mod.onRequest as MiddlewareEndpointHandler); - } response = await this.#pipeline.renderRoute(renderContext, pageModule); } catch (err: any) { if (err instanceof EndpointNotFoundError) { diff --git a/packages/astro/src/core/app/types.ts b/packages/astro/src/core/app/types.ts index 8812d2c44..faeb9377e 100644 --- a/packages/astro/src/core/app/types.ts +++ b/packages/astro/src/core/app/types.ts @@ -49,6 +49,7 @@ export type SSRManifest = { componentMetadata: SSRResult['componentMetadata']; pageModule?: SinglePageBuiltModule; pageMap?: Map; + middlewareEntryPoint: string | undefined; }; export type SerializedSSRManifest = Omit< diff --git a/packages/astro/src/core/build/buildPipeline.ts b/packages/astro/src/core/build/buildPipeline.ts index 5815fa5f5..ea6e9ecf4 100644 --- a/packages/astro/src/core/build/buildPipeline.ts +++ b/packages/astro/src/core/build/buildPipeline.ts @@ -77,6 +77,13 @@ export class BuildPipeline extends Pipeline { return this.#manifest; } + async retrieveMiddlewareFunction() { + if (this.#internals.middlewareEntryPoint) { + const middleware = await import(this.#internals.middlewareEntryPoint.toString()); + this.setMiddlewareFunction(middleware.onRequest); + } + } + /** * The SSR build emits two important files: * - dist/server/manifest.mjs diff --git a/packages/astro/src/core/build/generate.ts b/packages/astro/src/core/build/generate.ts index 224b96b63..73bb4a40f 100644 --- a/packages/astro/src/core/build/generate.ts +++ b/packages/astro/src/core/build/generate.ts @@ -10,7 +10,6 @@ import type { ComponentInstance, GetStaticPathsItem, ImageTransform, - MiddlewareEndpointHandler, RouteData, RouteType, SSRError, @@ -138,6 +137,7 @@ export async function generatePages(opts: StaticBuildOptions, internals: BuildIn ); } const buildPipeline = new BuildPipeline(opts, internals, manifest); + await buildPipeline.retrieveMiddlewareFunction(); const outFolder = ssr ? opts.settings.config.build.server : getOutDirWithinCwd(opts.settings.config.outDir); @@ -248,10 +248,6 @@ async function generatePage( .reduce(mergeInlineCss, []); const pageModulePromise = ssrEntry.page; - const onRequest = ssrEntry.onRequest; - if (onRequest) { - pipeline.setMiddlewareFunction(onRequest as MiddlewareEndpointHandler); - } if (!pageModulePromise) { throw new Error( @@ -612,5 +608,8 @@ export function createBuildManifest( ? new URL(settings.config.base, settings.config.site).toString() : settings.config.site, componentMetadata: internals.componentMetadata, + middlewareEntryPoint: internals.middlewareEntryPoint + ? internals.middlewareEntryPoint.toString() + : undefined, }; } diff --git a/packages/astro/src/core/build/plugins/plugin-manifest.ts b/packages/astro/src/core/build/plugins/plugin-manifest.ts index a7b254554..1364ebf1e 100644 --- a/packages/astro/src/core/build/plugins/plugin-manifest.ts +++ b/packages/astro/src/core/build/plugins/plugin-manifest.ts @@ -93,14 +93,19 @@ export function pluginManifest( } const manifest = await createManifest(options, internals); + const shouldPassMiddlewareEntryPoint = + // TODO: remove in Astro 4.0 + options.settings.config.build.excludeMiddleware || + options.settings.adapter?.adapterFeatures?.edgeMiddleware; await runHookBuildSsr({ config: options.settings.config, manifest, logging: options.logging, entryPoints: internals.entryPoints, - middlewareEntryPoint: internals.middlewareEntryPoint, + middlewareEntryPoint: shouldPassMiddlewareEntryPoint + ? internals.middlewareEntryPoint + : undefined, }); - // TODO: use the manifest entry chunk instead const code = injectManifest(manifest, internals.manifestEntryChunk); mutate(internals.manifestEntryChunk, 'server', code); }, @@ -232,6 +237,10 @@ function buildManifest( // Set this to an empty string so that the runtime knows not to try and load this. entryModules[BEFORE_HYDRATION_SCRIPT_ID] = ''; } + const isEdgeMiddleware = + // TODO: remove in Astro 4.0 + settings.config.build.excludeMiddleware || + settings.adapter?.adapterFeatures?.edgeMiddleware; const ssrManifest: SerializedSSRManifest = { adapterName: opts.settings.adapter?.name ?? '', @@ -245,6 +254,7 @@ function buildManifest( clientDirectives: Array.from(settings.clientDirectives), entryModules, assets: staticFiles.map(prefixAssetPath), + middlewareEntryPoint: !isEdgeMiddleware ? internals.middlewareEntryPoint?.toString() : undefined, }; return ssrManifest; diff --git a/packages/astro/src/core/build/plugins/plugin-middleware.ts b/packages/astro/src/core/build/plugins/plugin-middleware.ts index 5ed532d5e..7c945740c 100644 --- a/packages/astro/src/core/build/plugins/plugin-middleware.ts +++ b/packages/astro/src/core/build/plugins/plugin-middleware.ts @@ -4,6 +4,7 @@ import { addRollupInput } from '../add-rollup-input.js'; import type { BuildInternals } from '../internal'; import type { AstroBuildPlugin } from '../plugin'; import type { StaticBuildOptions } from '../types'; +import { getOutputDirectory } from '../../../prerender/utils.js'; export const MIDDLEWARE_MODULE_ID = '@astro-middleware'; @@ -56,8 +57,9 @@ export function vitePluginMiddleware( if (chunk.type === 'asset') { continue; } - if (chunk.fileName === 'middleware.mjs' && opts.settings.config.build.excludeMiddleware) { - internals.middlewareEntryPoint = new URL(chunkName, opts.settings.config.build.server); + if (chunk.fileName === 'middleware.mjs') { + const outputDirectory = getOutputDirectory(opts.settings.config); + internals.middlewareEntryPoint = new URL(chunkName, outputDirectory); } } }, diff --git a/packages/astro/src/core/build/types.ts b/packages/astro/src/core/build/types.ts index 472dc4b34..35451cacb 100644 --- a/packages/astro/src/core/build/types.ts +++ b/packages/astro/src/core/build/types.ts @@ -4,7 +4,6 @@ import type { AstroSettings, ComponentInstance, ManifestData, - MiddlewareHandler, RouteData, RuntimeMode, SSRLoadedRenderer, @@ -52,7 +51,6 @@ export interface SinglePageBuiltModule { /** * The `onRequest` hook exported by the middleware */ - onRequest?: MiddlewareHandler; renderers: SSRLoadedRenderer[]; } diff --git a/packages/astro/src/core/redirects/component.ts b/packages/astro/src/core/redirects/component.ts index d10cae4fe..93b1aa732 100644 --- a/packages/astro/src/core/redirects/component.ts +++ b/packages/astro/src/core/redirects/component.ts @@ -12,6 +12,5 @@ export const RedirectComponentInstance: ComponentInstance = { export const RedirectSinglePageBuiltModule: SinglePageBuiltModule = { page: () => Promise.resolve(RedirectComponentInstance), - onRequest: (ctx, next) => next(), renderers: [], }; diff --git a/packages/astro/src/vite-plugin-astro-server/plugin.ts b/packages/astro/src/vite-plugin-astro-server/plugin.ts index f7f7b0068..374f31fec 100644 --- a/packages/astro/src/vite-plugin-astro-server/plugin.ts +++ b/packages/astro/src/vite-plugin-astro-server/plugin.ts @@ -99,5 +99,6 @@ export function createDevelopmentManifest(settings: AstroSettings): SSRManifest ? new URL(settings.config.base, settings.config.site).toString() : settings.config.site, componentMetadata: new Map(), + middlewareEntryPoint: undefined, }; } diff --git a/packages/astro/test/fixtures/middleware-dev/src/middleware.js b/packages/astro/test/fixtures/middleware-dev/src/middleware.js index 854c997c1..eeb902fb8 100644 --- a/packages/astro/test/fixtures/middleware-dev/src/middleware.js +++ b/packages/astro/test/fixtures/middleware-dev/src/middleware.js @@ -18,18 +18,20 @@ const first = defineMiddleware(async (context, next) => { return new Response(JSON.stringify(object), { headers: response.headers, }); - } else if(context.url.pathname === '/clone') { + } else if (context.url.pathname === '/clone') { const response = await next(); const newResponse = response.clone(); const /** @type {string} */ html = await newResponse.text(); const newhtml = html.replace('

testing

', '

it works

'); return new Response(newhtml, { status: 200, headers: response.headers }); } else { - if(context.url.pathname === '/') { + if (context.url.pathname === '/') { context.cookies.set('foo', 'bar'); } - context.locals.name = 'bar'; + context.locals = { + name: 'bar', + }; } return await next(); }); diff --git a/packages/astro/test/fixtures/middleware-dev/src/pages/404.astro b/packages/astro/test/fixtures/middleware-dev/src/pages/404.astro new file mode 100644 index 000000000..ea85d240c --- /dev/null +++ b/packages/astro/test/fixtures/middleware-dev/src/pages/404.astro @@ -0,0 +1,6 @@ +--- +const name = Astro.locals.name; +--- + +Error +

{name}

diff --git a/packages/astro/test/middleware.test.js b/packages/astro/test/middleware.test.js index 3796a341f..b09c686f4 100644 --- a/packages/astro/test/middleware.test.js +++ b/packages/astro/test/middleware.test.js @@ -211,6 +211,16 @@ describe('Middleware API in PROD mode, SSR', () => { expect(text.includes('REDACTED')).to.be.true; }); + it('should correctly call the middleware function for 404', async () => { + const app = await fixture.loadTestAdapterApp(); + const request = new Request('http://example.com/funky-url'); + const routeData = app.match(request, { matchNotFound: true }); + const response = await app.render(request, routeData); + const text = await response.text(); + expect(text.includes('Error')).to.be.true; + expect(text.includes('bar')).to.be.true; + }); + it('the integration should receive the path to the middleware', async () => { fixture = await loadFixture({ root: './fixtures/middleware-dev/', @@ -219,10 +229,8 @@ describe('Middleware API in PROD mode, SSR', () => { excludeMiddleware: true, }, adapter: testAdapter({ - setEntryPoints(entryPointsOrMiddleware) { - if (entryPointsOrMiddleware instanceof URL) { - middlewarePath = entryPointsOrMiddleware; - } + setMiddlewareEntryPoint(entryPointsOrMiddleware) { + middlewarePath = entryPointsOrMiddleware; }, }), }); @@ -237,6 +245,7 @@ describe('Middleware API in PROD mode, SSR', () => { throw e; } }); + }); describe('Middleware with tailwind', () => { diff --git a/packages/astro/test/test-adapter.js b/packages/astro/test/test-adapter.js index 67058023d..0090b6d9d 100644 --- a/packages/astro/test/test-adapter.js +++ b/packages/astro/test/test-adapter.js @@ -5,7 +5,13 @@ import { viteID } from '../dist/core/util.js'; * @returns {import('../src/@types/astro').AstroIntegration} */ export default function ( - { provideAddress = true, extendAdapter, setEntryPoints = undefined, setRoutes = undefined } = { + { + provideAddress = true, + extendAdapter, + setEntryPoints = undefined, + setMiddlewareEntryPoint = undefined, + setRoutes = undefined, + } = { provideAddress: true, } ) { @@ -86,7 +92,9 @@ export default function ( 'astro:build:ssr': ({ entryPoints, middlewareEntryPoint }) => { if (setEntryPoints) { setEntryPoints(entryPoints); - setEntryPoints(middlewareEntryPoint); + } + if (setMiddlewareEntryPoint) { + setMiddlewareEntryPoint(middlewareEntryPoint); } }, 'astro:build:done': ({ routes }) => {