Closed MathurAditya724 closed 1 month ago
@MathurAditya724 Thank you for the issue.
@m-shaka Can you take a look at it?
Ok!
@m-shaka Thank you for the PR!
@MathurAditya724 I'm sure regression occurred, but I don't know if we can say it's a real bug since it is not expected for such complex usage. I'll look into it too.
Thank you, @yusukebe and @m-shaka, for taking the time to address this issue.
This is particularly relevant for those of us building custom middleware wrappers and libraries around Hono. Utilizing generic types is quite common, and I’ve encountered similar challenges while working on Honohub during the upgrade to the newer version.
You are correct that this seems more like a feature/improvement rather than a bug.
Hi @MathurAditya724 cc: @m-shaka
I've struggled to resolve this issue today, but I can't find a good way to remove the error. And #3138 by @m-shaka may not be the correct solution.
TData
as a JSON type is hardIt's very difficult or impossible to identify the TData
as a valid JSON type. The following throws an error.
type T = <TData>(data: TData) => JSONRespondReturn<{ data: TData }, StatusCode>
type T2 = <TData>(data: TData) => TypedResponse<{ data: TData }, StatusCode, 'json'> & Response
type verify = Expect<Equal<T, T2>> // Type error!
I'm unsure if we should compare the types as the same completely, and your error may be removed with the other way. But I've found handling generics - TData
in TypedResponse
and JSONRespondReturn
is very hard.
In my opinion, we don't have to struggle to remove the error because we don't have to support generics types. With PR #3074, thanks to @m-shaka, c.json()
validates the value whether it is valid JSON or not strictly. This improvement is super good for users who use Hono straightway. Honestly, I don't expect the complex usage for TypedResponse
. I think this should not be used directly.
Or do you have any good way to remove the error while keeping JSON types validated correctly?
Hmm. Anyway, I also think supporting it would be great.
@NamesMT Hey. Do you have any thoughts?
This is a little bit tricky, but you can get JSONRespondReturn
without hono exposing it
import { Context } from "hono";
import { StatusCode } from "hono/utils/http-status";
const contextVariables = {};
function okTypeHelper(c: Context) {
return <TData>(data: TData, meta?: { count?: number }) => c.json( { data, meta } );
}
type Ok = ReturnType<typeof okTypeHelper>
export type PublicContextVariables = typeof contextVariables & {
ok: Ok
};
Here is the detail of Ok
type
It seems like I got a strict response type on #3138
After some hours of testing, I've come with a conclusion: IMHO, this seems like a TypeScript behavior/bug/limitation.
If you get the typeof data
inside the c.set()
scope, you get a TData
that is virtually not yet typed to anything,
No type util/checking works on it, including Simplify
, IsAny
and extends unknown
check.
So it is causing issues, which including the IsInvalid
check inside of JSONParsed
discarding the data
key, hence causing the overload mismatching in this issue (#3135).
This is my minimal re-produce example, note that I have assigned a default value for TData
to demonstrated the "bug".
export type PublicContextVariables = {
ok: <TData extends number = 3>(data: TData) => TData
};
new Hono<{ Variables: PublicContextVariables }>()
.use((ctx, next) => {
ctx.set('ok', (data) => {
// data is hinted as `TData extends number = 3`
// DataType is hinted as `TData`
type DataType = typeof data
type CheckIsAny = Expect<Equal<IsAny<DataType>, true>> // Type 'boolean' is not assignable to type 'true'
type CheckIsUnknown = Expect<DataType extends unknown ? true : false> // Type 'boolean' is not assignable to type 'true'
return data
})
return next()
})
I think this is because, as you can see, the signature of ok
is: <TData extends number = 3>(data: TData) => TData
,
Which means it is a function that gets it's type from the variable passed into the data
argument,
But in the case of .set()
, we're "creating" that function, which logically means in this context, no type been assigned to TData
yet.
The weird/buggy part is that TypeScript sets the TData
in this scope to "nothing", not any
, not unknown
, simply "nothing" and it could not be used with any sort of type checking, even if we set an extends
constraint and a default value.
If my previous comment is correct, this is not really an issue with #3074,
It is true that #3074 improvement to hono
's type system introduced this regression, but it is only because of a weird behavior/limitation/bug in TypeScript.
IMO #3074 should be kept as it is a good improvement overall for hono
, and we should try to find some other way to implement the wanted functionality in middleware developer's side,
Because it is overall not a good idea to implement on a flow that includes a weird behavior from TypeScript.
I agree with @NamesMT, that we should keep the functionality introduced in #3074.
For the generics, IMHO it's not that TypeScript sets the TData
to "nothing". It's more like the value of TData
can be anything (eg, any
, number
, Record
) across different parts of a codebase, depending upon the requirement. Which is why figuring out the end value becomes difficult.
It would be better to have a hybrid approach to resolve this issue.
So to put this into perspective, this is what was happening in hono@4.4.11
-
Both static and generic types can return functions.
Now in the current version hono@4.5.1
-
Static types are improved but generic types are having issues.
So I think the best approach will be to parse the static types and leave the generic ones that I believe @m-shaka has already achieved in #3138.
But I do find it weird that we cannot extend the generic type - <TData extends string>(data: TData) => TypedResponse<{ data: TData }>
as pointed out by @NamesMT
On second thought, if we can figure out why we are unable to extend the generic type we can solve this. Then the dev can just extend it to be of type JSONValue
.
<TData extends JSONValue>(data: TData) => TypedResponse<{ data: TData }>
Let me summarise the discussion so far.
First, the concern @yusukebe described is about the difference between TypedResponse
and JSONRespondReturn
when they are generic type parameters and complex use of TypedResonse
.
https://github.com/honojs/hono/issues/3135#issuecomment-2241056347
I think I could solve it by giving a way to define a generic function type without TypedResponse
, which may feel a bit tricky.
https://github.com/honojs/hono/issues/3135#issuecomment-2241176298
Second, I guess @NamesMT pointed out the exact cause of this regression! Thank you. https://github.com/honojs/hono/issues/3135#issuecomment-2241197635
I think I could solve that on #3138 to some extent without giving up the strict JSON typing as @MathurAditya724 mentioned, and also I found a way to use extends
constraint like this (I edited the first repro)
import { Context, Hono } from "hono";
import { serve } from "@hono/node-server";
const contextVariables = {};
const ERROR_RESPONSES = {
NOT_FOUND: { message: "Not found", code: 404 },
INTERNAL: { message: "Not found", code: 500 },
} as const;
type ErrorName = keyof typeof ERROR_RESPONSES;
function okHelper(ctx: Context) {
return <TData>(data: TData, meta?: { count?: number }) => ctx.json( { data, meta } );
}
function failHelper(ctx: Context) {
return (errorName: ErrorName) =>
ctx.json({ error: ERROR_RESPONSES[errorName] }, ERROR_RESPONSES[errorName].code);
}
function okExtendedHelper(ctx: Context) {
return <TData extends number>(data: TData, meta?: { count?: number }) => ctx.json( { data, meta } );
}
type Ok = ReturnType<typeof okHelper>
type Fail = ReturnType<typeof failHelper>
type OkExtended = ReturnType<typeof okExtendedHelper>
export type PublicContextVariables = typeof contextVariables & {
ok: Ok
fail: Fail
okExtended: OkExtended
};
const app = new Hono<{ Variables: PublicContextVariables }>();
const routes = app
.use((ctx, next) => {
Object.entries(contextVariables).forEach(([name, value]) => {
ctx.set(name as never, value as never);
});
// works on main
ctx.set("ok", okHelper(ctx));
ctx.set("fail", failHelper(ctx));
ctx.set("okExtended", okExtendedHelper(ctx));
// doesn't work on main, but works on #3138
ctx.set("ok", (data, meta) => ctx.json({ data, meta }));
// doesn't work anywhere for now
// @ts-expect-error
ctx.set("okExtended", (data, meta) => ctx.json({ data, meta }));
return next();
})
.get("/ping", ({ var: { ok, okExtended } }) =>{
return ok(['ping', undefined] as const, { count: 2 })
}
)
.get("/extended", ({ var: { okExtended } }) => {
// @ts-expect-error
okExtended('foo')
return okExtended(1)
})
serve({
fetch: app.fetch,
port: 4000,
});
Does it make sense?
The things that @m-shaka summarised sound good. I think this @MathurAditya724 's issue will be fixed by #3138. I'll merge it into main
. Thank you!
@MathurAditya724 FYI From TypeScript 5.6, you can't use an anonymous function in your use case
ctx.set("ok", (data, meta) => ctx.json({ data, meta }));
See #3310
Oh God! I need to update my libs now 🥲. Thanks for the heads-up
What version of Hono are you using?
4.4.13
What runtime/platform is your app running on?
Nodejs
What steps can reproduce the bug?
This is happening in the last 2 releases,
v4.4.12
andv4.4.13
. It seems like the issue is occurring because of this #3074 PR. A community member has created a sample repo - https://github.com/avetisk/dummy-hono-context-helper-type/blob/main/serve.tsThe issue is with the generic type (
TData
in this case), if we remove the generic type everything works like normal.I believe it is occurring because of this new addition in the
JSONParsed
type -Even if we
extend
the type, it still gives the error.What is the expected behavior?
No response
What do you see instead?
No response
Additional information
No response