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
fix(context): fix queries and headers types #2240
base: main
Are you sure you want to change the base?
Conversation
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. |
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? |
What is this last change about? To remove the access by |
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. |
Super sorry! What I provided and your PR was the same thing!
You are right. This PR fix is the latter, you say; it should be merged as a patch release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing yarn.lock
is not necessary. Please remove this change.
3b828cb
to
4762a80
Compare
Looked more into it and saw that:
|
Are you saying that app.get(
'/search',
validator('query', (value, c) => {
const foo = value['foo']
foo // currently `string | string[]`
}),
(c) => {//...}
) |
Of course. curl .../search
curl .../search?foo=1
curl .../search?foo=1&foo=2 Because |
As for the Validator, what about this change, since the value that passes into the validator could be 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> |
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, theundefined
type has been introduced. It is currently applicable for calls with keys likeContext.req.query('name')
, but not for those without keys such asContext.req.query()
.Practical Example
Consider the following code snippet, which will not exhibit type issues but is prone to runtime failures:
With this pull request, an error will be triggered on the last line, indicating that
name
could potentially be of typeundefined
.Author should do the followings, if applicable
yarn denoify
to generate files for DenoRelated
c.req.query
should support generics #1632This 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.