bug: JSONParsed type not correctly working for generic type

[MathurAditya724]

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 and v4.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.ts

The issue is with the generic type (TData in this case), if we remove the generic type everything works like normal.

ok: <TData>(
  data: TData,
  meta?: {
    count?: number;
  }
) => Response &
  TypedResponse<
    { data: TData; meta?: { count?: number } },
    StatusCode,
    "json"
  >;

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

[yusukebe]

@MathurAditya724 Thank you for the issue.

@m-shaka Can you take a look at it?

[m-shaka]

Ok!

[yusukebe]

@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.

[MathurAditya724]

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.

[yusukebe]

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.

Handling TData as a JSON type is hard

It'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.

Should we support generics as a valid JSON type?

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?

[yusukebe]

Hmm. Anyway, I also think supporting it would be great.

[yusukebe]

@NamesMT Hey. Do you have any thoughts?

[m-shaka]

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

[NamesMT]

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.

[NamesMT]

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.

[MathurAditya724]

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

[MathurAditya724]

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 }>

[m-shaka]

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.
#3135 (comment)
I think I could solve it by giving a way to define a generic function type without TypedResponse, which may feel a bit tricky.
#3135 (comment)

Second, I guess @NamesMT pointed out the exact cause of this regression! Thank you.
#3135 (comment)

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?

[yusukebe]

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!

[m-shaka]

@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

[MathurAditya724]

Oh God! I need to update my libs now 🥲. Thanks for the heads-up