fix(type): degradation of generic type handling

[m-shaka]

The author should do the following, if applicable

fix #3135

This works in the use case of the reported issue, I'm not 100% sure why this works though

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code
• Add TSDoc/JSDoc to document the code

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.87%. Comparing base (2d01359) to head (4d9f716).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3138   +/-   ##
=======================================
  Coverage   95.87%   95.87%           
=======================================
  Files         137      137           
  Lines       13514    13514           
  Branches     2300     2285   -15     
=======================================
  Hits        12956    12956           
  Misses        558      558           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[yusukebe]

A quick question. Is adding InvalidJSONValue to JSONObject our ideal definition? I feel it's weird JSONObject has InvalidJSONValue.

[m-shaka]

I see. That depends on the meaning of JSONValue.

For example, the first parameter of Context.json can be an object with a key whose value is InvalidJSONValue type, which has no problem

hono/src/context.ts

Lines 723 to 727 in bb14b1e

	json: JSONRespond = <
	T extends JSONValue | SimplifyDeepArray<unknown> | InvalidJSONValue,
	U extends StatusCode = StatusCode
	>(
	object: T,

I've just given a quick look but JSONValue seems to be used in two ways:

• the type of a candidate value to be converted by JSON.stringify
• the type of a already converted value by JSON.stringify
  • like

    hono/src/types.ts

    Lines 1902 to 1910 in 4d33e1c

    	export type TypedResponse<
    	T = unknown,
    	U extends StatusCode = StatusCode,
    	F extends ResponseFormat = T extends string
    	? 'text'
    	: T extends JSONValue
    	? 'json'
    	: ResponseFormat
    	> = {

I'm not sure but I think it's not so harmful for now. We may need refactoring, though

[yusukebe]

Thank you for the explanation!

How about this? undefined is invalid, but having undefined as the value of a record is valid:

diff --git a/src/context.ts b/src/context.ts
index 8d0aa89e..365161fc 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -180,7 +180,7 @@ type JSONRespondReturn<
       ? JSONValue extends SimplifyDeepArray<T>
         ? never
         : JSONParsed<T>
-      : JSONParsed<T>,
+      : never,
     U,
     'json'
   >
diff --git a/src/utils/types.ts b/src/utils/types.ts
index 3fd17a94..b6cef375 100644
--- a/src/utils/types.ts
+++ b/src/utils/types.ts
@@ -26,7 +26,9 @@ export type IfAnyThenEmptyObject<T> = 0 extends 1 & T ? {} : T

 export type JSONPrimitive = string | boolean | number | null
 export type JSONArray = (JSONPrimitive | JSONObject | JSONArray)[]
-export type JSONObject = { [key: string]: JSONPrimitive | JSONArray | JSONObject | object }
+export type JSONObject = {
+  [key: string]: JSONPrimitive | JSONArray | JSONObject | object | undefined
+}
 export type InvalidJSONValue = undefined | symbol | ((...args: unknown[]) => unknown)

 type InvalidToNull<T> = T extends InvalidJSONValue ? null : T

[m-shaka]

In the meaning of the type of a candidate value, it's ok.
You can call ctx.json({ a: undefined }) and the type of output is {}, which is completely correct because the fact that JSON.stringify drops keys with undefined is the specification.
Same as ctx.json({ a: Symbol.for("") }) or ctx.json({ a: () => {} }).

However, in the meaning of the latter case (used as a type constraint of TypedResponse), it's not correct. It expects types being proceeded by JSONParsed, that never have keys with undefined, symbol, or function type.
But, my guess is that in most cases TypedResponse is used through JSONRespondReturn or like that, and JSONValue is used here only for deciding the response format, and types like { a: undefined } should be treated as 'json' format.
That's why I said it's not so harmful.

(I'm not confident if I understand what you mean. Sorry if not.)

[yusukebe]

@m-shaka

Understood well. You are right that the definition of JSONObject is good for now. Sorry for wasting your time. Thanks!

[m-shaka]

No way! Thank you for your patience!

[yusukebe]

LGTM!