feat(hono/context): add buffer returns #3813
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3813 +/- ##
=======================================
Coverage 91.71% 91.71%
=======================================
Files 160 160
Lines 10195 10195
Branches 2885 2850 -35
=======================================
Hits 9350 9350
Misses 844 844
Partials 1 1 ☔ View full report in Codecov by Sentry. |
askorupskyy
force-pushed
the
feat/allow-buffer-returns
branch
from
4ce76f9 to
27c8cb6
Compare
askorupskyy
force-pushed
the
feat/allow-buffer-returns
branch
from
a59e19c to
22a2251
Compare
|
Tested this PR with the sample repo, using 
|
src/context.ts
Outdated
| @@ -1,3 +1,5 @@ | |||
| import type { Buffer } from 'node:buffer' | |||
There was a problem hiding this comment.
Is this import type necessary? In my environment, it does not throw an error without this.
There was a problem hiding this comment.
I added this for Deno. Deno does not have a Buffer exposed to the global scope
There was a problem hiding this comment.
It is importing just types, but I think we should not use node:* modules in the core of hono. Actually, Buffer is not in Web Standards API.
There was a problem hiding this comment.
I think using node:buffer is fine here, especially as a Type. There's no way to polyfill types for specific platforms as far as I am aware (so Deno will continue to fail). In addition to that, node:buffer is either the default implementation for a lot of runtimes, such as Cloudflare Workers, Deno, etc, or is already defined as a global replicating Node.js API. I see no problem with using the type from Node, since all runtimes are pretty much using the same API as node.
This thing will not execute after TSC, so I think it's a good fix, at least for now. Better to have partial buffer support than none at all
There was a problem hiding this comment.
It might be a good option to have a helper function to convert from node:buffer to the web standard accepted body format (For example, ReadbleStream).
If you think it is a good idea, we can change the implementation of this PR.
Of course, if you think it is overhead, I think current impl is fine.
There was a problem hiding this comment.
Let me see if i can make this work with a uint8array
|
@yusukebe @EdamAme-x looks like
|
There was a problem hiding this comment.
This is unfolded BodyInit types.
type BodyInit = ReadableStream<any> | string | Blob | ArrayBufferView<ArrayBuffer | SharedArrayBuffer> | ArrayBuffer | FormData | URLSearchParams
If I added Uint8Array, _BodyInit was marked as extended.
So I think looks good to me.
type _BodyInit = ReadableStream<any> | string | Blob | ArrayBufferView<ArrayBuffer |
SharedArrayBuffer> | ArrayBuffer | FormData | URLSearchParams | Uint8Array
// ^^^^^^^^^^
type isExtended = _BodyInit extends BodyInit ? true : false
// ^^ true ^^
|
One thing I was wondering, is there a reason why |
|
@EdamAme-x first of all, |
Context
I needed to be able to return Buffers from my Hono app, and was not able to do so.
Resolves #3729 #3720
The author should do the following, if applicable
bun run format:fix && bun run lint:fixto format the code