Closed matthewp closed 1 year ago
If the middleware returned a response with a non-html body on SSG, should it ignore the path or throw an error? I think currently Astro throws an error if you try to build a page that returns a redirect response?
@pilcrowOnPaper
I am collecting information on different fronts around Middleware. I want to ask you about a couple of things since you are one of the people who made the proposal.
How would a middleware work in SSG? What's an actual use case that would fit this scenario? While you provided an example of how an API will look, we should also provide an actual use case. This is helpful to shape the API and understand if it's needed.
How would a middleware work in SSR? What's an actual use case that would fit this scenario? Considering that "middleware routing" is out of scope, there should be some usage that doesn't involve "sessions" or "login information."
@ematipico
Good point, for SSG (and SSR), one use can be post-processing the response html. Since this proposal also includes locals
, you can define shared state in the middleware and have it accessible across routes, which would be useful both for SSR and SSG.
Considering that "middleware routing" is out of scope
I'm not sure where you got this from? By "Route specific middleware," I mean middleware that only runs on specific routes (like /some-route/*
)
@ematipico
Good point, for SSG (and SSR), one use can be post-processing the response html. Since this proposal also includes
locals
, you can define shared state in the middleware and have it accessible across routes, which would be useful both for SSR and SSG.
Thank you! Specifically, if you were a user, how would you use locals
in SSG? For what and how?
Considering that "middleware routing" is out of scope
I'm not sure where you got this from? By "Route specific middleware," I mean middleware that only runs on specific routes (like
/some-route/*
)
Thank you for clarifying!
@ematipico Another thing to think about, Vercel has a way to specific certain routes that run at the edge. Could/would we want our middleware layer to run at the edge if using the Vercel adapter? https://vercel.com/docs/build-output-api/v3#features/edge-middleware
@matthewp
Can you share state between 2 runtimes? If not, locals
would not work as expected.
@ematipico
I can't think of any specific examples right now for SSG, though I'm sure there are times where you want to run the same function between routes. I just think it doesn't make sense to limit middleware to SSR and have more SSR specific features.
A prototype is available here:
npm i astro@0.0.0-middleware-20230331181015
The prototype mostly follows the RFC, a small deviation:
onRequest
function and onError
function;onError
is not used yet in the prototypeexport default defineConfig({
middlewareOrder: ['auth', 'server']
})
sequence
function is not exposed yetProbably okay to leave as-is given the good convo already kicked off, but for future proposals: we discussed reducing the effort on Stage 1/2 and moving away from including a specific solution in the OP to just including background/goals/non-goals. Solution ideas and discussion can go in a comment below (optional) at anytime, or just the RFC.
Two examples:
@ematipico any reason for the deviations you made? It would be helpful to evaluate them. Without context, I find the RFC API much more friendly vs. having to couple function names to a special string array in astro.config.js
. I don't understand the need for onRequest
or onError
either (why a special API vs. the try-catch
the developer is used to).
But, you also mention the sequence
function as not exposed yet, which would get rid of middlewareOrder
(since it defines a sequence) so maybe this is just a placeholder. If the RFC is still the final intended outcome and middlewareOrder
is just a placeholder for your MVP, then no need to address this feedback!
@matthewp on your proposal: you mention middleware
is an array, but then export function middleware
. Assuming typo? One thing I like about sequence
is that it basically lets you be more strict that middleware is always a function, and use sequence
to wrap multiple middleware in a single function.
Vercel has a way to specific certain routes that run at the edge. Could/would we want our middleware layer to run at the edge if using the Vercel adapter?
Big +1 for this. It's not just Vercel. Cloudflare is another example where they have the ability to automatically choose the best location for your worker to run in. Splitting middleware into its own cloud function would let you run the middleware fn
near your user, and the main app fn
near your DB.
It might be good to explicitly say that implementing either of these is out of scope, but that handling this future use-case in the API design now (ex: allowing someone else to implement it with this API) is in scope for this RFC, if at all possible.
I'm not a big fan of using configuration to define middleware. It should be handled locally (within middleware.ts
) whenever possible, and using type-gen or something to type check middlewareOrder
config seems excessive.
I don't understand the need for onRequest or onError either
Agree with @FredKSchott. No need to make it complicated.
Vercel has a way to specific certain routes that run at the edge. Could/would we want our middleware layer to run at the edge if using the Vercel adapter?
I'd love to have this as a configurable option if locals
work as intended. That said, I think it should be left out of this RFC since it's not something critical.
The latest PR/preview release now has an identical implementation based on this RFC, with execption for the function called onRequest
.
The sequence
function is exported and I made sure that it works as intended.
@pilcrowOnPaper what is the difference between exporting an array of middleware vs. using sequence
?
@matthewp That's a mistake! I think I accidentally included both. I prefer using sequence()
since you can nest them:
export const middleware: Middleware = sequence(sequence());
I also think it makes more sense that there's only a single middleware function.
Also, would a default export work better if the file name is middleware.ts
?
const middleware: Middleware = async () => {
// ...
};
export default middleware;
@pilcrowOnPaper I was worried about nesting. My concern is that people will expect:
const a = () => { ... };
const b = () => { ... };
const c = () => { ... };
const d = sequence(a, b);
export const middleware = sequence(b, c, d);
Above b
is added twice, in the d
sequence and the other exported sequence. Is the expectation that b
only runs once? I'm worried we'll create footguns if we create that expectation.
@matthewp The expectation (or at least mine's) is that it'll run twice.
The latest PR/preview release now has an identical implementation based on this RFC, with execption for the function called
onRequest
.The
sequence
function is exported and I made sure that it works as intended.
could you show a snippet of how you set this up since your explanations are a little bit confusing
The latest PR/preview release now has an identical implementation based on this RFC, with execption for the function called
onRequest
. Thesequence
function is exported and I made sure that it works as intended.could you show a snippet of how you set this up since your explanations are a little bit confusing
I opened a PR: https://github.com/withastro/astro/pull/6721
In the body of the issue I put some example. The npm tag to use is the following: 0.0.0-middleware-20230403121346
Here's a link to the npm package: https://www.npmjs.com/package/astro/v/0.0.0-middleware-20230403121346
Warning: the middleware works only in DEV mode for now
A few pieces of feedback after playing with this preview:
We should expose Astro's Locals
type as an interface
so that user code can augment it like so:
declare module 'astro' {
interface Locals {
foo: string
}
}
We should possibly rename resolve
to next
because that's what most users are familiar with.
We should possibly explore removing the requirement that resolve(context)
needs to be passed the full context
object. That's not a pattern I've seen anywhere else. It's repetitive and makes destructuring context
very inconvenient.
Should we warn if a user tries to assign context.locals
to something other than an object?
Here's some feedback of mine from trying out the APIs:
new Response
instead of mutating the provided one.resolve
was a little confusing and I wasn't expecting to have to call it. As soon as it was described as being like next()
from Express it made sense that you needed to. Maybe it should be renamed.context
as an argument be required, and the need to do so makes it harder to do destructing.context.locals
, ala context.locals = { }
. While I can see that it might be nice to blow away previous values, I don't think this is what you want most of the time. Most of the time a middleware is its own isolated thing and doesn't know about other middleware. Because of that I would expect you to mutate the locals
object. So I think we should at the very least document it as being mutated. And I wouldn't be opposed to making the object writable: false
to avoid someone mistakenly overriding the value. It might be safer to do so, and if use-cases for blowing it away come up we can always relax that restriction.locals
object.cookies
and if a cookie is missing, return a new Response 405
status.I think you should be able to return a new Response instead of mutating the provided one.
Should've included that in the RFC, but that was my intention.
On renaming resolve
to next
, the concepts are similar and using next()
is definitely more familiar, but I'm worried people are going to do something like this:
const middleware = async (context, next) => {
next()
}
Though TypeScript should catch the error.
It's repetitive and makes destructuring context very inconvenient.
Ah yeah, agreed that we should make it so we don't have to pass on context
to resolve()
.
The examples should override the value of context.locals, ala context.locals = { }.
@matthewp I'm kinda lost as to where this is pointing to
@matthewp I'm kinda lost as to where this is pointing to
To be more clear, the team felt kind worried about the possibility to override the locals
object with anything.
For example, we could have two or more middleware where each one is in charge to change the locals. Then, a user, by mistake does this at the very end:
context.locals = 155;
This piece of code would make locals
a number, not an object anymore.
The team felt that these kinds of scenarios should be prevented at runtime somehow.
Ah, gotcha. I was kinda confused since I wrongly assumed setting context.locals
to writable: false
would set content.locals
as readonly object.
@pilcrowOnPaper Setting it to writable: false
just means you can't assign the locals
property on context
. It doesn't freeze the object. But currently this isn't done and you can assign the property to another value.
Some use-cases and how I would think to implement them:
export const onRequest = ({ url, redirect }, next) => {
if(url.pathname.endsWith('.html')) {
return redirect(url.pathname.replace('.html', ''));
}
next();
}
export const onRequest = ({ url }, next) => {
if(url.pathname === '/testing' && import.meta.env.MODE !== 'development') {
return new Response(null, {
status: 405
})
}
next();
};
import { sequence } from 'astro/middleware';
const auth = async ({ cookies, locals }, next) => {
if(!cookies.has('sid')) {
return new Response(null, {
status: 405 // Not allowed
});
}
let sessionId = cookies.get('sid');
let user = await getUserFromSession(sessionId);
if(!user) {
return new Response(null, {
status: 405 // Not allowed
});
}
locals.user = user;
next();
};
export {
auth as onRequest
}
---
const { user } = Astro.locals;
---
import { auth } from './auth';
const allowed = async({ locals, url }, next) => {
if(url.pathname === '/admin') {
if(locals.user.isAdmin) {
return next();
} else {
return new Response('Not allowed', {
status: 405
})
}
}
next();
};
export const onRequest = sequence(auth, allowed);
export const onRequest = async (context, next) {
let response = await next();
if(response.headers.get('content-type') === 'text/html') {
let html = await response.text();
let minified = await minifyHTML(html);
return new Response(minified, {
status: 200,
headers: response.headers
});
}
return response;
}
@matthewp
return next();
and not next()
? I don't think typescript can detect if next()
is getting called.onRequest()
final? I think it's inconsistent that middleware uses onRequest()
but API routes go withget
, post
, etc (and not onRequest
, onGetRequest
, etc). In that sense, middleware()
or a more generic handle()
seems better.@matthewp
1. Shouldn't it always be `return next();` and not `next()`? I don't think typescript can detect if `next()` is getting called.
Not necessarily. It all depends of what a middleware wants to do.
2. Is the name `onRequest()` final? I think it's inconsistent that middleware uses `onRequest()` but API routes go with`get`, `post`, etc (and not `onRequest`, `onGetRequest`, etc). In that sense, `middleware()` or a more generic `handle()` seems better.
The name is not final. I found the const middleware
name redundant because we already instructed people to have their files under the middleware/
folder/middleware
file. handle
is still generic. What does it handle? What if we wanted to export a new hook from the middleware?
return
statement - it has to return something no?middleware.ts
only have a single purpose, which is to handle middleware? I don't think any other hook should be present there. Maybe it would be more appropriate to change the file name to hooks.ts
and export middleware
if we are planning to add new hooks to it?@pilcrowOnPaper
That's a good point. I would say yes, we should return what's returned by next()
. Although Matthew raised an interesting question: https://github.com/withastro/roadmap/pull/555#issuecomment-1509144891
I don't mind the new suggested names :)
Is there any thoughts on how Astro will expose locals
to adapters? I see that adapter-specific middleware is to be supported in a future release, so I'm not sure if the approach I went for in my fork is the idealized way. It's similar to the old Astro.context
PR from a long time ago https://github.com/withastro/astro/pull/3934.
This allows adapters to pass in a locals
object through the request. Currently, I'm using this on our website / services since there isn't really a nice alternative besides deriving state in Astro itself (in our case it would create duplicated work). https://github.com/brickadia/astro/tree/expose-locals.
Is this in the same direction the RFC wanted to go? I'd like to help polish and upstream these changes, since having just this feature would be a huge win for us! BTW, the feature already reduced a lot of the work I needed to maintain before, so huge thanks for getting this in Astro already!
@wrapperup I think there's a couple of ideas here:
locals
object in the adapter API itself. That way an adapter could append to locals before it ever gets to the rest of the app.Personally I like both (1) and (2) idea and am less sure about (3) as it messes with the ability for the app developer to control ordering.
I like number 2, and that's essentially what we do now on our project. The nice thing is that the change is pretty dang small too, and it let us migrate all of our pages to Astro that required state from our Koa app. I have this work pretty much done in a fork, so I'd like to polish it up so that locals are passed in nicer and make a PR for it.
I like 1 too, but in our case it wouldn't solve the above issue, since other parts of our app don't live in Astro (like our API). But both can exist!
Is it possible to use middleware to add a defaultLang
for i18n (internationalization)? Currently the options for supporting multiple languages with a defaultLang
are:
Avoid it and redirect /about
to /en/about
instead https://docs.astro.build/en/recipes/i18n/
i18next Astro accomplishes this by copying all sites into new directories for each language thats not your default.
In my case I chose to support english and german, with german being the default. So i18next copies the site files with english translation settings into the en/ directory, while keeping the original files with the german settings where they are.
You might be able to write an NPM script to do this for you. Alternatively, if you are on a Posix system, you could do some shenanigans with symlinks. Though they tend to screw up git sometimes.
you could ignore default language posts, and create a separate [...slug].astro file without the language tag and do the same thing but just for the default language
rename the page to src/pages/[...slug.astro] and build the slug using ${lang}/blog/${postSlug}, omitting lang for default language posts
A built-in middleware for properly handling defaultLang
for i18n would be great. That way, all my language content could live in their respective folders, like en
, es
, and de
. But, for a default language, say English, I could return the contents of /en/about
when navigating to /about
, while redirecting /en/about
to /about
.
A super hacky workaround I have is to build the plain output I have for all lang routes, /en/about
. Then, run preview for that first build. Next, run build again with a different out dir, but this time, use middleware to intercept /about
, fetch /en/about
from the preview server, return that response, and generate html redirects for /en/about
to /about
. Finally, it may be necessary to merge some of the build outputs.
Is it possible to be able to make fetch requests or generate different page responses in the middleware function? Using fetch for http://localhost:3000/en/about
and returning the response in the middleware actually works in the dev server, but not during build, since the server isn’t running.
I think i18n should be a first-class concern for Astro.
@jlarmstrongiv
That is one of the use cases you can solve with middleware.
Although, we are not there yet to provide a "native" solution to these problems using integrations. I would suggest shipping your solution for the time being!
@ematipico
That is one of the use cases you can solve with middleware.
Is there a way to return a different page with the current middleware API? The request.url
is readonly, and path parameters cannot be optional/nullable. As far as I can tell, middleware can only return the page content of the requested page with const response = await next();
, not any page I want. Please correct me if I’m wrong—it would make this a lot easier.
Although, we are not there yet to provide a "native" solution to these problems using integrations
Do you have specific APIs you would recommend I look at? A push in the right direction would help a lot.
@jlarmstrongiv Can't you use a redirect? The first argument of the middleware is APIContext
, which has a redirect
function.
@ematipico not really, no
First, redirects don’t work when using SSG https://github.com/withastro/roadmap/issues/466#issuecomment-1544933273 but that can be worked around by returning an html meta redirect page
Second, for the defaultLang
support—going to /about
should display the about page in English, not redirect to /en/about
, while /en/about
should redirect to /about
. What I would like to do is define my pages regularly with a language parameters as described here, but have middleware handle the redirects for the defaultLang
The workarounds are:
defaultLang
support/about
and /[lang]/about
[...path]
to handle every route. None of these are as good as supporting fetching page data from different routes in middleware so I can handle the defaultLang
redirects. Totally open to another solution too, but I think the middleware route would be easiest.
Closing as this is completed and in stable.
How can i do rewrite like in nextjs Middleware
Does anyone have an example of how to use locals in a SSG page?
Body
Astro.context
RFC https://github.com/withastro/roadmap/pull/230Summary
Introduce a middleware to Astro, where you can define code that runs on every request. This API should work regardless of the rendering mode (SSG or SSR) or adapter used. Also introduces a simple way to share request-specific data between proposed middleware, API routes, and
.astro
routes.Background & Motivation
Middleware has been one of the most heavily requested feature in Astro. It's useful for handling common tasks like auth guards and setting cache headers. For me, it would make handling authentication much easier.
This proposal is heavily inspired by SvelteKit's
handle
hooks;Goals
Non-Goals
Example
A quick prototype
Middlewares can be defined in
src/middleware.ts
by exportingmiddleware
(array):A simple middleware looks like so:
context
is the same one provided to API route handlers. Most of Astro's request handling process will be behindresolve()
, and the response object returned frommiddleware
will be sent to the user's browser.Multiple middleware
sequence
can be imported to run multiple middlewares in sequence.The log result for this example is:
locals
This proposal also adds
locals
property toAPIContext
andAsroGlobal
. Thislocals
object will be forwarded across the request handling process, allowing for data to be shared between middlewares, API routes, and.astro
pages. This is useful for storing request specific data, such as user data, across the rendering step.The value type of
locals
can be anything as it won't be JSON-stringified:locals
can be typed insidesrc/env.d.ts
(I think it's possible?):SSG mode
The middleware function will run on each route's pre-rendering step, as if it were handling a normal SSR request.