fix(middleware/body-limit): set default duplex option for readable stream

[fzn0x]

Fixes #2627

In this case we do not need globalThis.Request anymore.

The author should do the following, if applicable

• Add tests
• Run tests
• bun run format:fix && bun run lint:fix to format the code

[fzn0x]

Tested without buildRequestInit default duplex is working!

[usualoma]

Hi @fzn0x
Very good! It works fine on all runtimes.

By the way, it seems that ec04a03 alone would solve the problem—do we need a734bf2? I don't see the need to add code to src/hono-base.ts for testing.

[fzn0x]

Hi @fzn0x Very good! It works fine on all runtimes.

By the way, it seems that ec04a03 alone would solve the problem—do we need a734bf2? I don't see the need to add code to src/hono-base.ts for testing.

Looks like we need to always add duplex by default from now though it's only one available value half for now, this approach was used by ky and some other famous Request wrapper libraries.

[fzn0x]

-- To avoid user manually adds duplex for their ReadableStream requests.

[usualoma]

@fzn0x Thanks for the answer!

Yes, I know how you feel.

I just think "ky" is a library for sending HTTP requests, which is different from a web application framework like "hono". I think it's great that "ky" makes it easy for users to send POST requests.

However, as far as "hono" is concerned, I think it is better to use the RequestInit as it is passed, and I don't think that what the duplex value of the Request object is is a value that "hono" should take care of. For duplex, it is better if the user explicitly specifies 'half' (or in the future 'full').

As a matter of fact, "node-server" does the same thing, but in hindsight I think this was also overkill.

https://github.com/usualoma/node-server/blob/a24d3ec1e45634d74b0e24bf215eb80e7606ad52/src/request.ts#L34-L38

That's my opinion, but it's not mandatory, so we might as well listen to @yusukebe's opinion too.

[usualoma]

Avoid @ts-expect-error

Rather than using @ts-expect-error, I prefer to avoid errors in the type definition.

diff --git a/src/middleware/body-limit/index.test.ts b/src/middleware/body-limit/index.test.ts
index 3da88445..60fae554 100644
--- a/src/middleware/body-limit/index.test.ts
+++ b/src/middleware/body-limit/index.test.ts
@@ -1,7 +1,7 @@
 import { Hono } from '../../hono'
 import { bodyLimit } from '.'
 
-const buildRequestInit = (init: RequestInit = {}): RequestInit => {
+const buildRequestInit = (init: RequestInit = {}): RequestInit & { duplex: 'half' } => {
   const headers: Record<string, string> = {
     'Content-Type': 'text/plain',
   }
@@ -13,7 +13,6 @@ const buildRequestInit = (init: RequestInit = {}): RequestInit => {
     headers,
     body: null,
     ...init,
-    // @ts-expect-error - duplex do not have any type yet
     duplex: 'half',
   }
 }
diff --git a/src/middleware/body-limit/index.ts b/src/middleware/body-limit/index.ts
index 282f4d61..7f636830 100644
--- a/src/middleware/body-limit/index.ts
+++ b/src/middleware/body-limit/index.ts
@@ -103,8 +103,8 @@ export const bodyLimit = (options: BodyLimitOptions): MiddlewareHandler => {
       },
     })
 
-    // @ts-expect-error - duplex do not have any type yet
-    c.req.raw = new Request(c.req.raw, { body: reader, duplex: 'half' })
+    const requestInit: RequestInit & { duplex: 'half' } = { body: reader, duplex: 'half' }
+    c.req.raw = new Request(c.req.raw, requestInit as RequestInit)
 
     await next()
 

[fzn0x]

For duplex, it is better if the user explicitly specifies 'half' (or in the future 'full').

Since the Request throws duplex errors once users sent ReadableStream requests without specifying it, I think this is makes sense.

Revert and types fix is applied!

[usualoma]

@fzn0x Thank you, LGTM 👍

[yusukebe]

LGTM!

[yusukebe]

@fzn0x @usualoma Thank you both! It's an interesting discussion. I fully agree. Looks good merging.