-
-
Notifications
You must be signed in to change notification settings - Fork 3k
feat: add support for running full middleware on static pages #14617
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.
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
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 75fd088 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 |
CodSpeed Performance ReportMerging #14617 will not alter performanceComparing Summary
Footnotes |
|
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. |
|
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. |
|
No problem, enjoy! Would you mind converting it back to draft for now? Helps us while triaging/reviewing |
|
So we didn't take any decision on what direction this should take, but people will look at it |
|
Hi @florian-lefebvre 👋 |
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
runMiddlewareOnRequestuser 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
middlewarepage 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!