Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: singular middleware #9776

Merged
merged 4 commits into from
Jan 25, 2024
Merged

refactor: singular middleware #9776

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
92d7390
per-route middleware -> singular middleware
lilnasy Jan 15, 2024
ad1a731
add changeset
lilnasy Jan 22, 2024
2e6008a
emit middleware.mjs only when user middleware exists
lilnasy Jan 25, 2024
59b97de
Apply suggestions from code review
lilnasy Jan 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/twenty-papayas-agree.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"astro": patch
---

Simplifies internals that handle middleware.
2 changes: 2 additions & 0 deletions packages/astro/src/core/app/common.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,8 @@ export function deserializeManifest(serializedManifest: SerializedSSRManifest):
const clientDirectives = new Map(serializedManifest.clientDirectives);

return {
// in case user middleware exists, this no-op middleware will be reassigned (see plugin-ssr.ts)
middleware(_, next) { return next() },
...serializedManifest,
assets,
componentMetadata,
Expand Down
18 changes: 6 additions & 12 deletions packages/astro/src/core/app/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ export interface RenderErrorOptions {
response?: Response;
status: 404 | 500;
/**
* Whether to skip onRequest() while rendering the error page. Defaults to false.
* Whether to skip middleware while rendering the error page. Defaults to false.
*/
skipMiddleware?: boolean;
}
Expand Down Expand Up @@ -259,16 +259,10 @@ export class App {
this.#manifest.buildFormat
);
if (i18nMiddleware) {
if (mod.onRequest) {
this.#pipeline.setMiddlewareFunction(sequence(i18nMiddleware, mod.onRequest));
} else {
this.#pipeline.setMiddlewareFunction(i18nMiddleware);
}
this.#pipeline.setMiddlewareFunction(sequence(i18nMiddleware, this.#manifest.middleware));
this.#pipeline.onBeforeRenderRoute(i18nPipelineHook);
} else {
if (mod.onRequest) {
this.#pipeline.setMiddlewareFunction(mod.onRequest);
}
this.#pipeline.setMiddlewareFunction(this.#manifest.middleware);
}
response = await this.#pipeline.renderRoute(renderContext, pageModule);
} catch (err: any) {
Expand Down Expand Up @@ -428,8 +422,8 @@ export class App {
status
);
const page = (await mod.page()) as any;
if (skipMiddleware === false && mod.onRequest) {
this.#pipeline.setMiddlewareFunction(mod.onRequest);
if (skipMiddleware === false) {
this.#pipeline.setMiddlewareFunction(this.#manifest.middleware);
}
if (skipMiddleware) {
// make sure middleware set by other requests is cleared out
Expand All @@ -439,7 +433,7 @@ export class App {
return this.#mergeResponses(response, originalResponse);
} catch {
// Middleware may be the cause of the error, so we try rendering 404/500.astro without it.
if (skipMiddleware === false && mod.onRequest) {
if (skipMiddleware === false) {
return this.#renderError(request, {
status,
response: originalResponse,
Expand Down
4 changes: 3 additions & 1 deletion packages/astro/src/core/app/types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import type {
Locales,
MiddlewareHandler,
RouteData,
SerializedRouteData,
SSRComponentMetadata,
Expand Down Expand Up @@ -54,6 +55,7 @@ export type SSRManifest = {
pageModule?: SinglePageBuiltModule;
pageMap?: Map<ComponentPath, ImportComponentInstance>;
i18n: SSRManifestI18n | undefined;
middleware: MiddlewareHandler;
};

export type SSRManifestI18n = {
Expand All @@ -65,7 +67,7 @@ export type SSRManifestI18n = {

export type SerializedSSRManifest = Omit<
SSRManifest,
'routes' | 'assets' | 'componentMetadata' | 'clientDirectives'
'middleware' | 'routes' | 'assets' | 'componentMetadata' | 'clientDirectives'
> & {
routes: SerializedRouteInfo[];
assets: string[];
Expand Down
35 changes: 21 additions & 14 deletions packages/astro/src/core/build/generate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import type {
AstroSettings,
ComponentInstance,
GetStaticPathsItem,
MiddlewareHandler,
RouteData,
RouteType,
SSRError,
Expand Down Expand Up @@ -145,10 +146,17 @@ export async function generatePages(opts: StaticBuildOptions, internals: BuildIn
const baseDirectory = getOutputDirectory(opts.settings.config);
const renderersEntryUrl = new URL('renderers.mjs', baseDirectory);
const renderers = await import(renderersEntryUrl.toString());
let middleware: MiddlewareHandler = (_, next) => next();
try {
// 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
middleware = await import(new URL('middleware.mjs', baseDirectory).toString()).then(mod => mod.onRequest);
} catch {}
manifest = createBuildManifest(
opts.settings,
internals,
renderers.renderers as SSRLoadedRenderer[]
renderers.renderers as SSRLoadedRenderer[],
middleware
);
}
const pipeline = new BuildPipeline(opts, internals, manifest);
Expand Down Expand Up @@ -249,8 +257,9 @@ async function generatePage(
// prepare information we need
const logger = pipeline.getLogger();
const config = pipeline.getConfig();
const manifest = pipeline.getManifest();
const pageModulePromise = ssrEntry.page;
const onRequest = ssrEntry.onRequest;
const onRequest = manifest.middleware;
const pageInfo = getPageDataByComponent(pipeline.getInternals(), pageData.route.component);

// Calculate information of the page, like scripts, links and styles
Expand All @@ -263,19 +272,15 @@ async function generatePage(
const scripts = pageInfo?.hoistedScript ?? null;
// prepare the middleware
const i18nMiddleware = createI18nMiddleware(
pipeline.getManifest().i18n,
pipeline.getManifest().base,
pipeline.getManifest().trailingSlash,
pipeline.getManifest().buildFormat
manifest.i18n,
manifest.base,
manifest.trailingSlash,
manifest.buildFormat
);
if (config.i18n && i18nMiddleware) {
if (onRequest) {
pipeline.setMiddlewareFunction(sequence(i18nMiddleware, onRequest));
} else {
pipeline.setMiddlewareFunction(i18nMiddleware);
}
pipeline.setMiddlewareFunction(sequence(i18nMiddleware, onRequest));
pipeline.onBeforeRenderRoute(i18nPipelineHook);
} else if (onRequest) {
} else {
pipeline.setMiddlewareFunction(onRequest);
}
if (!pageModulePromise) {
Expand Down Expand Up @@ -629,10 +634,11 @@ function getPrettyRouteName(route: RouteData): string {
* @param settings
* @param renderers
*/
export function createBuildManifest(
function createBuildManifest(
settings: AstroSettings,
internals: BuildInternals,
renderers: SSRLoadedRenderer[]
renderers: SSRLoadedRenderer[],
middleware: MiddlewareHandler
): SSRManifest {
let i18nManifest: SSRManifestI18n | undefined = undefined;
if (settings.config.i18n) {
Expand Down Expand Up @@ -660,5 +666,6 @@ export function createBuildManifest(
componentMetadata: internals.componentMetadata,
i18n: i18nManifest,
buildFormat: settings.config.build.format,
middleware
};
}
24 changes: 10 additions & 14 deletions packages/astro/src/core/build/plugins/plugin-manifest.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,22 +42,18 @@ function vitePluginManifest(options: StaticBuildOptions, internals: BuildInterna
},
async load(id) {
if (id === RESOLVED_SSR_MANIFEST_VIRTUAL_MODULE_ID) {
const imports = [];
const contents = [];
const exports = [];
imports.push(
const imports = [
`import { deserializeManifest as _deserializeManifest } from 'astro/app'`,
`import { _privateSetManifestDontUseThis } from 'astro:ssr-manifest'`
);

contents.push(`
const manifest = _deserializeManifest('${manifestReplace}');
_privateSetManifestDontUseThis(manifest);
`);

exports.push('export { manifest }');

return `${imports.join('\n')}${contents.join('\n')}${exports.join('\n')}`;
];
const contents = [
`const manifest = _deserializeManifest('${manifestReplace}');`,
`_privateSetManifestDontUseThis(manifest);`
];
const exports = [
`export { manifest }`
];
return [...imports, ...contents, ...exports].join('\n');
}
},

Expand Down
21 changes: 0 additions & 21 deletions packages/astro/src/core/build/plugins/plugin-pages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,10 @@
import { extname } from 'node:path';
import type { Plugin as VitePlugin } from 'vite';
import type { AstroSettings } from '../../../@types/astro.js';
import { routeIsRedirect } from '../../redirects/index.js';
import { addRollupInput } from '../add-rollup-input.js';
import { eachPageFromAllPages, type BuildInternals } from '../internal.js';
import type { AstroBuildPlugin } from '../plugin.js';
import type { StaticBuildOptions } from '../types.js';
import { MIDDLEWARE_MODULE_ID } from './plugin-middleware.js';
import { RENDERERS_MODULE_ID } from './plugin-renderers.js';
import { ASTRO_PAGE_EXTENSION_POST_PATTERN, getPathFromVirtualModulePageName } from './util.js';

Expand Down Expand Up @@ -37,7 +35,6 @@ export function getVirtualModulePageIdFromPath(path: string) {
function vitePluginPages(opts: StaticBuildOptions, internals: BuildInternals): VitePlugin {
return {
name: '@astro/plugin-build-pages',

options(options) {
if (opts.settings.config.output === 'static') {
const inputs = new Set<string>();
Expand All @@ -52,13 +49,11 @@ function vitePluginPages(opts: StaticBuildOptions, internals: BuildInternals): V
return addRollupInput(options, Array.from(inputs));
}
},

resolveId(id) {
if (id.startsWith(ASTRO_PAGE_MODULE_ID)) {
return '\0' + id;
}
},

async load(id) {
if (id.startsWith(ASTRO_PAGE_RESOLVED_MODULE_ID)) {
const imports: string[] = [];
Expand All @@ -74,15 +69,6 @@ function vitePluginPages(opts: StaticBuildOptions, internals: BuildInternals): V
imports.push(`import { renderers } from "${RENDERERS_MODULE_ID}";`);
exports.push(`export { renderers };`);

// The middleware should not be imported by the pages
if (shouldBundleMiddleware(opts.settings)) {
const middlewareModule = await this.resolve(MIDDLEWARE_MODULE_ID);
if (middlewareModule) {
imports.push(`import { onRequest } from "${middlewareModule.id}";`);
exports.push(`export { onRequest };`);
}
}
Comment on lines -77 to -84
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why was this logic removed? A middleware shouldn't be part of an Astro route/page when we are in the edge

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a middleware is now never part of an Astro route/page

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious why too. Related to edge?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To detach middleware from routes. We had to import a route before importing middleware. Now, middleware lives in the manifest instead.

The snippet here serves the purpose of excluding middleware when it is supposed to go into edge middleware. That purpose is served by this line now.

https://github.com/withastro/astro/pull/9776/files#diff-524e25c0bd5feda71ed7489963414a0c1f06da3caa327933574a72b85b668bddR245


return `${imports.join('\n')}${exports.join('\n')}`;
}
}
Expand All @@ -91,13 +77,6 @@ function vitePluginPages(opts: StaticBuildOptions, internals: BuildInternals): V
};
}

export function shouldBundleMiddleware(settings: AstroSettings) {
if (settings.adapter?.adapterFeatures?.edgeMiddleware === true) {
return false;
}
return true;
}

export function pluginPages(opts: StaticBuildOptions, internals: BuildInternals): AstroBuildPlugin {
return {
targets: ['server'],
Expand Down
Loading
Loading