Skip to content
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

Response with null body yields a compile error, which should be allowed for 204 #205

Closed
yudai-nkt opened this issue Oct 20, 2023 · 3 comments · Fixed by #206
Closed

Response with null body yields a compile error, which should be allowed for 204 #205

yudai-nkt opened this issue Oct 20, 2023 · 3 comments · Fixed by #206

Comments

@yudai-nkt
Copy link
Contributor

This is a continuation from honojs/hono#1529 (comment), which I still think is a problem on second thought.

This is the reproduction code I've shown in the issue above

import { OpenAPIHono, createRoute, z } from "@hono/zod-openapi";

const userSchema = z.object({ id: z.string() });
declare const getUserFromDatabase: (
  id: string,
) => z.infer<typeof userSchema> | undefined;

export default new OpenAPIHono()
  .openapi(
    createRoute({
      method: "get",
      path: "/users/{id}",
      responses: {
        200: {
          content: { "application/json": { schema: userSchema } },
          description: "Found",
        },
        404: { description: "Not found" },
      },
    }),
    (c) => {
      const user = getUserFromDatabase(c.req.param("id"));
      return user ? c.jsonT(user) : c.jsonT(null, 404);
    },
  )
  .openapi(
    createRoute({
      method: "put",
      path: "/users/{id}",
      responses: { 204: { description: "Successfull update" } },
    }),
    (c) => {
      // update user info here.
      return c.jsonT(null, 204)
    },
  );

and c.jsonT(null) yields the following compile error.

Argument of type '(c: Context<Env, "/users/:id", {}>) => TypedResponse<{ id: string; }> | TypedResponse<null>' is not assignable to parameter of type 'Handler<Env, "/users/:id", {}, HandlerResponse<{} | { id: string; }>>'.
  Type 'TypedResponse<{ id: string; }> | TypedResponse<null>' is not assignable to type 'HandlerResponse<{} | { id: string; }>'.
    Type 'TypedResponse<null>' is not assignable to type 'HandlerResponse<{} | { id: string; }>'.
      Type 'TypedResponse<null>' is not assignable to type 'TypedResponse<{} | { id: string; }>'.
        Type 'null' is not assignable to type '{} | { id: string; }'.(2345)

Replacing null with {} eliminates the compile error but is problematic in case of 204 status response. You have to use null as a response body if the status is 204:

import { Hono } from "hono";

// TypeError: Response constructor: Invalid response status code 204
await new Hono().get("/", (c) => c.jsonT({}, 204)).request("/");

Returning either null or {} for 4xx/5xx responses might be a personal preference (I'd prefer the former), but I'd be happy if we can return null in Zod OpenAPI Hono because there are no choices for 204.

@yudai-nkt yudai-nkt changed the title Response with null body yields a compile error Response with null body yields a compile error, which should be allowed for 204 Oct 20, 2023
@yusukebe
Copy link
Member

@yudai-nkt

One of the problems is that if you pass null to c.jsonT() and c.json(), it will be interpreted as a string, not real null. So, if the status code is 204, it will trigger the error as you mentioned.

Trace: TypeError: Response constructor: Invalid response status code 204

If you wish to return a 204 status code, you can use c.body(null, 204). However, Hono's validator doesn't support c.body(); it only supports c.jsonT() to infer types correctly.

One idea to address this issue is to make the expected type null if the return method is not c.jsonT().

app.openapi(
  createRoute({
    method: 'get',
    path: '/',
    responses: {
      204: {
        description: 'No Content',
      },
    },
  }),
  (c) => {
    return c.body(null, 204)
  }
)

To enable this, allow the response type to be Response and not just TypedResponse.

@yusukebe
Copy link
Member

@yudai-nkt

This PR #206 might resolve this issue. What do you think it?

@yudai-nkt
Copy link
Contributor Author

I agree with that solution. In my current use case, just checking Response.prototype.status on client side would be enough for responses with null body, so returning Response instead of TypedResponse is a good idea.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants