honojs / hono

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

Extend type safe RPC with zod validator (to numbers at least) #3495

Open BryanAbate opened 1 week ago

BryanAbate commented 1 week ago

What is the feature you are proposing?

Following the recent fix of https://github.com/honojs/hono/issues/2481 that allows type safety for enum in the RPC client with zod validator. What do you think about extending this to the actual type of the validator or at least for number? I know that it's going to be cast to string anyway but it would help with number parameters which are often used, like page and page size.

Here is a more concrete example: This code:

zValidator(
  'query',
  z.object({
    page: z.number(),
    pageSize: z.number(),
  }),
)

is currently inferred as:

query: {
  page: string | string[];
  pageSize: string | string[];
}

but it would be nice to have it inferred as:

query: {
  page: number;
  pageSize: number;
}
yusukebe commented 6 days ago

Hi @BryanAbate

It's useful that it infers as number, but the following code will throw 400.

const app = new Hono()

const routes = app.get(
  '/',
  zValidator(
    'query',
    z.object({
      page: z.number()
    })
  ),
  (c) => c.text('Hi')
)

const client = testClient(routes)
const res = await client.index.$get({
  query: {
    page: 123
  }
})

console.log(res.status) // 400

This means the number 123 on the client side will be converted to a string, and Zod will invalidate it because it expects number. The actual value of query will not be number. So, it should not be number, I think. But in this case, if you set page as z.number(), there will be a type mismatch in another case. So, using coerce is good.

const routes = app.get(
  '/',
  zValidator(
    'query',
    z.object({
      page: z.coerce.number()
    })
  ),
  (c) => c.text('Hi')
)

This is a bit of a difficult problem. Either way, that should not be inferred as a number because the query can't accept number.

BryanAbate commented 6 days ago

Hi.

Thank you very much for your answer.

You are completely right about coerce, I actually use it everywhere in my app but was too focused on the typing aspect when writing the issue and forgot about it, sorry about that.

Either way, that should not be inferred as a number because the query can't accept number.

I agree that it makes sense to be a string because it's going to be converted to string ultimately but at the same time passing any string that is not a valid number will throw a 400. And in my opinion this is where we would benefit from a tradeoff between absolute correctness and developer experience. Thinking about it, the type should be number | string anyway because there are still some valid strings that can represent numbers like 1e3.

If you still think this should stay as is, we can close this issue, it was more a nice to have feature :)