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

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

Open
BryanAbate opened this issue Oct 7, 2024 · 2 comments
Open

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

BryanAbate opened this issue Oct 7, 2024 · 2 comments
Labels
enhancement New feature or request.

Comments

@BryanAbate
Copy link

What is the feature you are proposing?

Following the recent fix of #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;
}
@BryanAbate BryanAbate added the enhancement New feature or request. label Oct 7, 2024
@yusukebe
Copy link
Member

yusukebe commented Oct 7, 2024

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
Copy link
Author

BryanAbate commented Oct 8, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request.
Projects
None yet
Development

No branches or pull requests

2 participants