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.

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

Astro middleware #531

Closed
matthewp opened this issue Mar 22, 2023 · 41 comments
Closed

Astro middleware #531

matthewp opened this issue Mar 22, 2023 · 41 comments
Assignees

Comments

@matthewp
Copy link
Contributor

matthewp commented Mar 22, 2023

Body

Summary

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

  • Provide a way intercept requests and responses, allowing users to set cookies and headers
  • Works both in SSR and SSG mode
  • Allow users to use community-created middlewares (libraries)
    • Make available via integrations API.
  • Provide an API for request-specific data
  • Non-Node runtimes specific APIs. ie. Cloudflare Durable Objects.
    • Add middleware from adapter.

Non-Goals

  • Route specific middleware, middlewares that are run only on specific routes

Example

A quick prototype

Middlewares can be defined in src/middleware.ts by exporting middleware (array):

export cost middleware = []

A simple middleware looks like so:

export const middleware: Middleware = async (context: APIContext, resolve: Resolve) => {
  const session = await getSession(context.request);
  const isProtectedRoute = matchRoute(context.url);
  if (!session && isProtectedRoute) {
    // early response
    return new Response(null, {
      status: 400,
    });
  }
  context.cookies.set("random-cookie", "some-value");
  const response = await resolve(context); // resolve api route or render page
  return response;
};

context is the same one provided to API route handlers. Most of Astro's request handling process will be behind resolve(), and the response object returned from middleware will be sent to the user's browser.

Multiple middleware

sequence can be imported to run multiple middlewares in sequence.

export const middleware: Middleware = sequence(
  async (context, resolve) => {
    console.log("1a");
    const response = await resolve(event);
    console.log("1b");
    return response;
  },
  async (context, resolve) => {
    console.log("2a");
    const response = await resolve(event);
    console.log("2b");
    return response;
  }
);

The log result for this example is:

1a
2a
2b
1b

locals

This proposal also adds locals property to APIContext and AsroGlobal. This locals 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.

export const middleware = async (context: APIContext, resolve: Resolve) => {
  context.session = await getSession(context.request);
  const response = await resolve(context);
  return response;
};
---
// pages/index.astro
const session = Astro.locals.session;
---

The value type of locals can be anything as it won't be JSON-stringified:

---
// pages/index.astro
const session = await Astro.locals.getSession();
---

locals can be typed inside src/env.d.ts (I think it's possible?):

// src/env.d.ts
// at least one of these should be possible
type Locals {}

declare namespace App {
    type Locals = {}
}

SSG mode

The middleware function will run on each route's pre-rendering step, as if it were handling a normal SSR request.

export const middleware = async (context: APIContext, resolve: Resolve) => {
  const response = await resolve(context); // pre-render step here
  return response;
};
@pilcrowOnPaper
Copy link

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?

@ematipico
Copy link
Member

ematipico commented Mar 27, 2023

@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.

  1. 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.

  2. 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."

@pilcrowOnPaper
Copy link

@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
Copy link
Member

@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!

@matthewp
Copy link
Contributor Author

@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

@pilcrowOnPaper
Copy link

@matthewp
Can you share state between 2 runtimes? If not, locals would not work as expected.

@pilcrowOnPaper
Copy link

pilcrowOnPaper commented Mar 27, 2023

@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.

@ematipico
Copy link
Member

ematipico commented Mar 31, 2023

A prototype is available here:

npm i astro@0.0.0-middleware-20230331181015

The prototype mostly follows the RFC, a small deviation:

  • the middleware file will export different functions;
  • the file will export onRequest function and onError function;
  • onError is not used yet in the prototype
  • to load the middlewares
export default defineConfig({
	middlewareOrder: ['auth', 'server']
})
  • the sequence function is not exposed yet

@FredKSchott
Copy link
Member

FredKSchott commented Mar 31, 2023

Probably 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:

@FredKSchott
Copy link
Member

@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!

@FredKSchott
Copy link
Member

@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.

@pilcrowOnPaper
Copy link

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.

@ematipico
Copy link
Member

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.

@matthewp
Copy link
Contributor Author

matthewp commented Apr 3, 2023

@pilcrowOnPaper what is the difference between exporting an array of middleware vs. using sequence?

@pilcrowOnPaper
Copy link

@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;

@matthewp
Copy link
Contributor Author

matthewp commented Apr 3, 2023

@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.

@pilcrowOnPaper
Copy link

@matthewp The expectation (or at least mine's) is that it'll run twice.

@lvindotexe
Copy link

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

@ematipico
Copy link
Member

ematipico commented Apr 4, 2023

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

I opened a PR: withastro/astro#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

@natemoo-re
Copy link
Member

natemoo-re commented Apr 12, 2023

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?

@matthewp
Copy link
Contributor Author

matthewp commented Apr 12, 2023

Here's some feedback of mine from trying out the APIs:

  1. I think you should be able to return a new Response instead of mutating the provided one.
  2. The name 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.
  3. Wouldn't expect that providing the context as an argument be required, and the need to do so makes it harder to do destructing.
  4. The examples should override the value of 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.
  5. I think we need some use-case based documentation around the various things you might want to do in middleware such as:
    • Add a value to the locals object.
    • Inspect the cookies and if a cookie is missing, return a new Response 405 status.
    • Call resolve()/next() and then mutate the response. Such as getting the HTML and changing it.

@pilcrowOnPaper
Copy link

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

@ematipico
Copy link
Member

@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.

@pilcrowOnPaper
Copy link

Ah, gotcha. I was kinda confused since I wrongly assumed setting context.locals to writable: false would set content.locals as readonly object.

@matthewp
Copy link
Contributor Author

@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.

@matthewp
Copy link
Contributor Author

matthewp commented Apr 14, 2023

Some use-cases and how I would think to implement them:

Redirect config

export const onRequest = ({ url, redirect }, next) => {
  if(url.pathname.endsWith('.html')) {
    return redirect(url.pathname.replace('.html', ''));
  }
  next();
}

Dev-only route

export const onRequest = ({ url }, next) => {
  if(url.pathname === '/testing' && import.meta.env.MODE !== 'development') {
    return new Response(null, {
      status: 405
    })
  }
  next();
};

Authentication

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;
---

Protected routes

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);

Modifying response HTML

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;
}

@pilcrowOnPaper
Copy link

@matthewp

  1. Shouldn't it always be return next(); and not next()? I don't think typescript can detect if next() is getting called.
  2. Is the name 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.

@ematipico
Copy link
Member

ematipico commented Apr 17, 2023

@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?

@pilcrowOnPaper
Copy link

  1. I was referring to the examples without any return statement - it has to return something no?
  2. Shouldn't 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?

@ematipico
Copy link
Member

@pilcrowOnPaper

That's a good point. I would say yes, we should return what's returned by next(). Although Matthew raised an interesting question: #555 (comment)

I don't mind the new suggested names :)

@wrapperup
Copy link

wrapperup commented May 8, 2023

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 withastro/astro#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!

@matthewp
Copy link
Contributor Author

matthewp commented May 8, 2023

@wrapperup I think there's a couple of ideas here:

  1. An adapter could ship with a middleware. The user would import and use the middleware like any other. Advantage of this approach is that the user has full flexibility to use it how they wish.
  2. We could expose the 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.
  3. Another idea we have tossed around is the idea of an integration being able to "inject" a middleware into the chain without the user needing to import it in their own middleware.ts

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.

@wrapperup
Copy link

wrapperup commented May 8, 2023

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!

@jlarmstrongiv
Copy link

Is it possible to use middleware to add a defaultLang for i18n (internationalization)? Currently the options for supporting multiple languages with a defaultLang are:

Option 1

Avoid it and redirect /about to /en/about instead https://docs.astro.build/en/recipes/i18n/

Option 2

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.

Option 3

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

Option 4

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.

@ematipico
Copy link
Member

@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!

@jlarmstrongiv
Copy link

@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.

@ematipico
Copy link
Member

@jlarmstrongiv Can't you use a redirect? The first argument of the middleware is APIContext, which has a redirect function.

@jlarmstrongiv
Copy link

jlarmstrongiv commented May 21, 2023

@ematipico not really, no

First, redirects don’t work when using SSG #466 (comment) 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:

  1. No defaultLang support
  2. Duplicating data fetching and routing on both /about and /[lang]/about
  3. Avoid file routing entirely and just use [...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.

@matthewp
Copy link
Contributor Author

Closing as this is completed and in stable.

@nabildroid
Copy link

How can i do rewrite like in nextjs Middleware

@danielo515
Copy link

Does anyone have an example of how to use locals in a SSG page?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Implemented
Development

No branches or pull requests

10 participants