refactor: singular middleware #9776

lilnasy · 2024-01-23T00:05:21Z

Changes

The onRequest function is internally re-exported from each page and endpoint, and loaded after the request matches a particular one.
The middleware is singular in astro and the current arrangement prevents enhancement that require the middleware to be independent.
This PR moves onRequest from page chunks to the manifest, where it can be loaded sooner than route data.

Testing

Does not affect behavior. Existing tests should pass.

Docs

Does not affect usage.

changeset-bot · 2024-01-23T00:05:27Z

🦋 Changeset detected

Latest commit: 59b97de

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

ematipico

Here's some feedback:

let's revert the changes around i18n, they are intentional, and they aren't part of the middleware work
I don't like that we create a middleware by default because this will always create a middleware.mjs file, which is weird is a user doesn't use a middleware at all. The fact that onRequest can be undefined was a good compromise
I have some concerns on how this change affects edge middleware

ematipico · 2024-01-23T10:37:40Z

.changeset/twenty-papayas-agree.md

I don't think we need a changeset if the code is just a refactor and it doesn't change/fix anything for the users

I went back and forth on this. We can't say ever say for sure, there won't be regression and the changelog comes in handy. At the end, it's just another entry in the changelog.

ematipico · 2024-01-23T10:39:57Z

packages/astro/src/core/build/plugins/plugin-manifest.ts

@@ -263,7 +248,7 @@ function buildManifest(
 		clientDirectives: Array.from(settings.clientDirectives),
 		entryModules,
 		assets: staticFiles.map(prefixAssetPath),
-		i18n: i18nManifest,
+		i18n: settings.config.i18n,


Let's revert this change. It's intentional to pass only a subset of information in the SSR manifest.

ematipico · 2024-01-23T10:41:59Z

packages/astro/src/core/build/plugins/plugin-pages.ts

-						// 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 };`);
-							}
-						}


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

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

Curious why too. Related to edge?

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

packages/astro/src/core/middleware/vite-plugin.ts

ematipico · 2024-01-23T10:47:12Z

packages/astro/src/vite-plugin-astro-server/plugin.ts

@@ -139,6 +129,9 @@ export function createDevelopmentManifest(settings: AstroSettings): SSRManifest
 			? new URL(settings.config.base, settings.config.site).toString()
 			: settings.config.site,
 		componentMetadata: new Map(),
-		i18n: i18nManifest,
+		i18n: settings.config.i18n,


lilnasy · 2024-01-23T14:15:27Z

let's revert the changes around i18n

This is a stop-gap change. Soon enough, it will not even be in the manifest.

I don't like that we create a middleware by default

There is a good reason for it. Middleware is a no-op in this PR but will soon contain internal rendering logic that goes in the "middle".

As a bonus, we get to delete several nested if-else branches dealing with the undefined that are duplicated in multiple places.

I have some concerns on how this change affects edge middleware

Could you elaborate?

ematipico · 2024-01-23T14:34:28Z

let's revert the changes around i18n

This is a stop-gap change. Soon enough, it will not even be in the manifest.

I want to revert these changes: i18n: settings.config.i18n,. This was intentional, I wanted to store in the manifest only information that are needed by the astro/app. For example, i18n.fallback isn't needed, so we shouldn't store it.

I have some concerns on how this change affects edge middleware

Could you elaborate?

In the adapters, we create an edge middleware if edgeMiddleware is true and if middlewareEntryPoint exists. With this change, we now emit a middlewareEntryPoint because it always contains at least a no-op function. This means that now we always create an edge function, even if there isn't a middleware.

lilnasy · 2024-01-23T15:05:35Z

This means that now we always create an edge function, even if there isn't a middleware.

That's true, unfortunately. I'll take a look.

matthewp · 2024-01-23T15:22:30Z

packages/astro/src/core/build/generate.ts

@@ -145,10 +145,12 @@ 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());
+		const { onRequest: middleware } = await import(new URL('middleware.mjs', baseDirectory).toString());


This is new, can you explain why this is imported this way? I can see that it's passed into createBuildManifest, how was the middleware previously imported in generation? I don't see an old import() that was removed.

Previously, manifest was picked up from ssrEntry.

https://github.com/withastro/astro/pull/9776/files#diff-10cec9990a82f35ed4ce5e0137005fc5035b0291910a4162f1ff0c5adbf2a8efL253

Here, it is being imported in the same way as renderers.

ematipico · 2024-01-23T16:19:25Z

This means that now we always create an edge function, even if there isn't a middleware.

That's true, unfortunately. I'll take a look.

We could also cheat, and set middleEntrypoint to undefined if we know that we are emitting a no-op

lilnasy · 2024-01-23T16:27:57Z

We could also cheat, and set middleEntrypoint to undefined if we know that we are emitting a no-op

That's my temptation as well!

I want to sit on it for a day. If nothing else pops up in my head, I'll go ahead with that.

lilnasy · 2024-01-25T16:06:34Z

I want to sit on it for a day. If nothing else pops up in my head, I'll go ahead with that.

Implemented a no-compromise solution - the middleware function will always exist, middleware.mjs may not.

packages/astro/src/core/build/generate.ts

natemoo-re

LGTM! Nice refactors and I'm satisfied that you've addressed all feedback.

natemoo-re · 2024-01-25T18:35:13Z

packages/astro/src/core/build/plugins/plugin-ssr.ts

+	];
+
+	const contents = [
+		edgeMiddleware ? `const middleware = (_, next) => next()` : '',


Clever! I think this makes a lot of sense.

github-actions bot added the pkg: astro Related to the core `astro` package (scope) label Jan 23, 2024

ematipico requested changes Jan 23, 2024

View reviewed changes

lilnasy force-pushed the refactor-singular-middleware branch from 9459137 to 054460e Compare January 23, 2024 14:51

matthewp reviewed Jan 23, 2024

View reviewed changes

lilnasy added 3 commits January 25, 2024 14:48

per-route middleware -> singular middleware

92d7390

add changeset

ad1a731

emit middleware.mjs only when user middleware exists

2e6008a

lilnasy force-pushed the refactor-singular-middleware branch from 054460e to 2e6008a Compare January 25, 2024 15:31

ematipico approved these changes Jan 25, 2024

View reviewed changes

packages/astro/src/core/build/generate.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

59b97de

lilnasy force-pushed the refactor-singular-middleware branch from 88cb7b3 to 59b97de Compare January 25, 2024 18:03

natemoo-re approved these changes Jan 25, 2024

View reviewed changes

matthewp approved these changes Jan 25, 2024

View reviewed changes

lilnasy merged commit dc75180 into withastro:main Jan 25, 2024
13 checks passed

lilnasy deleted the refactor-singular-middleware branch January 25, 2024 19:38

astrobot-houston mentioned this pull request Jan 25, 2024

[ci] release #9802

Merged

lilnasy mentioned this pull request Feb 1, 2024

fix(middleware): load while retrieving ssr manifest for prerendering #9938

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: singular middleware #9776

refactor: singular middleware #9776

lilnasy commented Jan 23, 2024

changeset-bot bot commented Jan 23, 2024 •

edited

Loading

ematipico left a comment •

edited

Loading

ematipico Jan 23, 2024

lilnasy Jan 23, 2024

ematipico Jan 23, 2024

ematipico Jan 23, 2024

lilnasy Jan 23, 2024

matthewp Jan 23, 2024

lilnasy Jan 23, 2024

ematipico Jan 23, 2024

lilnasy commented Jan 23, 2024

ematipico commented Jan 23, 2024 •

edited

Loading

lilnasy commented Jan 23, 2024

matthewp Jan 23, 2024

lilnasy Jan 23, 2024

ematipico commented Jan 23, 2024

lilnasy commented Jan 23, 2024

lilnasy commented Jan 25, 2024

natemoo-re left a comment

natemoo-re Jan 25, 2024

refactor: singular middleware #9776

refactor: singular middleware #9776

Conversation

lilnasy commented Jan 23, 2024

Changes

Testing

Docs

changeset-bot bot commented Jan 23, 2024 • edited Loading

🦋 Changeset detected

ematipico left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilnasy commented Jan 23, 2024

ematipico commented Jan 23, 2024 • edited Loading

lilnasy commented Jan 23, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ematipico commented Jan 23, 2024

lilnasy commented Jan 23, 2024

lilnasy commented Jan 25, 2024

natemoo-re left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

changeset-bot bot commented Jan 23, 2024 •

edited

Loading

ematipico left a comment •

edited

Loading

ematipico commented Jan 23, 2024 •

edited

Loading