honojs / hono

Web framework built on Web Standards
https://hono.dev
MIT License
18.48k stars 520 forks source link

Infer correct types when creating `MiddlewareHandler` dynamically #3202

Open yusukebe opened 1 month ago

yusukebe commented 1 month ago

What is the feature you are proposing?

If you create a middleware from a function like a factory method, it can't infer types correctly in the handler:

const app = new Hono<{
  Variables: {
    foo: string
  }
}>()

type CreateMiddleware = <E extends Env, P extends string>(
  handler: (c: Context<E, P>) => any
) => MiddlewareHandler<E, P>

const createMiddleware: CreateMiddleware = () => {
  return async () => {}
}

app.get('/abc/:id', async (c) => {
  const foo = c.get('foo') // foo is string
  const id = c.req.param('id') // id is inferred correctly.
  type T = typeof id // string
})

app.get(
  '/abc/:id',
  createMiddleware(async (c) => {
    const foo = c.get('foo') // foo is never
    const id = c.req.param('id') // id is not inferred correctly.
    type T = typeof id // string | undefined
  })
)

This is a TypeScript limitation, but we have to find a way to resolve it.

Related to #3198

yusukebe commented 1 month ago

Hi @NamesMT @m-shaka and others

It's hard for me. Do you have any ideas on how to solve this?

mvares commented 1 month ago

Solution:

const createMiddleware: CreateMiddleware = <E extends Env, P extends string>(
  handler: (c: Context<E, P>) => any
) => {
  return async (c: Context<E, P>, next: () => Promise<void>) => {
    await handler(c);
    await next();
  };
};
yusukebe commented 1 month ago

Hi @mvares

It does not solve the problem:

CleanShot 2024-07-28 at 22 28 05@2x

Maybe we should change the CreateMiddleware.

yusukebe commented 1 month ago

This is a TypeScript Playground.

m-shaka commented 1 month ago

Hmmm, I think I can say what you want is like this

type Handler<P extends string> = (p: P) => void

declare function f<P extends string>(p: P, handler: Handler<P>): void;

declare function create<P extends string>(fun: (p: P) => void): Handler<P>

f('foo', create(p => {
  type P = typeof p
}))

The last P is correctly inferred as 'foo'

playground

m-shaka commented 1 month ago

I found a clue about path parameter. https://github.com/honojs/hono/compare/main...m-shaka:hono:exp-dynamic-middleware?w=1

import { Hono, Env, Context, MiddlewareHandler, Handler, Input } from 'hono'

const app = new Hono<{
  Variables: {
    foo: string
  }
}>().basePath('/api/:v')

type CreateMiddleware = <E extends Env, P extends string>(
  handler: (c: Context<E, P>) => any
) => MiddlewareHandler<E, P>

const createMiddleware: CreateMiddleware = () => {
  return async () => {}
}

app.get(
  createMiddleware(async (c) => {
    const v = c.req.param('v') // inferred!!!
    type T = typeof v // string
  })
)

app.get(
  '/abc/:id',
  createMiddleware(async (c) => {
    const foo = c.get('foo') // foo is never
    const id = c.req.param('id') // inferred!!!
    type T = typeof id // string
  })
)
m-shaka commented 1 month ago

Let me share my progress.

Also regarding Env, I found a way to enhance typing. https://github.com/honojs/hono/commit/003ad218332415ab26e0c029aface550c644cc3f?w=1

const anotherMW: MiddlewareHandler<{ Variables: { bar: string }}> = async (c, next) => {
  next()
}

app.get(
  createMiddleware(async (c) => {
    const foo: string = c.get('foo') // foo is string
    const v: string = c.req.param('v')
    type T = typeof v // string
  })
)

app.get(
  anotherMW,
  createMiddleware(async (c) => {
    const foo: string = c.get('foo') // foo is string
    const bar: string = c.get('bar') // bar is string
    const v: string = c.req.param('v')
    type T = typeof v // string
  })
)
app.get(
  anotherMW,
  async (c) => {
    const foo: string = c.get('foo') // foo is string
    const bar: string = c.get('bar') // bar is string
    const v = c.req.param('v')
    type T = typeof v // string
  },
)

However, changes here is based on that you use createMiddleware as the last argument of get. If you want to put it elsewhere, a combinatorial explosion of type overload may happen

yusukebe commented 1 month ago

I'm working on it. But it is too difficult or impossible to enable the two patterns at once:

// A
import type { Context, Env, MiddlewareHandler } from 'hono'
import { Hono } from 'hono'

const app = new Hono<{
  Variables: {
    foo: string
  }
}>()

type CreateMiddleware = <E extends Env, P extends string>(
  handler: (c: Context<E, P>) => any
) => MiddlewareHandler<E, P>

declare const createMiddleware: CreateMiddleware

app.get(
  '/abc/:id',
  createMiddleware(async (c) => {
    const foo = c.get('foo') // foo should be string
    const id = c.req.param('id') // id should be string
  })
)
// B
import type { MiddlewareHandler } from 'hono'
import { Hono } from 'hono'

const app = new Hono<{
  Variables: {
    foo: string
  }
}>()

const anotherMW: MiddlewareHandler<{ Variables: { bar: string } }> = async (c, next) => {
  next()
}

app.get('/abc/:id', anotherMW, (c) => {
  const foo = c.get('foo') // foo should be string
  const bar = c.get('bar') // bar should be string
  const id = c.req.param('id') // id should be string
  return c.json(0)
})

Currently, A is not working, but B is working. Hmmmm.

@m-shaka Do you have any idea about this?

yusukebe commented 1 week ago

Ending the fight with the Types

Let's end this fight about the problem of this Types matter. It's not only this issue. We have the same problem in the following issues:

We have to solve them.

What's the problem?

So, what's the problem? As mentioned above, the Context in a function in Middleware is not inferred correctly. It's understandable; you can image Basic Auth Middleware.

First, I passed MyEnv to the generics of new Hono(). The c.get('foo') will be inferred as string correctly:


type MyEnv = {
  Variables: {
    foo: string
  }
}

const app = new Hono<MyEnv>()

app.get('*', (c) => {
  const foo = c.get('foo') // OK: foo is string
  return c.text('Hi')
})

Does the function inside of Basic Auth Middleware work well? The middleware can have a verfiyUser function option. You can access the Context object in the function. But c.get('foo') is not inferred correctly:

app.get(
  '*',
  basicAuth({
    username: 'user',
    password: 'pass',
    verifyUser: (user, pass, c) => {
      const foo = c.get('foo') // NG: foo is `any`
      return true
    },
  })
)

This problem is for more than just Basic Auth Middleware. Zod Validator also has the same problem and other middleware, too.

Give up infer perfectly

Ideally, c.get('foo') will be inferred as string without the user adding code. But after our investigation, we found it's impossible! You can see how we dealt with this matter in the comments on this issue.

It's time to give up.

Passing Env as generics

I found the solution the users have to do one thing, but it's simple enough to implement. Passing Env as generics to creating middleware function:

app.get(
  '/abc/:id',
  createMiddleware<MyEnv>(async (c) => {
    const foo = c.get('foo') // foo is string
  })
)

Currently, it's not implemented, but you can write like the following with Basic Auth Middleware:

app.get(
  '*',
  basicAuth<MyEnv>({
    verifyUser: (user, pass, c) => {
      const foo = c.get('foo') // OK: foo is string
      return true
    },
  })
)

Internal implementation

To enable it, we can add the changes for Basic Auth like the following:

diff --git a/src/middleware/basic-auth/index.ts b/src/middleware/basic-auth/index.ts
index 3c1cad5e..15c8be54 100644
--- a/src/middleware/basic-auth/index.ts
+++ b/src/middleware/basic-auth/index.ts
@@ -6,7 +6,7 @@
 import type { Context } from '../../context'
 import { HTTPException } from '../../http-exception'
 import type { HonoRequest } from '../../request'
-import type { MiddlewareHandler } from '../../types'
+import type { Env, MiddlewareHandler } from '../../types'
 import { timingSafeEqual } from '../../utils/buffer'
 import { decodeBase64 } from '../../utils/encode'

@@ -32,7 +32,7 @@ const auth = (req: HonoRequest) => {
   return { username: userPass[1], password: userPass[2] }
 }

-type BasicAuthOptions =
+type BasicAuthOptions<E extends Env = Env> =
   | {
       username: string
       password: string
@@ -40,7 +40,7 @@ type BasicAuthOptions =
       hashFunction?: Function
     }
   | {
-      verifyUser: (username: string, password: string, c: Context) => boolean | Promise<boolean>
+      verifyUser: (username: string, password: string, c: Context<E>) => boolean | Promise<boolean>
       realm?: string
       hashFunction?: Function
     }
@@ -76,10 +76,10 @@ type BasicAuthOptions =
  * })
  * ```
  */
-export const basicAuth = (
-  options: BasicAuthOptions,
+export const basicAuth = <E extends Env = Env>(
+  options: BasicAuthOptions<E>,
   ...users: { username: string; password: string }[]
-): MiddlewareHandler => {
+): MiddlewareHandler<E> => {
   const usernamePasswordInOptions = 'username' in options && 'password' in options
   const verifyUserInOptions = 'verifyUser' in options

CreateMiddlewareHandler

With summarization, the type CreateMiddlewareHandler to create a middleware handler like basicAuth() will be defined in the following:

type CreateMiddlewareHandler = <E extends Env = Env, P extends string = string>(
  handler: (c: Context<E, P>) => unknown
) => MiddlewareHandler<E, P>

So, we have to create the middleware following that type definition.

Next step

Ideally, all middleware follows the CreateMiddlewareHandler type, but I think we don't have to hurry to it. First, we can make popular middleware such as Zod Validator support the type.

Anyway, we can end this fight. Thanks.

yusukebe commented 1 week ago

@m-shaka @NamesMT can you check the above comment? You don't need a lot of time.

m-shaka commented 1 week ago

I'll have a look tomorrow!(JST

m-shaka commented 1 week ago

Adding CreateMiddlewareHandler as a type that all the middlewares should follow would be nice!

As I described in https://github.com/honojs/hono/issues/3202#issuecomment-2264272802, you can solve the issue to some extent. One of the biggest issues there is that you need more type overloads of HandlerInterface. I'm also worried about the impact on type-check performance.

What do you think about that? Do you have any other concerns?

Created a POC PR for better understanding https://github.com/honojs/hono/pull/3376

yusukebe commented 1 week ago

Hi @m-shaka

What do you think about that? Do you have any other concerns?

That seems to be great, though it's magical!

But we have to consider the combination matter that you mentioned:

However, changes here is based on that you use createMiddleware as the last argument of get. If you want to put it elsewhere, a combinatorial explosion of type overload may happen

I think, in most cases, the user does not put createMiddleware in the last argument. The last will be a main handler in many cases:

app.get(
  '/abc/:id',
  // Like Basic Auth or Zod Validator
  createMiddleware(async (c) => {
    const foo = c.get('foo') // foo is never
    const id = c.req.param('id') // id is not inferred correctly.
    type T = typeof id // string | undefined
  }),
  // Main handler
  (c) => {
    return c.json({})
  }
)

With this condition, your code will not work well, right?

MachineMakesNoise commented 1 week ago

Hi!

Adding my two cents here:

This would be really nice if the inference would work better between different middlewares so one could "build up middleware features" with type safety. As an example one middleware would be a "usePostgres" and then next middleware environments would contain "c.var.pgClient". One middleware could be "getUserId" that injects "c.var.userId" and then the last one would be "verifyRoles" which would need both "c.var.pgClient" and "c.var.userId" in order to work and the type checker would complain about if those are missing.

Of course one can already do this but since there is no type safety, the order of middleware could be wrong.

Above examples (especially the CreateMiddleware type which accepts a function) work fine with the last handler but not the intermediate middlewares.

Is this doable somehow currently with some type magic - AND can this be made to work also with the Input (the third one on the MiddlewareHandler) type parameter since it would be nice to get type validation on the likes of c.req.valid("json") too?

m-shaka commented 1 week ago

@yusukebe I see

I think, in most cases, the user does not put createMiddleware in the last argument.

Yeah, you need more type overloads to make that work. There might be a way to avoid the massive combination of overloads but it must be a long long journey to find that, which may require an overhaul of the definition of HandlerInterface. At least, we need to do #3377 first

Passing type parameters manually is the best solution for now!!!

yusukebe commented 1 week ago

@m-shaka

Passing type parameters manually is the best solution for now!!!

Yeah!

But, let's keep finding out to avoid a lot of overloads. Our journey continues.