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

Fixed "Missing Buffer type in the parameter of c.body" #3733

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

itztiva
Copy link

@itztiva itztiva commented Dec 6, 2024

Updated the "Data" type definitions in src/context.ts to include Buffer.

This fixes issues such as the following here

The author should do the following, if applicable

  • Add tests
  • Run tests
  • bun run format:fix && bun run lint:fix to format the code
  • Add TSDoc/JSDoc to document the code

@EdamAme-x
Copy link
Contributor

Some runtimes appear to fail because there is no built-in type called Buffer.

@usualoma
Copy link
Member

usualoma commented Dec 7, 2024

There may be historical reasons for this, but considering the current Hono, it might be better to simply use BodyInit.

diff --git a/src/context.ts b/src/context.ts
index 86124e00..10dea474 100644
--- a/src/context.ts
+++ b/src/context.ts
@@ -27,9 +27,9 @@ type HeaderRecord =
   | Record<string, string | string[]>
 
 /**
- * Data type can be a string, ArrayBuffer, or ReadableStream.
+ * Data type can be a string, ArrayBuffer, ReadableStream, or any type accepted by the Response constructor.
  */
-export type Data = string | ArrayBuffer | ReadableStream
+export type Data = BodyInit
 
 /**
  * Interface for the execution context in a web worker or similar environment.

@itztiva
Copy link
Author

itztiva commented Dec 7, 2024

I saw a couple issues in the past on this issue, not that many people use BodyInit and this is more useful for some people. Isn't there a way to solve the runtime issues? I've never used deno personally but I do believe there is a library you can import?

@itztiva
Copy link
Author

itztiva commented Dec 7, 2024

image
According to a recent issue about this this has solved it for them.

@yusukebe
Copy link
Member

yusukebe commented Dec 7, 2024

If you write the test for this PR, the current main should fail. So the following change is needed:

diff --git a/package.json b/package.json
index 669c2027..dc8ac997 100644
--- a/package.json
+++ b/package.json
@@ -630,7 +630,7 @@
     "@hono/node-server": "^1.13.5",
     "@types/glob": "^8.1.0",
     "@types/jsdom": "^21.1.7",
-    "@types/node": "20.11.4",
+    "@types/node": "^22.10.1",
     "@types/supertest": "^2.0.16",
     "@vitest/coverage-v8": "^2.0.5",
     "arg": "^5.0.2",
@@ -644,7 +644,7 @@
     "prettier": "^2.6.2",
     "publint": "^0.1.16",
     "supertest": "^6.3.4",
-    "typescript": "^5.3.3",
+    "typescript": "^5.7.2",
     "vite-plugin-fastly-js-compute": "^0.4.2",
     "vitest": "^2.0.5",
     "wrangler": "3.58.0",
diff --git a/tsconfig.json b/tsconfig.json
index a580d709..ba3eba46 100644
--- a/tsconfig.json
+++ b/tsconfig.json
@@ -1,6 +1,9 @@
 {
   "compilerOptions": {
-    "target": "ES2022",
+    "target": "ES2024",
+    "lib": [
+      "ES2024"
+    ],
     "declaration": true,
     "moduleResolution": "Bundler",
     "outDir": "./dist",
@@ -25,4 +28,4 @@
     "src/**/*.test.ts",
     "src/**/*.test.tsx"
   ],
-}
+}
\ No newline at end of file

With this change, the test actually fails, and it's okay. But to resolve that, we have to fix the many things not only this c.body matter:

CleanShot 2024-12-07 at 17 06 23@2x

So, I think this is not an easy thing.

@itztiva
Copy link
Author

itztiva commented Dec 7, 2024

I ran a test with the newest commit I just pushed, By switching Buffer to Uint8Array it solved all Deno issues and runtime issues in general. It also solves this issue ( tested for myself )

Copy link

codecov bot commented Dec 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.72%. Comparing base (50ff212) to head (bf834d8).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3733   +/-   ##
=======================================
  Coverage   91.72%   91.72%           
=======================================
  Files         159      159           
  Lines       10184    10184           
  Branches     2990     2990           
=======================================
  Hits         9341     9341           
  Misses        842      842           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@itztiva
Copy link
Author

itztiva commented Dec 8, 2024

Can this be reviewed? @usualoma @yusukebe

@yusukebe
Copy link
Member

@itztiva

As commented on here, if we add a new feature or a fix, we have to add tests basically.

If there is a problem like #3729, the test should fail with that condition. To do so, we may upgrade the TypeScript version and update the tsconfig.json. This is not an easy matter, and this PR does not solve it.

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

Successfully merging this pull request may close these issues.

4 participants