ronantakizawa/codereview-bench huggingface dataset:
Repository: honojs/hono
File: src/hono.test.ts (TypeScript)

Code before:

describe('GET Request', () => {
  const app = new Hono()

  app.get('/hello', async () => {
    return new Response('hello', {
      status: 200,
      statusText: 'Hono is OK',
    })
  })

  app.get('/hello-with-shortcuts', (c) => {
    c.header('X-Custom', 'This is Hono')
    c.status(201)
    return c.html('<h1>Hono!!!</h1>')
  })

  app.get('/hello-env', (c) => {
    return c.json(c.env)
  })

  it('GET http://localhost/hello is ok', async () => {
    const res = await app.request('http://localhost/hello')
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(res.statusText).toBe('Hono is OK')
    expect(await res.text()).toBe('hello')
  })

  it('GET httphello is ng', async () => {
    const res = await app.request('httphello')
    expect(res.status).toBe(404)
  })

  it('GET /hello is ok', async () => {
    const res = await app.request('/hello')
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(res.statusText).toBe('Hono is OK')
    expect(await res.text()).toBe('hello')
  })

  it('GET hello is ok', async () => {
    const res = await app.request('hello')
    expect(res).not.toBeNull()
    expect(res.status).toBe(200)
    expect(res.statusText).toBe('Hono is OK')
    expect(await res.text()).toBe('hello')
  })

  it('GET /hello-with-shortcuts is ok', async () => {

Review comment (suggestion):
This - adding the tests to handle the `Proxy` object is good!

Code after:

describe('GET Request', () => {
  describe('without middleware', () => {
    // In other words, this is a test for cases that do not use `compose()`

    const app = new Hono()

    app.get('/hello', async () => {
      return new Response('hello', {
        status: 200,
        statusText: 'Hono is OK',
      })
    })

    app.get('/hello-with-shortcuts', (c) => {
      c.header('X-Custom', 'This is Hono')
      c.status(201)
      return c.html('<h1>Hono!!!</h1>')
    })

    app.get('/hello-env', (c) => {
      return c.json(c.env)
    })

    app.get(
      '/proxy-object',
      () =>
        new Proxy(new Response('proxy'), {
          get(target, prop: keyof Response) {
            return target[prop]
          },
        })
    )

    app.get(
      '/async-proxy-object',
      async () =>
        new Proxy(new Response('proxy'), {
          get(target, prop: keyof Response) {
            return target[prop]
          },
        })
    )

    it('GET http://localhost/hello is ok', async () => {
      const res = await app.request('http://localhost/hello')
      expect(res).not.toBeNull()
      expect(res.status).toBe(200)
      expect(res.statusText).toBe('Hono is OK')
      expect(await res.text()).toBe('hello')
    })
Repository: honojs/hono
File: src/adapter/cloudflare-workers/serve-static.ts (TypeScript)

Code before:
  return async (c, next) => {
    // Do nothing if Response is already set
    if (c.finalized) {
      await next()
      return
    }

    const url = new URL(c.req.url)
    const filename = options.path ?? decodeURI(url.pathname)
    const path = getFilePath({
      filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename,
      root: options.root,
      defaultDocument: DEFAULT_DOCUMENT,
    })

    if (!path) return await next()

    const content = await getContentFromKVAsset(path, {
      manifest: options.manifest,
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      namespace: options.namespace ? options.namespace : c.env ? c.env.__STATIC_CONTENT : undefined,
    })

    if (content) {
      const mimeType = getMimeType(path, options.mime)
      if (mimeType) {
        c.header('Content-Type', mimeType)
      }
      // Return Response object
      return c.body(content)
    }

    await options.onNotFound?.(path, c)
    await next()
    return
  }
}


Review comment (suggestion):
I would suggest using the default mime types if there were no mime types specified.

```ts
let mimeType: string | undefined = undefined
if (options.mime) {
  mimeType = getMimeType(path, options.mime) ?? getMimeType(path)
} else {
  mimeType = getMimeType(path)
}
```

Code after:
  return async (c, next) => {
    // Do nothing if Response is already set
    if (c.finalized) {
      await next()
      return
    }

    const url = new URL(c.req.url)
    const filename = options.path ?? decodeURI(url.pathname)
    const path = getFilePath({
      filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename,
      root: options.root,
      defaultDocument: DEFAULT_DOCUMENT,
    })

    if (!path) return await next()

    const content = await getContentFromKVAsset(path, {
      manifest: options.manifest,
      // eslint-disable-next-line @typescript-eslint/ban-ts-comment
      // @ts-ignore
      namespace: options.namespace ? options.namespace : c.env ? c.env.__STATIC_CONTENT : undefined,
    })

    if (content) {
      let mimeType: string | undefined = undefined
      if (options.mimes) {
        mimeType = getMimeType(path, options.mimes) ?? getMimeType(path)
      } else {
        mimeType = getMimeType(path)
      }
      if (mimeType) {
        c.header('Content-Type', mimeType)
      }
      // Return Response object
      return c.body(content)
    }

    await options.onNotFound?.(path, c)
    await next()
    return
  }
}
Repository: honojs/hono
File: deno_dist/utils/mime.ts (TypeScript)

Code before:
export const getMimeType = (filename: string, mime = baseMime): string | undefined => {
  const regexp = /\.([a-zA-Z0-9]+?)$/
  const match = filename.match(regexp)
  if (!match) return
  let mimeType = mime[match[1]]
  if ((mimeType && mimeType.startsWith('text')) || mimeType === 'application/json') {
    mimeType += '; charset=utf-8'
  }
  return mimeType
}

export const getExtension = (mimeType: string): string | undefined => {
  return Object.keys(baseMime).find((ext) => baseMime[ext] === mimeType)
}

const baseMime: Record<string, string> = {
  aac: 'audio/aac',
  abw: 'application/x-abiword',
  arc: 'application/x-freearc',
  avi: 'video/x-msvideo',
  avif: 'image/avif',
  av1: 'video/av1',
  azw: 'application/vnd.amazon.ebook',
  bin: 'application/octet-stream',
  bmp: 'image/bmp',
  bz: 'application/x-bzip',

Review comment (suggestion):
I think `mimes` is better since it handles multiple Mime Types.

```ts
mimes = baseMimes
```

Code after:
export const getMimeType = (filename: string, mimes = baseMimes): string | undefined => {
  const regexp = /\.([a-zA-Z0-9]+?)$/
  const match = filename.match(regexp)
  if (!match) return
  let mimeType = mimes[match[1]]
  if ((mimeType && mimeType.startsWith('text')) || mimeType === 'application/json') {
    mimeType += '; charset=utf-8'
  }
  return mimeType
}

export const getExtension = (mimeType: string): string | undefined => {
  return Object.keys(baseMimes).find((ext) => baseMimes[ext] === mimeType)
}

const baseMimes: Record<string, string> = {
  aac: 'audio/aac',
  abw: 'application/x-abiword',
  arc: 'application/x-freearc',
  avi: 'video/x-msvideo',
  avif: 'image/avif',
  av1: 'video/av1',
  azw: 'application/vnd.amazon.ebook',
  bin: 'application/octet-stream',
  bmp: 'image/bmp',
  bz: 'application/x-bzip',
Repository: honojs/hono
File: deno_dist/utils/mime.ts (TypeScript)

Code before:
export const getMimeType = (filename: string, mime = baseMime): string | undefined => {
  const regexp = /\.([a-zA-Z0-9]+?)$/
  const match = filename.match(regexp)
  if (!match) return
  let mimeType = mime[match[1]]
  if ((mimeType && mimeType.startsWith('text')) || mimeType === 'application/json') {
    mimeType += '; charset=utf-8'
  }
  return mimeType
}

export const getExtension = (mimeType: string): string | undefined => {
  return Object.keys(baseMime).find((ext) => baseMime[ext] === mimeType)
}

const baseMime: Record<string, string> = {
  aac: 'audio/aac',
  abw: 'application/x-abiword',
  arc: 'application/x-freearc',
  avi: 'video/x-msvideo',
  avif: 'image/avif',
  av1: 'video/av1',
  azw: 'application/vnd.amazon.ebook',
  bin: 'application/octet-stream',
  bmp: 'image/bmp',
  bz: 'application/x-bzip',
  bz2: 'application/x-bzip2',
  csh: 'application/x-csh',
  css: 'text/css',
  csv: 'text/csv',
  doc: 'application/msword',
  docx: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
  eot: 'application/vnd.ms-fontobject',
  epub: 'application/epub+zip',
  gif: 'image/gif',
  gz: 'application/gzip',
  htm: 'text/html',
  html: 'text/html',
  ico: 'image/x-icon',
  ics: 'text/calendar',
  jar: 'application/java-archive',

Review comment (suggestion):
I would prefer `baseMimes` for the same reason as above.

Code after:
export const getMimeType = (filename: string, mimes = baseMimes): string | undefined => {
  const regexp = /\.([a-zA-Z0-9]+?)$/
  const match = filename.match(regexp)
  if (!match) return
  let mimeType = mimes[match[1]]
  if ((mimeType && mimeType.startsWith('text')) || mimeType === 'application/json') {
    mimeType += '; charset=utf-8'
  }
  return mimeType
}

export const getExtension = (mimeType: string): string | undefined => {
  return Object.keys(baseMimes).find((ext) => baseMimes[ext] === mimeType)
}

const baseMimes: Record<string, string> = {
  aac: 'audio/aac',
  abw: 'application/x-abiword',
  arc: 'application/x-freearc',
  avi: 'video/x-msvideo',
  avif: 'image/avif',
  av1: 'video/av1',
  azw: 'application/vnd.amazon.ebook',
  bin: 'application/octet-stream',
  bmp: 'image/bmp',
  bz: 'application/x-bzip',
  bz2: 'application/x-bzip2',
  csh: 'application/x-csh',
  css: 'text/css',
  csv: 'text/csv',
  doc: 'application/msword',
  docx: 'application/vnd.openxmlformats-officedocument.wordprocessingml.document',
  eot: 'application/vnd.ms-fontobject',
  epub: 'application/epub+zip',
  gif: 'image/gif',
  gz: 'application/gzip',
  htm: 'text/html',
  html: 'text/html',
  ico: 'image/x-icon',
  ics: 'text/calendar',
  jar: 'application/java-archive',
Repository: honojs/hono
File: src/adapter/cloudflare-workers/serve-static.ts (TypeScript)

Code before:
// @denoify-ignore
import type { KVNamespace } from '@cloudflare/workers-types'
import type { Context } from '../../context'
import type { Env, MiddlewareHandler } from '../../types'
import { getFilePath } from '../../utils/filepath'
import { getMimeType } from '../../utils/mime'
import { getContentFromKVAsset } from './utils'

export type ServeStaticOptions<E extends Env = Env> = {
  root?: string
  path?: string
  mime?: Record<string, string>
  manifest: object | string
  namespace?: KVNamespace
  rewriteRequestPath?: (path: string) => string
  onNotFound?: (path: string, c: Context<E>) => void | Promise<void>
}

const DEFAULT_DOCUMENT = 'index.html'

// This middleware is available only on Cloudflare Workers.
export const serveStatic = <E extends Env = Env>(
  options: ServeStaticOptions<E>
): MiddlewareHandler => {
  return async (c, next) => {
    // Do nothing if Response is already set
    if (c.finalized) {
      await next()
      return
    }

    const url = new URL(c.req.url)
    const filename = options.path ?? decodeURI(url.pathname)
    const path = getFilePath({
      filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename,
      root: options.root,
      defaultDocument: DEFAULT_DOCUMENT,

Review comment (suggestion):
I think `mimes` is better, for the same reason as above.

Code after:
// @denoify-ignore
import type { KVNamespace } from '@cloudflare/workers-types'
import type { Context } from '../../context'
import type { Env, MiddlewareHandler } from '../../types'
import { getFilePath } from '../../utils/filepath'
import { getMimeType } from '../../utils/mime'
import { getContentFromKVAsset } from './utils'

export type ServeStaticOptions<E extends Env = Env> = {
  root?: string
  path?: string
  mimes?: Record<string, string>
  manifest: object | string
  namespace?: KVNamespace
  rewriteRequestPath?: (path: string) => string
  onNotFound?: (path: string, c: Context<E>) => void | Promise<void>
}

const DEFAULT_DOCUMENT = 'index.html'

// This middleware is available only on Cloudflare Workers.
export const serveStatic = <E extends Env = Env>(
  options: ServeStaticOptions<E>
): MiddlewareHandler => {
  return async (c, next) => {
    // Do nothing if Response is already set
    if (c.finalized) {
      await next()
      return
    }

    const url = new URL(c.req.url)
    const filename = options.path ?? decodeURI(url.pathname)
    const path = getFilePath({
      filename: options.rewriteRequestPath ? options.rewriteRequestPath(filename) : filename,
      root: options.root,
      defaultDocument: DEFAULT_DOCUMENT,
Repository: honojs/hono
File: deno_dist/middleware/body-limit/index.ts (TypeScript)

Code before:
    super(message)
    this.name = 'BodyLimitError'
  }
}

/**
 * Body Limit Middleware
 *
 * @example
 * ```ts
 * app.post(
 *  '/hello',
 *  bodyLimit({
 *    maxSize: 15 * Unit.b,
 *    onError: (c) => {
 *      return c.text('overflow :(', 413)
 *    }
 *  }),
 *  (c) => {
 *    return c.text('pass :)')
 *  }
 * )
 * ```
 */
export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
  const onError: OnError = options.onError || ((c) => c.text('413 Request Entity Too Large', 413))
  const maxSize = options.maxSize

  return async function bodyLimit(c, next) {
    if (!c.req.raw.body) {
      // maybe GET or HEAD request
      return next()
    }

    if (c.req.raw.headers.has('content-length')) {
      // we can trust content-length header because it's already validated by server
      const contentLength = parseInt(c.req.raw.headers.get('content-length') || '0', 10)
      return contentLength > maxSize ? onError(c) : next()
    }

    // maybe chunked transfer encoding

    let size = 0
    const rawReader = c.req.raw.body.getReader()
    const reader = new ReadableStream({
      async start(controller) {
        try {
          for (;;) {
            const { done, value } = await rawReader.read()
            if (done) {
              break

Review comment (suggestion):
It is better to make `'413 Request Entity Too Large'` as a constant to reuse in line 66.

Code after:

class BodyLimitError extends Error {
  constructor(message: string) {
    super(message)
    this.name = 'BodyLimitError'
  }
}

/**
 * Body Limit Middleware
 *
 * @example
 * ```ts
 * app.post(
 *  '/hello',
 *  bodyLimit({
 *    maxSize: 15 * Unit.b,
 *    onError: (c) => {
 *      return c.text('overflow :(', 413)
 *    }
 *  }),
 *  (c) => {
 *    return c.text('pass :)')
 *  }
 * )
 * ```
 */
export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
  const onError: OnError =
    options.onError ||
    (() => {
      const res = new Response(ERROR_MESSAGE, {
        status: 413,
      })
      throw new HTTPException(413, { res })
    })
  const maxSize = options.maxSize

  return async function bodyLimit(c, next) {
    if (!c.req.raw.body) {
      // maybe GET or HEAD request
      return next()
    }

    if (c.req.raw.headers.has('content-length')) {
      // we can trust content-length header because it's already validated by server
      const contentLength = parseInt(c.req.raw.headers.get('content-length') || '0', 10)
      return contentLength > maxSize ? onError(c) : next()
    }

    // maybe chunked transfer encoding
Repository: honojs/hono
File: src/helper/ssg/index.ts (TypeScript)

Code before:
 * `toSSG` is an experimental feature.
 * The API might be changed.
 */
export const toSSG: ToSSGInterface = async (app, fs, options) => {
  let result: ToSSGResult | undefined = undefined
  try {
    const outputDir = options?.dir ?? './static'
    const maps = await fetchRoutesContent(
      app,
      options?.beforeRequestHook,
      options?.afterResponseHook
    )
    const files = await saveContentToFiles(maps, fs, outputDir)
    result = { success: true, files }
  } catch (error) {
    const errorObj = error instanceof Error ? error : new Error(String(error))
    result = { success: false, files: [], error: errorObj }
  }
  await options?.afterGenerateHook?.(result)
  return result
}

/**
 * @experimental
 * `isSSGContext` is an experimental feature.
 * The API might be changed.
 */
export const isSSGContext = (c: Context): boolean => !!c.env?.[SSG_CONTEXT]

/**
 * @experimental
 * `disableSSG` is an experimental feature.
 * The API might be changed.
 */
export const disableSSG = (): MiddlewareHandler =>
  async function disableSSG(c, next) {
    if (isSSGContext(c)) {
      return SSG_DISABLED_RESPONSE
    }
    await next()
  }

/**
 * @experimental
 * `onlySSG` is an experimental feature.
 * The API might be changed.
 */
export const onlySSG = (): MiddlewareHandler =>
  async function onlySSG(c, next) {
    if (!isSSGContext(c)) {
      return c.notFound()

Review comment (refactor):
It can get the variables **only** for Cloudflare Workers from `c.env`. To handle environment variables on all runtimes, we should use `env()` in `hono/adapter`:

https://hono.dev/helpers/adapter#env
https://github.com/honojs/hono/blob/main/src/helper/adapter/index.ts#L5


Code after:
 */
export const toSSG: ToSSGInterface = async (app, fs, options) => {
  let result: ToSSGResult | undefined = undefined
  try {
    const outputDir = options?.dir ?? './static'
    const maps = await fetchRoutesContent(
      app,
      options?.beforeRequestHook,
      options?.afterResponseHook
    )
    const files = await saveContentToFiles(maps, fs, outputDir)
    result = { success: true, files }
  } catch (error) {
    const errorObj = error instanceof Error ? error : new Error(String(error))
    result = { success: false, files: [], error: errorObj }
  }
  await options?.afterGenerateHook?.(result)
  return result
}

/**
 * @experimental
 * `isSSGContext` is an experimental feature.
 * The API might be changed.
 */
export const isSSGContext = (c: Context): boolean => !!c.env?.[SSG_CONTEXT]

/**
 * @experimental
 * `disableSSG` is an experimental feature.
 * The API might be changed.
 */
export const disableSSG = (): MiddlewareHandler =>
  async function disableSSG(c, next) {
    if (isSSGContext(c)) {
      return SSG_DISABLED_RESPONSE
    }
    await next()
  }

/**
 * @experimental
 * `onlySSG` is an experimental feature.
 * The API might be changed.
 */
export const onlySSG = (): MiddlewareHandler =>
  async function onlySSG(c, next) {
    if (!isSSGContext(c)) {
      return c.notFound()
    }
    await next()
Repository: honojs/hono
File: deno_dist/client/utils.ts (TypeScript)

Code before:
import type { ObjectType } from './types.ts'

export const mergePath = (base: string, path: string) => {
  base = base.replace(/\/+$/, '')
  base = base + '/'
  path = path.replace(/^\/+/, '')
  return base + path
}

export const replaceUrlParam = (urlString: string, params: Record<string, string>) => {
  for (const [k, v] of Object.entries(params)) {
    const reg = new RegExp('/:' + k + '(\\{([^{}]*?(\\{[^{}]*?\\})?)+?\\})?')
    urlString = urlString.replace(reg, `/${v}`)
  }
  return urlString
}

export const removeIndexString = (urlSting: string) => {
  return urlSting.replace(/\/index$/, '/')
}

function isObject(item: unknown): item is ObjectType {
  return typeof item === 'object' && item !== null && !Array.isArray(item)
}

export function deepMerge<T>(target: T, source: Record<string, unknown>): T {
  if (!isObject(target) && !isObject(source)) {
    return source as T
  }
  const merged = { ...target } as ObjectType<T>

  for (const key in source) {
    const value = source[key]
    if (isObject(merged[key]) && isObject(value)) {
      merged[key] = deepMerge(merged[key], value)
    } else {
      merged[key] = value as T[keyof T] & T

Review comment (suggestion):
If we just pass that test pattern, the following simple code will work as expected.

```ts
const reg = new RegExp('/:' + k + '({[^/]+})?')
```

Code after:
import type { ObjectType } from './types.ts'

export const mergePath = (base: string, path: string) => {
  base = base.replace(/\/+$/, '')
  base = base + '/'
  path = path.replace(/^\/+/, '')
  return base + path
}

export const replaceUrlParam = (urlString: string, params: Record<string, string>) => {
  for (const [k, v] of Object.entries(params)) {
    const reg = new RegExp('/:' + k + '(?:{[^/]+})?')
    urlString = urlString.replace(reg, `/${v}`)
  }
  return urlString
}

export const removeIndexString = (urlSting: string) => {
  return urlSting.replace(/\/index$/, '/')
}

function isObject(item: unknown): item is ObjectType {
  return typeof item === 'object' && item !== null && !Array.isArray(item)
}

export function deepMerge<T>(target: T, source: Record<string, unknown>): T {
  if (!isObject(target) && !isObject(source)) {
    return source as T
  }
  const merged = { ...target } as ObjectType<T>

  for (const key in source) {
    const value = source[key]
    if (isObject(merged[key]) && isObject(value)) {
      merged[key] = deepMerge(merged[key], value)
    } else {
      merged[key] = value as T[keyof T] & T
Repository: honojs/hono
File: src/helper/streaming/sse.ts (TypeScript)

Code before:

    const sseData =
      [
        message.event && `event: ${message.event}`,
        data,
        message.id && `id: ${message.id}`,
        message.retry && `retry: ${message.retry}`,
      ]
        .filter(Boolean)
        .join('\n') + '\n\n'

    await this.write(sseData)
  }
}

const setSSEHeaders = (context: Context) => {
  context.header('Transfer-Encoding', 'chunked')
  context.header('Content-Type', 'text/event-stream')
  context.header('Cache-Control', 'no-cache')
  context.header('Connection', 'keep-alive')
}

export const streamSSE = (
  c: Context,
  cb: (stream: SSEStreamingApi) => Promise<void>,
  onError?: (e: Error, stream: StreamingApi) => Promise<void>
) => {
  const { readable, writable } = new TransformStream()
  const stream = new SSEStreamingApi(writable, readable)
  ;(async () => {
    try {
      await cb(stream)
    } catch (e) {
      if (e instanceof Error && onError) {
        await onError(e, stream)
      }
    } finally {
      stream.close()
    }
  })()
  setSSEHeaders(c)
  return c.newResponse(stream.responseReadable)
}


Review comment (suggestion):
Should be:

```ts
(e: Error, stream: SSEStreamingApi) => Promise<void>
```

Code after:

    const sseData =
      [
        message.event && `event: ${message.event}`,
        data,
        message.id && `id: ${message.id}`,
        message.retry && `retry: ${message.retry}`,
      ]
        .filter(Boolean)
        .join('\n') + '\n\n'

    await this.write(sseData)
  }
}

const setSSEHeaders = (context: Context) => {
  context.header('Transfer-Encoding', 'chunked')
  context.header('Content-Type', 'text/event-stream')
  context.header('Cache-Control', 'no-cache')
  context.header('Connection', 'keep-alive')
}

export const streamSSE = (
  c: Context,
  cb: (stream: SSEStreamingApi) => Promise<void>,
  onError?: (e: Error, stream: SSEStreamingApi) => Promise<void>
) => {
  const { readable, writable } = new TransformStream()
  const stream = new SSEStreamingApi(writable, readable)
  ;(async () => {
    try {
      await cb(stream)
    } catch (e) {
      if (e instanceof Error && onError) {
        await onError(e, stream)
      } else {
        console.error(e)
      }
    } finally {
      stream.close()
    }
  })()
  setSSEHeaders(c)
  return c.newResponse(stream.responseReadable)
}