honojs / middleware

monorepo for Hono third-party middleware/helpers/wrappers
https://hono.dev
468 stars 167 forks source link

[zod-openapi] Problem with strict type checking of return values #374

Open WiktorKryzia opened 9 months ago

WiktorKryzia commented 9 months ago

Strict type checking was added in version 0.9.1 in such a way that if response type is declared, the handler of a given route can only return a response of the TypedResponse type, and of the methods available in Hono Context, only the .json() (and deprecated .jsonT()) method returns such a type, which forces that only JSON can be returned from this endpoint.

It's about this piece of code:

// If response type is defined, only TypedResponse is allowed.
  R extends {
    responses: {
      [statusCode: string]: {
        content: {
          [mediaType: string]: ZodMediaTypeObject
        }
      }
    }
  }
    ? HandlerTypedResponse<OutputType<R>>
    : HandlerAllResponse<OutputType<R>>

As a result, if I want to have a route that returns text or e.g. YAML, TS will report an error because the methods Context.text(), Context.body() etc. return the Response type, but TypedResponse is expected.

yusukebe commented 9 months ago

Hi @WiktorKryzia

Indeed, only TypeResponse is now allowed. But I think this is fine.

Since c.text() and c.body() cannot return a type, they cannot be defined in the ZodMediaTypeObject schema. So, if you want to return a response with c.text() or c.body(), write like this:

const route = createRoute({
  method: 'get',
  path: '/hello',
  responses: {
    200: {
      description: 'Say hello'
    }
  }
})

app.openapi(route, (c) => {
  return c.text('hello')
})
WiktorKryzia commented 9 months ago

Hi @yusukebe

Thanks for the quick reply. Ok, I understand, so I can only include important information in the description.

I think it's a bit of a pity that it can't be declared in such a way as to explicitly and precisely specify the returned Content-Type (at least) and some example of the returned response in dedicated fields. Some OpenAPI UIs then nicely display this example, response schema type and add an 'Accept' header to the example query.

Some simple example: to describe an endpoint that returns the exact API version, I have to describe it like this:

  responses: {
    200: {
      description: 'Returns string (`text/plain` body) with API version number, e.g. `API v1.0.0`.',
    },
  }

Instead of, e.g.:

responses: {
    200: {
      description: 'Returns string with API version number.',
      content: {
        'text/plain': {
          schema: z.string().openapi({ example: 'API v1.0.0' }),
        },
      },
    },
  }

Better example: if the same endpoint can return data in YAML and JSON format for different status codes, then response for YAML declared only using description can't specified the Content-Type header, so then such a UI in Accept header won't add the appropriate header value for YAML. (on the one hand, this can be interpreted as an error of this UI, but on the other hand, the ability to specify a response header for each status code would be useful and more "transparent" for the reader of the documentation in my opinion)

yusukebe commented 9 months ago

@WiktorKryzia

I understand what you want to do. Thanks for the explanation.

Certainly it would be better to be able to specify content other than JSON. However, since shema supports only JSON, it would be better to make schema optional or undefined for text/plain.

const route = createRoute({
  method: 'get',
  path: '/info',
  responses: {
    200: {
      description: 'OK',
      content: {
        'text/plain': {
          example: 'API v1.0.0',
          schema: undefined
        }
      }
    }
  }
})

This feature can be added.

WiktorKryzia commented 9 months ago

Yes exactly, I think it would be really nice to add such an option.

yusukebe commented 9 months ago

@WiktorKryzia

Thanks! I'll work on it later.

marvinruder commented 3 months ago

Related: #641

@yusukebe Allowing schema: undefined seems to be a nice solution. Will this be implemented soon?

micheledicosmo commented 1 week ago

+1

abielzulio commented 5 hours ago

any updates? would love to have optional defined schema here