-
-
Notifications
You must be signed in to change notification settings - Fork 635
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
feat(hono/context): add buffer returns #3813
base: main
Are you sure you want to change the base?
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. |
4ce76f9
to
27c8cb6
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this import type
necessary? In my environment, it does not throw an error without this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this for Deno. Deno does not have a Buffer exposed to the global scope
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me see if i can make this work with a uint8array
@yusukebe @EdamAme-x looks like
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:fix
to format the code