fix(context): fix queries and headers types

[termosa]

Importance of Implementation

The current routing system lacks enforcement of query parameters and headers, allowing them to be undefined if middleware fails to validate them. This behavior can lead to errors caused by type mismatches. To address this issue and align with the framework's behavior, the undefined type has been introduced. It is currently applicable for calls with keys like Context.req.query('name'), but not for those without keys such as Context.req.query().

Practical Example

Consider the following code snippet, which will not exhibit type issues but is prone to runtime failures:

app.get('/capitalized-name', (c) => {
  // At this point, 'name' is assumed to be a string
  const { name } = c.req.query();
  // This line is at risk of failure if the API is invoked without the 'name' query parameter
  return c.text(name.toUpperCase());
})

With this pull request, an error will be triggered on the last line, indicating that name could potentially be of type undefined.

Author should do the followings, if applicable

• Add tests
• Run tests
• yarn denoify to generate files for Deno

Related

• c.req.query should support generics #1632
  This PR does not add generics as requested in the issue. However, if they are ever implemented, the undefined should be present and serve as the foundation for all generics, unless a router can filter query parameters.

[yusukebe]

Hi @termosa !

Thanks for the PR. You are right, the type does not match the actual data, which should be corrected.

However, this change would be a breaking change and cannot be accepted in a patch release right now. It can be said that the original API was wrong, but it does have an impact on users. We must announce this to users. Please wait a little while as we are considering whether to include it in the major version or the minor version.

[yusukebe]

Or my favorite is to make the following types.

query(): Record<string, string | undefined>
// ...
queries(): Record<string, string[] | undefined>
// ...
header(): Record<string, string | undefined>

This has no difference from the actual value and has less impact on the user. What do you think?

[termosa]

What is this last change about? To remove the access by key?

[termosa]

I was wonder how would you decide to merge it. Because on one side, it is a major fix, as it is changing the interface of the Hono. And on the other side it is a patch, because it is fixing the runtime issue, replacing it with build time error.

[yusukebe]

@termosa

What is this last change about? To remove the access by key?

Super sorry! What I provided and your PR was the same thing!

I was wonder how would you decide to merge it. Because on one side, it is a major fix, as it is changing the interface of the Hono. And on the other side it is a patch, because it is fixing the runtime issue, replacing it with build time error.

You are right. This PR fix is the latter, you say; it should be merged as a patch release.

[yusukebe]

@termosa

Changing yarn.lock is not necessary. Please remove this change.

[termosa]

Looked more into it and saw that:

1. The validator also needs to know about undefined state
2. The Hono Client is affected by this PR
   The Client does a great job of interfering types from the routing and validators and I don't want to break that. Once I'll manage to configure it I will ensure that it behaves properly and reopen the PR.

[yusukebe]

@termosa

1. The validator also needs to know about undefined state

Are you saying that foo is currently a string | string[] but must be a string | string[] | undefined?

app.get(
  '/search',
  validator('query', (value, c) => {
    const foo = value['foo']
    foo // currently `string | string[]`
  }),
  (c) => {//...}
)

[termosa]

Of course.
All of these will be calling the validator you defined and types should represent it.

curl .../search
curl .../search?foo=1
curl .../search?foo=1&foo=2

Because validator() can modify the final result, it may be a good idea to interfere the result type to pass it to the route callback.
Yet, this is the next level task, and I am not sure if I can accomplish it in a mater of short time.

[yusukebe]

@termosa

As for the Validator, what about this change, since the value that passes into the validator could be undefined?

diff --git a/src/validator/validator.test.ts b/src/validator/validator.test.ts
index e5a3747..dcfeb5a 100644
--- a/src/validator/validator.test.ts
+++ b/src/validator/validator.test.ts
@@ -40,7 +40,7 @@ describe('Validator middleware', () => {
       await next()
     },
     validator('query', (value, c) => {
-      type verify = Expect<Equal<Record<string, string | string[]>, typeof value>>
+      type verify = Expect<Equal<Record<string, string | string[] | undefined>, typeof value>>
       if (!value) {
         return c.text('Invalid!', 400)
       }
diff --git a/src/validator/validator.ts b/src/validator/validator.ts
index b60299e..f291c83 100644
--- a/src/validator/validator.ts
+++ b/src/validator/validator.ts
@@ -16,7 +16,7 @@ export type ValidationFunction<
   E extends Env = {},
   P extends string = string
 > = (
-  value: InputType,
+  value: InputType extends Record<infer K, infer V> ? Record<K, V | undefined> : InputType,
   c: Context<E, P>
 ) => OutputType | Response | Promise<OutputType> | Promise<Response>