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

Content-Type header only works with ctx.body() #3908

Open
lorypelli opened this issue Feb 9, 2025 · 9 comments
Open

Content-Type header only works with ctx.body() #3908

lorypelli opened this issue Feb 9, 2025 · 9 comments
Labels

Comments

@lorypelli
Copy link

What version of Hono are you using?

4.7.0

What runtime/platform is your app running on? (with version if possible)

Cloudflare Workers

What steps can reproduce the bug?

The following code DOESN'T work

ctx.text(await res.text(), {
        headers: {
            'Content-Type': 'text/javascript',
        },
});

The following code DOES work

ctx.body(await res.text(), {
        headers: {
            'Content-Type': 'text/javascript',
        },
});

You should make text function (but probably also json and others) to omit Content-Type header from his typescript type...

What is the expected behavior?

if it doesn't work typescript should complain (also javascript because you shouldn't be able to do a thing like this at all)

What do you see instead?

no error but wrong Content-Type (text/plain in my case)

Additional information

No response

@EdamAme-x
Copy link
Contributor

The internal header seems to take precedence.

const setHeaders = (headers: Headers, map: Record<string, string> = {}) => {
  for (const key of Object.keys(map)) {
    headers.set(key, map[key])
  }
  return headers
}

@lorypelli
Copy link
Author

The internal header seems to take precedence.

const setHeaders = (headers: Headers, map: Record<string, string> = {}) => {
  for (const key of Object.keys(map)) {
    headers.set(key, map[key])
  }
  return headers
}

I guess this is intended behaviour because ctx.text() is just normal text, you shouldn't be able to override Content-Type and in fact it doesn't work...

@lorypelli
Copy link
Author

Just that a sintax like that should throw an error

@EdamAme-x
Copy link
Contributor

hmm...
Whether the error is indicated by the typescript or detected internally, the cost of detector is too high.

I think we need to hope that this issue will make users aware of this.

@lorypelli
Copy link
Author

hmm... Whether the error is indicated by the typescript or detected internally, the cost of detector is too high.

I think we need to hope that this issue will make users aware of this.

isn't just Omit<Headers, "Content-Type"> for typescript and if (headers.includes("Content-Type") for javascript?

@yusukebe
Copy link
Member

yusukebe commented Feb 9, 2025

Like this? If you use c.text(),, the content type will be inferred as only text/plain. But I can't find a good way to throw a type error now. And type definitions will be complex.

Image

@EdamAme-x
Copy link
Contributor

I don't think it is necessary to add this to the internal implementation, as it can be avoided if the user is careful.

Types are something to consider, but Hono's internal types are flapped several times, which complicates the code a bit.

@yusukebe
Copy link
Member

yusukebe commented Feb 9, 2025

This is the diff (but verbose):

diff --git a/src/context.ts b/src/context.ts
index 9c19eff3..50ed2d06 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -21,10 +21,9 @@ import type {
   SimplifyDeepArray,
 } from './utils/types'
 
-type HeaderRecord =
-  | Record<'Content-Type', BaseMime>
-  | Record<ResponseHeader, string | string[]>
-  | Record<string, string | string[]>
+type HeaderRecord<
+  ContentType extends Record<'Content-Type', string> = Record<'Content-Type', BaseMime>
+> = ContentType | Record<ResponseHeader, string | string[]> | Record<string, string | string[]>
 
 /**
  * Data type can be a string, ArrayBuffer, Uint8Array (buffer), or ReadableStream.
@@ -139,15 +138,17 @@ interface BodyRespond {
  *
  * @returns {Response & TypedResponse<T, U, 'text'>} - The response after rendering the text content, typed with the provided text and status code types.
  */
-interface TextRespond {
+interface TextRespond<
+  ContentType extends Record<'Content-Type', string> = Record<'Content-Type', BaseMime>
+> {
   <T extends string, U extends ContentfulStatusCode = ContentfulStatusCode>(
     text: T,
     status?: U,
-    headers?: HeaderRecord
+    headers?: HeaderRecord<ContentType>
   ): Response & TypedResponse<T, U, 'text'>
   <T extends string, U extends ContentfulStatusCode = ContentfulStatusCode>(
     text: T,
-    init?: ResponseOrInit<U>
+    init?: ResponseOrInit<U, ContentType>
   ): Response & TypedResponse<T, U, 'text'>
 }
 
@@ -256,20 +257,28 @@ interface SetHeaders {
   (name: string, value?: string, options?: SetHeadersOptions): void
 }
 
-type ResponseHeadersInit =
+type ResponseHeadersInit<
+  ContentType extends Record<'Content-Type', string> = Record<'Content-Type', BaseMime>
+> =
   | [string, string][]
-  | Record<'Content-Type', BaseMime>
+  | ContentType
   | Record<ResponseHeader, string>
   | Record<string, string>
   | Headers
 
-interface ResponseInit<T extends StatusCode = StatusCode> {
-  headers?: ResponseHeadersInit
+interface ResponseInit<
+  T extends StatusCode = StatusCode,
+  ContentType extends Record<'Content-Type', string> = Record<'Content-Type', BaseMime>
+> {
+  headers?: ResponseHeadersInit<ContentType>
   status?: T
   statusText?: string
 }
 
-type ResponseOrInit<T extends StatusCode = StatusCode> = ResponseInit<T> | Response
+type ResponseOrInit<
+  T extends StatusCode = StatusCode,
+  ContentType extends Record<'Content-Type', string> = Record<'Content-Type', BaseMime>
+> = ResponseInit<T, ContentType> | Response
 
 export const TEXT_PLAIN = 'text/plain; charset=UTF-8'
 
@@ -749,7 +758,7 @@ export class Context<
    * })
    * ```
    */
-  text: TextRespond = (
+  text: TextRespond<Record<'Content-Type', 'text/plain'>> = (
     text: string,
     arg?: ContentfulStatusCode | ResponseOrInit,
     headers?: HeaderRecord

@EdamAme-x
Copy link
Contributor

The types are even more complex...
I wonder the inference time.

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

No branches or pull requests

3 participants