panteliselef / astro-with-clerk-auth

Community Astro SDK for Clerk with starter `astro-clerk-auth`
https://astro-clerk.elef.codes
MIT License
41 stars 9 forks source link

Use the new `addMiddleware` option #101

Open ematipico opened 3 months ago

ematipico commented 3 months ago

Since version 3.5.0, the integration APIs have a new function called addMiddleware that allows to add a middleware to the sequence, without the user creating their own middleware (or updating it seamlessly if they have one): https://docs.astro.build/en/reference/integrations-reference/#addmiddleware-option

panteliselef commented 3 months ago

Hey @ematipico, I have considered this option in the past, but having a "default" middleware does not feel something very useful as I imagine most people would like to customize it.

Developers would have to customize the middleware by passing their configuration via the integration. Is this a common thing to do in the "astro land" ?

And the if they wanted to use the middleware file, there should be a flag in the integration to avoid using the default one, right ?

ematipico commented 3 months ago

Hey @ematipico, I have considered this option in the past, but having a "default" middleware does not feel something very useful as I imagine most people would like to customize it.

Developers would have to customize the middleware by passing their configuration via the integration. Is this a common thing to do in the "astro land" ?

That's very common. Almost all the adapters (@astrojs/node, @astrojs/vercel, etc.) can accept options when they are used inside the astro.config.mjs file. Starlight, for example, can accept a HUGE object to customise it.

And the if they wanted to use the middleware file, there should be a flag in the integration to avoid using the default one, right ?

No, developers won't need to do anything. Astro will make sure that the middleware function created by your integration is correctly placed (before or after, using pre or post) in the user middleware workflow, using the function sequence.

panteliselef commented 3 months ago

Is there any other benefit from having this inside the integration that I may be missing ?

I believe the API would look something like this

import { defineConfig } from "astro/config";
import node from "@astrojs/node";
import clerk from "astro-clerk-auth";
import { createRouteMatcher } from "astro-clerk-auth/server";

const isProtectedPage = createRouteMatcher(['/user(.*)'])

export default defineConfig({
  integrations: [
    clerk({
      signInUrl: '/sign-in',
      middleware: (auth, context, next) => {
        if (isProtectedPage(context.request) && !auth().userId) {
          return auth().redirectToSignIn();
        }
        return next();
      }
    }),
  ],
  output: "server",
  adapter: node({
    mode: "standalone",
  }),
});

No, developers won't need to do anything. Astro will make sure that the middleware function created by your integration is correctly placed (before or after, using pre or post) in the user middleware workflow, using the function sequence.

  1. Should the pre and post options be set from the developer ? In that case the options passed to clerk() should allow for that.
  2. Can the developer use the sequence in their middleware file to define in what order the clerk middleware will be called ?

I'm trying to understand the pattern here, apologies for the questions.

I have this example in the docs, is this an "antipattern" ? By this I'm referring to call a "lib" middleware along side a developers middelware.

const greeting = defineMiddleware(async (context, next) => {
  console.log("greeting request");
  console.log(context.locals.auth());
  const response = await next();
  console.log("greeting response");
  return response;
});

export const onRequest = sequence(
  clerkMiddleware(),
  greeting,
);

Btw thank you for taking the time and providing me with feedback.

bholmesdev commented 2 days ago

Good questions here! I agree with Ema that addMiddleware() can simplify configuration for the developer. I would only go this route if a majority of users will grab the "out-of-the-box" setup though. If the user customizes the Clerk middleware more times than not, I think a copy-able snippet is okay.

As for your questions above:

  1. I'd recommend the pre option so any user middleware that relies on auth() will be run afterwards. The post option means the Clerk middleware would run after all user middleware, but before their Astro component runs. If there are cases where that is preferred, make it configurable!
  2. The above option would be the only level of customization if you went the addMiddleware route. If you wanted fine-grained control of ordering via sequence, you would probably want to allow a middleware: false so the user can set up Clerk manually.

I would not call your code snippet an anti-pattern by the way! This manual setup is preferred by Lucia auth actually. It all depends on how likely user customization will be.