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

Code before:
const mergeBuffers = (buffer1: ArrayBuffer | undefined, buffer2: Uint8Array): Uint8Array => {
  if (!buffer1) {
    return buffer2
  }
  const merged = new Uint8Array(buffer1.byteLength + buffer2.byteLength)
  merged.set(new Uint8Array(buffer1), 0)
  merged.set(buffer2, buffer1.byteLength)
  return merged
}

export const generateDigest = async (
  stream: ReadableStream<Uint8Array> | null,
  generator?: (body: Uint8Array) => ArrayBuffer | Promise<ArrayBuffer>
): Promise<string | null> => {
  if (!stream || !generator) {
    return null
  }

  let result: ArrayBuffer | undefined = undefined

  const reader = stream.getReader()
  for (;;) {
    const { value, done } = await reader.read()
    if (done) {
      break
    }

    result = await generator(mergeBuffers(result, value))
  }

  if (!result) {
    return null
  }

  return Array.prototype.map
    .call(new Uint8Array(result), (x) => x.toString(16).padStart(2, '0'))
    .join('')
}

Review comment (suggestion):
It depends on the situation, but in general, I think it's better to avoid optional arguments in internal APIs.

Code after:
const mergeBuffers = (buffer1: ArrayBuffer | undefined, buffer2: Uint8Array): Uint8Array => {
  if (!buffer1) {
    return buffer2
  }
  const merged = new Uint8Array(buffer1.byteLength + buffer2.byteLength)
  merged.set(new Uint8Array(buffer1), 0)
  merged.set(buffer2, buffer1.byteLength)
  return merged
}

export const generateDigest = async (
  stream: ReadableStream<Uint8Array> | null,
  generator: (body: Uint8Array) => ArrayBuffer | Promise<ArrayBuffer>
): Promise<string | null> => {
  if (!stream) {
    return null
  }

  let result: ArrayBuffer | undefined = undefined

  const reader = stream.getReader()
  for (;;) {
    const { value, done } = await reader.read()
    if (done) {
      break
    }

    result = await generator(mergeBuffers(result, value))
  }

  if (!result) {
    return null
  }

  return Array.prototype.map
    .call(new Uint8Array(result), (x) => x.toString(16).padStart(2, '0'))
    .join('')
}
Repository: honojs/hono
File: src/middleware/etag/index.ts (TypeScript)

Code before:
 * @example
 * ```ts
 * const app = new Hono()
 *
 * app.use('/etag/*', etag())
 * app.get('/etag/abc', (c) => {
 *   return c.text('Hono is cool')
 * })
 * ```
 */
export const etag = (options?: ETagOptions): MiddlewareHandler => {
  const retainedHeaders = options?.retainedHeaders ?? RETAINED_304_HEADERS
  const weak = options?.weak ?? false
  let generator = options?.generateDigest

  if (!generator) {
    if (crypto && crypto.subtle) {
      generator = (body: Uint8Array) =>
        crypto.subtle.digest(
          {
            name: 'SHA-1',
          },
          body
        )
    }
  }

  return async function etag(c, next) {
    const ifNoneMatch = c.req.header('If-None-Match') ?? null

    await next()

    const res = c.res as Response
    let etag = res.headers.get('ETag')

    if (!etag) {
      const hash = await generateDigest(res.clone().body, generator)
      if (hash === null) {
        return
      }
      etag = weak ? `W/"${hash}"` : `"${hash}"`
    }

    if (etagMatches(etag, ifNoneMatch)) {
      c.res = new Response(null, {
        status: 304,
        statusText: 'Not Modified',
        headers: {
          ETag: etag,
        },
      })

Review comment (suggestion):
The initialisation conditions are complicated, so I think it would be better to make it a separate function.

Code after:
  }

  return generator
}

/**
 * ETag Middleware for Hono.
 *
 * @see {@link https://hono.dev/docs/middleware/builtin/etag}
 *
 * @param {ETagOptions} [options] - The options for the ETag middleware.
 * @param {boolean} [options.weak=false] - Define using or not using a weak validation. If true is set, then `W/` is added to the prefix of the value.
 * @param {string[]} [options.retainedHeaders=RETAINED_304_HEADERS] - The headers that you want to retain in the 304 Response.
 * @param {function(Uint8Array): ArrayBuffer | Promise<ArrayBuffer>} [options.generateDigest] -
 * A custom digest generation function. By default, it uses 'SHA-1'
 * This function is called with the response body as a `Uint8Array` and should return a hash as an `ArrayBuffer` or a Promise of one.
 * @returns {MiddlewareHandler} The middleware handler function.
 *
 * @example
 * ```ts
 * const app = new Hono()
 *
 * app.use('/etag/*', etag())
 * app.get('/etag/abc', (c) => {
 *   return c.text('Hono is cool')
 * })
 * ```
 */
export const etag = (options?: ETagOptions): MiddlewareHandler => {
  const retainedHeaders = options?.retainedHeaders ?? RETAINED_304_HEADERS
  const weak = options?.weak ?? false
  const generator = initializeGenerator(options?.generateDigest)

  return async function etag(c, next) {
    const ifNoneMatch = c.req.header('If-None-Match') ?? null

    await next()

    const res = c.res as Response
    let etag = res.headers.get('ETag')

    if (!etag) {
      if (!generator) {
        return
      }
      const hash = await generateDigest(res.clone().body, generator)
      if (hash === null) {
        return
      }
      etag = weak ? `W/"${hash}"` : `"${hash}"`
    }
Repository: honojs/hono
File: runtime-tests/deno/hono.test.ts (TypeScript)

Code before:
    c.header('x-param', c.req.param('foo'))
    c.header('x-query', c.req.query('q') || '')
    return c.text('Hello Deno!')
  })

  const res = await app.request('/foo?q=bar')
  assertEquals(res.status, 200)
  assertEquals(await res.text(), 'Hello Deno!')
  assertEquals(res.headers.get('x-param'), 'foo')
  assertEquals(res.headers.get('x-query'), 'bar')
})

Deno.test('runtime', () => {
  assertEquals(getRuntimeKey(), 'deno')
})

Deno.test('environment variables', () => {
  const c = new Context(new Request('http://localhost/'))
  const { NAME } = env<{ NAME: string }>(c)
  assertEquals(NAME, 'Deno')
})

Deno.test('Buffers', async () => {
  const app = new Hono().get('/', async (c) => {
    const encoder = new TextEncoder()
    return c.body(encoder.encode('hello'))
  })

  const res = await app.request('/')
  assertEquals(res.status, 200)
  assertEquals(await res.text(), 'hello')
})


Review comment (suggestion):
Seems a bit odd to use `Uint8Array` for testing buffers, though,
It may make sense given that it was removed from Deno's API.

https://docs.deno.com/runtime/reference/migration_guide/#api-changes

Code after:
import { assertEquals } from '@std/assert'
import { Buffer } from 'node:buffer'

import { Context } from '../../src/context.ts'
import { env, getRuntimeKey } from '../../src/helper/adapter/index.ts'
import { Hono } from '../../src/hono.ts'

// Test just only minimal patterns.
// Because others are tested well in Cloudflare Workers environment already.

Deno.env.set('NAME', 'Deno')

Deno.test('Hello World', async () => {
  const app = new Hono()
  app.get('/:foo', (c) => {
    c.header('x-param', c.req.param('foo'))
    c.header('x-query', c.req.query('q') || '')
    return c.text('Hello Deno!')
  })

  const res = await app.request('/foo?q=bar')
  assertEquals(res.status, 200)
  assertEquals(await res.text(), 'Hello Deno!')
  assertEquals(res.headers.get('x-param'), 'foo')
  assertEquals(res.headers.get('x-query'), 'bar')
})

Deno.test('runtime', () => {
  assertEquals(getRuntimeKey(), 'deno')
})

Deno.test('environment variables', () => {
  const c = new Context(new Request('http://localhost/'))
  const { NAME } = env<{ NAME: string }>(c)
  assertEquals(NAME, 'Deno')
})
Repository: honojs/hono
File: src/utils/color.ts (TypeScript)

Code before:
/**
 * @module
 * Color utility.
 */

/**
 * Get whether color change on terminal is enabled or disabled.
 * If `NO_COLOR` environment variable is set, this function returns `false`.
 * @see {@link https://no-color.org/}
 *
 * @returns {boolean}
 */
export function getColorEnabled(): boolean {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  const { process, Deno } = globalThis as any

  const isNoColor =
    typeof Deno?.noColor === 'boolean'
      ? (Deno.noColor as boolean)
      : process !== undefined
      ? // eslint-disable-next-line no-unsafe-optional-chaining
        'NO_COLOR' in process?.env
      : false

  return !isNoColor
}


Review comment (suggestion):
got this error from this line:

<img width="874" alt="image" src="https://github.com/user-attachments/assets/04d427a8-62e9-46e3-a96a-65f5b30841e2" />

environment: monorepo + nextjs + hono (v4.8.0)

cc @yusukebe 

Code after:
    typeof Deno?.noColor === 'boolean'
      ? (Deno.noColor as boolean)
      : process !== undefined
      ? // eslint-disable-next-line no-unsafe-optional-chaining
        'NO_COLOR' in process?.env
      : false

  return !isNoColor
}

/**
 * Get whether color change on terminal is enabled or disabled.
 * If `NO_COLOR` environment variable is set, this function returns `false`.
 * @see {@link https://no-color.org/}
 *
 * @returns {boolean}
 */
export async function getColorEnabledAsync(): Promise<boolean> {
  // eslint-disable-next-line @typescript-eslint/no-explicit-any
  const { navigator } = globalThis as any

  const isNoColor =
    navigator !== undefined && navigator.userAgent === 'Cloudflare-Workers'
      ? // eslint-disable-next-line @typescript-eslint/ban-ts-comment
        // @ts-ignore
        'NO_COLOR' in ((await import('cloudflare:workers')).env ?? {}) // ?? {} is for backward compat
      : !getColorEnabled()

  return !isNoColor
}
Repository: honojs/hono
File: runtime-tests/workerd/index.test.ts (TypeScript)

Code before:
    expect(res.status).toBe(200)
    expect(await res.text()).toBe('Hono')
  })
})

describe('workerd with WebSocket', () => {
  // worker.fetch does not support WebSocket:
  // https://github.com/cloudflare/workers-sdk/issues/4573#issuecomment-1850420973
  it('Should handle the WebSocket connection correctly', async () => {
    const worker = await unstable_dev('./runtime-tests/workerd/index.ts', {
      experimental: { disableExperimentalWarning: true },
    })
    const ws = new WebSocket(`ws://${worker.address}:${worker.port}/ws`)

    const openHandler = vi.fn()
    const messageHandler = vi.fn()
    const closeHandler = vi.fn()

    const waitForOpen = new Promise((resolve) => {
      ws.addEventListener('open', () => {
        openHandler()
        ws.send('Hello')
        resolve(undefined)
      })
      ws.addEventListener('close', async () => {
        closeHandler()
      })
      ws.addEventListener('message', async (event) => {
        messageHandler(event.data)
        ws.close()
      })
    })

    await waitForOpen
    await worker.stop()

    expect(openHandler).toHaveBeenCalled()
    expect(messageHandler).toHaveBeenCalledWith('Hello')
    expect(closeHandler).toHaveBeenCalled()
  })
})


Review comment (bug):
It should `resolve` when in the `close` event. Otherwise, the test will fail.

Code after:
    expect(res.status).toBe(200)
    expect(await res.text()).toBe('Hono')
  })
})

describe('workerd with WebSocket', () => {
  // worker.fetch does not support WebSocket:
  // https://github.com/cloudflare/workers-sdk/issues/4573#issuecomment-1850420973
  it('Should handle the WebSocket connection correctly', async () => {
    const worker = await unstable_dev('./runtime-tests/workerd/index.ts', {
      experimental: { disableExperimentalWarning: true },
    })
    const ws = new WebSocket(`ws://${worker.address}:${worker.port}/ws`)

    const openHandler = vi.fn()
    const messageHandler = vi.fn()
    const closeHandler = vi.fn()

    const waitForOpen = new Promise((resolve) => {
      ws.addEventListener('open', () => {
        openHandler()
        ws.send('Hello')
      })
      ws.addEventListener('close', async () => {
        closeHandler()
        resolve(undefined)
      })
      ws.addEventListener('message', async (event) => {
        messageHandler(event.data)
        ws.close()
      })
    })

    await waitForOpen
    await worker.stop()

    expect(openHandler).toHaveBeenCalled()
    expect(messageHandler).toHaveBeenCalledWith('Hello')
    expect(closeHandler).toHaveBeenCalled()
  })
})
Repository: honojs/hono
File: src/utils/body.test.ts (TypeScript)

Code before:
    })
  })

  it('should parse multiple values if values are `File`', async () => {
    const file1 = new File(['foo'], 'file1', {
      type: 'application/octet-stream',
    })
    const file2 = new File(['bar'], 'file2', {
      type: 'application/octet-stream',
    })
    const data = new FormData()
    data.append('file', file1)
    data.append('file', file2)

    const req = createRequest(FORM_URL, 'POST', data)

    const result = await parseBody(req, { all: true })
    expect(result).toMatchObject({
      file: [
        { name: 'file1', type: 'application/octet-stream' },
        { name: 'file2', type: 'application/octet-stream' },
      ],
    })
  })

  it('should parse multiple values if key ends with `[]`', async () => {
    const data = new FormData()
    data.append('foo[]', 'aaa')
    data.append('foo[]', 'bbb')
    data.append('message', 'hello')

    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['aaa', 'bbb'],
      message: 'hello',
    })
  })

  it('should parse single value as array if the key has []', async () => {
    const data = new FormData()
    data.append('foo[]', 'bar')
    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['bar'],
    })
  })

  it('should return blank object if body is JSON', async () => {
    const payload = { message: 'hello hono' }

Review comment (suggestion):
I think you don't need to change the name of the key. You can keep `file` as is.

Code after:
    })
  })

  it('should parse multiple values if values are `File`', async () => {
    const file1 = new File(['foo'], 'file1', {
      type: 'application/octet-stream',
    })
    const file2 = new File(['bar'], 'file2', {
      type: 'application/octet-stream',
    })
    const data = new FormData()
    data.append('file', file1)
    data.append('file', file2)

    const req = createRequest(FORM_URL, 'POST', data)

    const result = await parseBody(req, { all: true })
    expect(result).toMatchObject({
      file: [
        { name: 'file1', type: 'application/octet-stream' },
        { name: 'file2', type: 'application/octet-stream' },
      ],
    })
  })

  it('should parse multiple values if key ends with `[]`', async () => {
    const data = new FormData()
    data.append('file[]', 'aaa')
    data.append('file[]', 'bbb')
    data.append('message', 'hello')

    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'file[]': ['aaa', 'bbb'],
      message: 'hello',
    })
  })

  it('should parse single value as array if the key has `[]`', async () => {
    const data = new FormData()
    data.append('foo[]', 'bar')
    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['bar'],
    })
  })

  it('should return blank object if body is JSON', async () => {
    const payload = { message: 'hello hono' }
Repository: honojs/hono
File: src/utils/body.test.ts (TypeScript)

Code before:
    const req = createRequest(FORM_URL, 'POST', data)

    const result = await parseBody(req, { all: true })
    expect(result).toMatchObject({
      file: [
        { name: 'file1', type: 'application/octet-stream' },
        { name: 'file2', type: 'application/octet-stream' },
      ],
    })
  })

  it('should parse multiple values if key ends with `[]`', async () => {
    const data = new FormData()
    data.append('foo[]', 'aaa')
    data.append('foo[]', 'bbb')
    data.append('message', 'hello')

    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['aaa', 'bbb'],
      message: 'hello',
    })
  })

  it('should parse single value as array if the key has []', async () => {
    const data = new FormData()
    data.append('foo[]', 'bar')
    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['bar'],
    })
  })

  it('should return blank object if body is JSON', async () => {
    const payload = { message: 'hello hono' }

    const req = createRequest('http://localhost/json', 'POST', JSON.stringify(payload), {
      'Content-Type': 'application/json',
    })

    expect(await parseBody(req)).toEqual({})
  })

  describe('Return type', () => {
    let req: Request
    beforeEach(() => {
      req = createRequest(FORM_URL, 'POST', new FormData())
    })


Review comment (suggestion):
```suggestion
  it('should parse single value as array if the key has `[]`', async () => {
```
nit

Code after:
    const req = createRequest(FORM_URL, 'POST', data)

    const result = await parseBody(req, { all: true })
    expect(result).toMatchObject({
      file: [
        { name: 'file1', type: 'application/octet-stream' },
        { name: 'file2', type: 'application/octet-stream' },
      ],
    })
  })

  it('should parse multiple values if key ends with `[]`', async () => {
    const data = new FormData()
    data.append('file[]', 'aaa')
    data.append('file[]', 'bbb')
    data.append('message', 'hello')

    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'file[]': ['aaa', 'bbb'],
      message: 'hello',
    })
  })

  it('should parse single value as array if the key has `[]`', async () => {
    const data = new FormData()
    data.append('foo[]', 'bar')
    const req = createRequest(FORM_URL, 'POST', data)

    expect(await parseBody(req, { all: true })).toEqual({
      'foo[]': ['bar'],
    })
  })

  it('should return blank object if body is JSON', async () => {
    const payload = { message: 'hello hono' }

    const req = createRequest('http://localhost/json', 'POST', JSON.stringify(payload), {
      'Content-Type': 'application/json',
    })

    expect(await parseBody(req)).toEqual({})
  })

  describe('Return type', () => {
    let req: Request
    beforeEach(() => {
      req = createRequest(FORM_URL, 'POST', new FormData())
    })
Repository: honojs/hono
File: src/adapter/service-worker/index.ts (TypeScript)

Code before:
 * @module
 */
import type { Hono } from '../../hono'
import { handle } from './handler'

/**
 * Registers a Hono app to handle fetch events in a service worker.
 * This sets up `addEventListener('fetch', handle(app))` for the provided app.
 *
 * @param app - The Hono application instance
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { fire } from 'hono/service-worker'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => c.text('Hi'))
 *
 * fire(app)
 * ```
 */
const fire = (app: Hono): void => {
  // @ts-expect-error addEventListener is not typed well
  addEventListener('fetch', handle(app))
}

export { handle, fire }


Review comment (suggestion):
For compatibility with `base fetch` (and a good default for web servers but maybe not a good default for service workers):

```suggestion
const fire = (app: Hono): void => {
  // @ts-expect-error addEventListener is not typed well
  addEventListener('fetch', handle(app. {fetch: undefined}))
}
```

For usage in either web worker or web server, but a potentially unexpected default for web server:

```suggestion
const fire = (app: Hono, opts: HandleOpts): void => {
  // @ts-expect-error addEventListener is not typed well
  addEventListener('fetch', handle(app, opts))
}
```

Code after:
 * @module
 */
import type { Hono } from '../../hono'
import { handle } from './handler'
import type { HandleOptions } from './handler'

/**
 * Registers a Hono app to handle fetch events in a service worker.
 * This sets up `addEventListener('fetch', handle(app, options))` for the provided app.
 *
 * @param app - The Hono application instance
 * @param options - Options for handling requests (fetch defaults to undefined)
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { fire } from 'hono/service-worker'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => c.text('Hi'))
 *
 * fire(app)
 * ```
 */
const fire = (
  app: Hono,
  options: HandleOptions = {
    fetch: undefined,
  }
): void => {
  // @ts-expect-error addEventListener is not typed well in ServiceWorker-like contexts, see: https://github.com/microsoft/TypeScript/issues/14877
  addEventListener('fetch', handle(app, options))
}

export { handle, fire }
Repository: honojs/hono
File: src/adapter/service-worker/index.ts (TypeScript)

Code before:

/**
 * Registers a Hono app to handle fetch events in a service worker.
 * This sets up `addEventListener('fetch', handle(app, options))` for the provided app.
 *
 * @param app - The Hono application instance
 * @param options - Options for handling requests (fetch defaults to undefined)
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { fire } from 'hono/service-worker'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => c.text('Hi'))
 *
 * fire(app)
 * ```
 */
const fire = (
  app: Hono,
  options: HandleOptions = {
    fetch: undefined,
  }
): void => {
  // @ts-expect-error addEventListener is not typed well
  addEventListener('fetch', handle(app, options))
}

export { handle, fire }


Review comment (suggestion):
```suggestion
  // @ts-expect-error addEventListener is not typed well in ServiceWorker-like contexts, see: https://github.com/microsoft/TypeScript/issues/14877
```

Code after:

/**
 * Registers a Hono app to handle fetch events in a service worker.
 * This sets up `addEventListener('fetch', handle(app, options))` for the provided app.
 *
 * @param app - The Hono application instance
 * @param options - Options for handling requests (fetch defaults to undefined)
 * @example
 * ```ts
 * import { Hono } from 'hono'
 * import { fire } from 'hono/service-worker'
 *
 * const app = new Hono()
 *
 * app.get('/', (c) => c.text('Hi'))
 *
 * fire(app)
 * ```
 */
const fire = (
  app: Hono,
  options: HandleOptions = {
    fetch: undefined,
  }
): void => {
  // @ts-expect-error addEventListener is not typed well in ServiceWorker-like contexts, see: https://github.com/microsoft/TypeScript/issues/14877
  addEventListener('fetch', handle(app, options))
}

export { handle, fire }