honojs / middleware

monorepo for Hono third-party middleware/helpers/wrappers
https://hono.dev
468 stars 167 forks source link

[zod-openapi] The recommended way to apply middleware loses Context type info #637

Open wfjsw opened 4 months ago

wfjsw commented 4 months ago

I want to use a middleware to somehow change the type of the context:

const app = new Hono<HonoEnv>();
type AuthenticationVariable = {
  user?: string;
};

export type WithAuthenticatedRequest = {
  Variables: AuthenticationVariable;
};

export const requireAuth = createMiddleware<HonoEnv & WithAuthenticatedRequest>(...)
// The ctx here transforms from Context<HonoEnv> to Context<HonoEnv & WithAuthenticatedRequest>
app.get('/protected', requireAuth, async (ctx) => {});

However, this is currently impossible to do with the use approach:

const app = new OpenAPIHono<HonoEnv>();

app.use(route.getRoutingPath(), requireAuth);

// The ctx here only has Context<HonoEnv>
app.openapi(route, async (ctx) => {});

And I cannot find apparent workaround. If I manually type the ctx, it will lose input/output info.

askorupskyy commented 4 months ago

Yes, I am encountering this as well. I also do not like the approach mentioned in the docs, where you have to specify the middleware in createRoute call. It kind of breaks the separation of concerns for me.

@yusukebe would it be possible to have something like app.use(middleware).openapi(route, async (c) => yourcode) ?

askorupskyy commented 4 months ago

Hey @wfjsw I think I figured it out.

What you need to do is create a router with env included, like:

const authRouter = new OpenAPIHono<{ Variables: ContextVariables }>();

Which on one hand sucks, because you'll need to double-check if the variable in the context exists (without openapi, if the middleware is used, then ts would 100% know it's there)

However, with this you'll now have better type-completion on the front-end so you'll be able to catch your errors better.

You need to define your error message as such in the definition:

401: {
  description: 'Unauhorized',
  content: {
    'application/json': { schema: z.object({ message: z.string() }) },
  },
},

And then return in the router:

if (!c.var.user?.id) {
  return c.json({ message: 'Unauthorized' }, 401);
}

Which is not ideal, but still gives you auto-completion pretty much everywhere. I think ideally Hono should have type-safe middleware implemented.

yusukebe commented 4 months ago

Hi @wfjsw

However, this is currently impossible to do with the use approach: And I cannot find apparent workaround. If I manually type the ctx, it will lose input/output info.

I can understand what you want to do well. But unfortunately, you can only do that even if it is ZodOpenAPI or Hono.

const app = new Hono()

app.use(requireAuth)
app.get('/foo', (c) => {
  c.get('') // NG: Can't infer `user`
})

This is because app can't keep the type of requireAuth. And if you are using Hono it can infer with that syntax:

const app = new Hono()

app.use(requireAuth).get('/foo', (c) => {
  c.get('user') // OK: Can infer user
})

However, you can't write the following with ZodOpenAPI.

const app = new OpenAPIHono();

app.use(route.getRoutingPath(), requireAuth).openapi // NG

This is because the app is always Hono instead of OpenAPIHono. Unfortunately, this is a TypeScript limitation.

So, the only way we can do this is to pass it as generics for OpenAPIHono.

const app = new OpenAPIHono<HonoEnv & WithAuthenticatedRequest>()
sventi555 commented 3 months ago

Facing the same issue :(

Blankeos commented 3 weeks ago

So, the only way we can do this is to pass it as generics for OpenAPIHono.

const app = new OpenAPIHono<HonoEnv & WithAuthenticatedRequest>()

I kinda get this for keeping the types, but how do you apply the middleware? Is it:

const route = createRoute({
  middlewares: [withAuthenticatedRequest]
})

const endpoint = new OpenApiHono().openapi(route, (c) => {
   // ...
})
askorupskyy commented 3 weeks ago

@Blankeos yeah that's how I do it. The other way is via use() but it applies to the route (all GET, POST, PATCH, etc) and not specific endpoint

Blankeos commented 3 weeks ago

Ahh true. Thanks @askorupskyy

Yeah I settled on a trpc-like procedure instead:

export const h = {
   get public() {
      return new OpenApiHono()
   },
   get private() {
      const app = new OpenApiHono<MyMiddlewareBindings>();

      app.use(myMiddleware);

      return app;
   }
} 

const route = createRoute({
    // ...
});

const endpoint = h.private.openapi(route, async (c) => {
   // ...
})

The typesafety works so far, but have not tested it yet.


Update: Okay I have tested, and it sucks to use app.use() here before defining the route.

Mainly because doing this:

const myController = new OpenApiHono()
   .route("/", myEndPoint1) // the middleware used by this, will go below (e.g. private)
   .route("/", myEndPoint2) // even if this is public, it will use the private middleware.
maou-shonen commented 2 weeks ago

This is my rough workaround for now, I using this approach to solve the problem temporarily.

// OpenAPIHonoFactory.ts
import { createRoute, OpenAPIHono, z } from "@hono/zod-openapi"
import type { Env as HonoEnv, MiddlewareHandler } from "hono"
import type { IsAny } from "hono/utils/types"

type OpenApiRoute = ReturnType<typeof createRoute>

export class OpenAPIHonoFactory<E extends HonoEnv = HonoEnv> {
  constructor(public route: OpenApiRoute) {}

  middlewares: MiddlewareHandler[] = []

  createApp() {
    const app = new OpenAPIHono<E>()

    for (const middleware of this.middlewares) {
      app.use(this.route.path, middleware)
    }

    return app
  }

  use<ExtraEnv extends HonoEnv>(
    middleware: MiddlewareHandler<ExtraEnv>,
  ): IsAny<ExtraEnv> extends true
    ? OpenAPIHonoFactory<E>
    : OpenAPIHonoFactory<E & ExtraEnv> {
    this.middlewares.push(middleware)
    return this as any
  }
}

Here’s how to use it:

import { createRoute } from "@hono/zod-openapi"
import { OpenAPIHonoFactory } from "./OpenAPIHonoFactory"
import type { MiddlewareHandler } from "hono"

const route = createRoute({
  path: "/",
  method: "get",
  responses: {
    200: {
      description: "OK",
    },
  },
})

const factory = new OpenAPIHonoFactory(route)

const myMiddleware1: MiddlewareHandler<{ Variables: { foo: string } }> = (
  c,
  next,
) => {
  c.set("foo", "bar")
  return next()
}

const myMiddleware2: MiddlewareHandler<{
  Variables: { hi: (name: string) => void }
}> = (c, next) => {
  c.set("hi", (name: string) => console.log(`hello ${name}`))
  return next()
}

const app = factory
  .use(myMiddleware1)
  .use(myMiddleware2)
  .createApp()
  .openapi(route, (c) => {
    c.var.hi("world")
    return c.text(c.var.foo)
  })

export default app

image

I might create another package for this purpose. Take a look at this. https://github.com/honojs/middleware/issues/758#issuecomment-2387632283

oberbeck commented 1 week ago

The initial issue is now solved with @hono/zod-openapi@0.17.0, right?

https://github.com/honojs/middleware/issues/715#issuecomment-2461800286

Here my suggested edit to the docs to make this more clear: https://github.com/honojs/middleware/pull/812/files