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
[Bun] Add Bun runtime #8416
[Bun] Add Bun runtime #8416
Conversation
@@ -0,0 +1,13 @@ | |||
FROM oven/bun |
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.
Could you lock this down to a specific version please
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.
Done
|
||
const handlers = new Map<string, HandlerFn>(); | ||
handlers.set("/json", () => Response.json({ message: "Hello, World!" }, options)); | ||
handlers.set("/plaintext", () => new Response("Hello, World!", options)); |
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.
out of curiosity, shouldn't this be Response.text("Hello, World!", options)
instead? ... or better, what is Response.json
in there? 🤔
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.
out of curiosity, shouldn't this be Response.text("Hello, World!", options) instead?
Not really, new Response()
returns text/plain
as content type already, required by the /plaintext
scenario.
or better, what is Response.json in there?
Response.json
returns the object serialized and application/json
as content type, required by the /json
scenario.
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.
not in the docs and not standard though, maybe a comment would do
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 the documentation for Response.json
: https://developer.mozilla.org/en-US/docs/Web/API/Response/json_static
And I believe text/plain
is the default content type.
Do you think an extra comment is really needed?
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’ve missed that … my bad. No comment needed
const server = Bun.serve({ | ||
port: 8080, | ||
fetch(req: Request) { | ||
return handlers.get(new URL(req.url).pathname)?.call(req); |
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.
as a benchmark, I am not sure this it idiomatic or well performing + the call(req)
when functions are arrows seems off ..
fetch: (req: Request) => handlers.get(new URL(req.url).pathname)?.()
looks already better, but I am not sure that's how people really write routes in Bun neither .. if it's idiomatic benchrmaks we're after, this PR doesn't look like the best we can do ... thoughts?
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 agree, actually I'm refactoring my PR to simplify this part and not implementing anything other than what is strictly necessary. Just plain if
statements.
@marcbarbosa Let me know when you're done refactoring. This looks ready. |
b54c2b7
to
34c0927
Compare
@nbrady-techempower Done |
This needs parallelism to be potentially competitive with native languages — you can use Worker and enable reusePort. request.url already runs the URL parser so you’re running it twice here |
Initial (vanilla) implementation for Bun:
/json
and/plaintext
Reference: #7646