feat: add support for running full middleware on static pages

[leekeh]

Note

Please feel free to directly add to this PR to address any issues, I am absent for the month of November.

Changes

This pr adds the runMiddlewareOnRequest user configuration to the node adapter, which means that middleware runs on each user request, also for prerendered pages. This feature can be added to other adapters easily.

I did some refactoring so we reuse the same logic for static servers with middleware and app server middleware.

It is not a middleware feature, more of a server mode, which allows it to be used in any environment, not just edge-powered integrations like Netlify & Vercel. The reasoning here is that we still benefit from the prerendered pages, which reduces the effort of rendering for each request, but we can run normal server middleware next to it to handle things like authentication.

When this option is enabled, middleware does not run during the initial generation. This is done to prevent confusion and hard redirects. It is not the only way to do this, it could also be decided to allow the middleware to run both during generation and serve, but then we need an additional mechanism for the user to define which run where, which could make the API confusing. As the main use case for this feature is to be running auth checks on static pages, I argue it makes most sense to only run the middleware during serve.

Accessing dynamic context like cookies or context.locals on static pages will still not work, because the pages are still generated during the build step. The key difference is just that we run the middleware before running the static page is returned, allowing us to redirect or return a non-200 response code depending on the user context.

Relates to this discussion: withastro/roadmap#869

Testing

Full sample implementation was added under examples/static-with-middleware.

Tests were added in middleware-static-pages.test.js.

Docs

This is an opt-in behavior, but it can be helpful to add a section in the documentation to show how it works. The middleware page could be updated, but it might be better to only do this when other adapters have this functionality too.
/cc @withastro/maintainers-docs for feedback!

[changeset-bot]

⚠️ No Changeset found

Latest commit: 8068bad

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

Click here to learn what changesets are, and how to add one.

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

[codspeed-hq]

CodSpeed Performance Report

Merging #14617 will not alter performance

_{Comparing leekeh:main (75fd088) with main (f657183)¹}

Summary

✅ 6 untouched

Footnotes

1. No successful run was found on main (c4816e8) during the generation of this report, so f657183 was used instead as the comparison base. There might be some changes unrelated to this pull request in this report. ↩

[florian-lefebvre]

Hi thanks for starting this! I think we'd prefer to keep this as a node adapter only feature instead of modifying core, do you think it's possible?

[leekeh]

Hi thanks for starting this! I think we'd prefer to keep this as a node adapter only feature instead of modifying core, do you think it's possible?

For sure we can, I'd just be concerned about reusability. The node app server borrows from the core for how middleware is run and locals/cookies are updated, so I thought it would be more maintainable if we can reuse some logic across static and app servers. But if it is too invasive, it can be a later enhancement.

[florian-lefebvre]

I'll bring it up during tomorrow's standup and let you know

[leekeh]

I'll bring it up during tomorrow's standup and let you know

Thanks. I will be out of office for a while and will unplug, so others are free to continue my work. Otherwise, I can get back on it in December.

[florian-lefebvre]

No problem, enjoy! Would you mind converting it back to draft for now? Helps us while triaging/reviewing

[florian-lefebvre]

So we didn't take any decision on what direction this should take, but people will look at it

[thibault-mahe]

Hi @florian-lefebvre 👋
I'm working with @leekeh and I can progress on this PR while she's absent if it can help you.
Let me know when the PR is checked on your side and if I can be of any help, this feature might help us (and other users I'm sure) a lot :)

[leekeh]

So we didn't take any decision on what direction this should take, but people will look at it

Hi hi, I'm back. Hope you are doing well 😄 Do you have any news on the desired direction? Perhaps if it is too big of a decision, I can refactor it now to just be a node adapter only feature, and it can be cleaned up later. Let me know what you think :)

[ematipico]

Do you have any news on the desired direction?

It's a prominent feature that we want; however, at the moment we don't have the capacity to look at it, because we are busy with Astro 6 and internal refactors that involve the Vite Environment APIs.

For example, we refactored the whole rendering pipeline (e.g. app/index.ts), so taking this feature in might create disruptions to the mentioned developments.

From my first review (and the rest to the team agrees), we felt that this feature touches too much core, even though I understand why you introduced things like skipMiddlewareOnPrerender.

However, we also understand that the adapter can't use the edgeMiddleware adapter feature because this feature removes the middleware entirely from the rendering chain, which means SSR pages won't run the middleware anymore.

As for my personal advice, I would break down this PR in multiple PRs. As far I see the feature, I foresee the following:

1. Create a new adapter feature called middleware-runtime (or a better name). It's a enum that has the following variants:
   • edge: same as edgeMiddleware
   • classic: (default) same as today. It runs on static pages during the build. It runs on dynamic pages after the build
   • strict (or something else): It runs on static pages during the build. It runs on all pages after the build.
   • server-only (or something else): It doesn't run during the build. It runs on all pages after the build
2. Deprecate edgeMiddleware in favour of the new variant
3. Implement the new feature for the Node.js adapter

We are almost done with the refactors, so maybe after we merge the PR and we do a release, you could restart the works against the next branch, so you get familiar with the new rendering pipeline.