Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Bunny Edge adapter and integrates it across build, exports, types, docs, an example, and test config. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
commit: |
|
Minimal reproduction (both on Bunny.net and Deno) import { serve } from "https://esm.sh/pr/h3js/srvx@182/bunny"
serve({
fetch(request) {
console.log("New request from:", request.ip)
return Response.json({ hello: "world!" });
},
}); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 36: The package export under the "." map contains an undocumented ESM
condition key "bunny" which Bunny Edge won't resolve; edit the export entry that
currently reads "bunny": "./dist/adapters/bunny.mjs" and either remove that
"bunny" condition entirely or replace it with a supported condition such as
"browser" or "worker" (e.g., use "browser": "./dist/adapters/bunny.mjs" or
"worker": "./dist/adapters/bunny.mjs") so Bunny/deno-compatible consumers can
auto-resolve, ensuring the package.json export map no longer contains the
unsupported "bunny" key.
In `@src/adapters/bunny.ts`:
- Around line 94-99: The code in the Bunny adapter is pulling Nitro-specific env
vars; update the port and hostname parsing in the constructor/initialization
(look for _parsedPort, port, hostname and uses of this.options.port /
this.options.hostname) to stop reading process.env.NITRO_PORT and
process.env.NITRO_HOST—only parse Number.parseInt(this.options.port ??
process.env.PORT ?? "") for port (fall back to 3000 when NaN) and use
this.options.hostname ?? process.env.HOST for hostname (no Nitro variables).
- Around line 101-112: The Deno fallback calls Deno.serve(...) but discards the
returned Deno.HttpServer so close() is a no-op; add a private field (e.g.,
denoServer) to the class, assign the result of Deno.serve({port, hostname},
this.fetch) to that field, and update the class close() method to call
denoServer.close() (guarded for undefined) so the Deno fallback supports
graceful shutdown; ensure the field is typed as Deno.HttpServer | undefined and
only used in the Deno fallback branch where this.fetch is passed to Deno.serve.
- Line 1: Remove the top-level import of node:process and instead import
resolvePortAndHost from ../_utils.ts; then replace any direct uses of
process.env (e.g., references to NITRO_PORT/NITRO_HOST or process.env.*) with a
single call to resolvePortAndHost(this.options) and destructure const { port,
hostname } = resolvePortAndHost(this.options); update the imports at the top to
include resolvePortAndHost and remove the node:process import so the adapter
uses the same safe env-resolution utility as the other adapters
(resolvePortAndHost) rather than direct process.env access.
- Around line 58-78: The code currently mutates the incoming Request by calling
Object.defineProperties inside the exported fetch wrapper; remove that mutation
and instead compute a separate context object (e.g., const context = { runtime:
{ name: "bunny", bunny: {} }, ip: /* derive from request.headers x-forwarded-for
or x-real-ip */ }) and pass that context into fetchHandler (or if fetchHandler
signature cannot change, create a new Request via new Request(request, {
headers: new Headers(request.headers) }) and add metadata into a header or
cookie before calling fetchHandler); update the fetch wrapper to stop modifying
Request.prototype and to pass the newly created context/header-wrapped Request
into fetchHandler accordingly.
🧹 Nitpick comments (1)
src/types.ts (1)
365-366:BunnyFetchHandleris identical toFetchHandler.The signature
(request: Request) => Response | Promise<Response>is the same as the existingFetchHandlertype (line 351). This is fine for consistency with the per-runtime handler type pattern (BunFetchHandler,DenoFetchHandler), but noting it in case the Bunny runtime's fetch handler actually receives additional arguments (like Bun'sserverparameter).
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 115-122: The ready() implementation currently resolves immediately
(in function ready()) which is incorrect for the Deno fallback path because the
Deno HTTP server may not be listening yet; update ready() to detect the Deno
serve path and await the server's listening completion (e.g., await
Deno.HttpServer.finished(server) or the equivalent listening event returned by
Deno.serve) before resolving so callers receive an accurate readiness signal,
while leaving the Bunny runtime path behavior unchanged; no changes to close()
are required.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
9-10:FastURLandFastResponseare just aliases to globals — clarify intent or drop them.These exports simply re-bind
URLandResponseto new names with no added behavior. If this is a Bunny SDK convention for CDN edge optimization, a brief doc comment would help future readers. Otherwise, they add surface area without value.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 45-49: BunnyServer is missing the optional readonly url property
declared by Server; add a getter readonly url?: string on the BunnyServer class
that returns the Deno.HttpServer listening address when the Deno fallback is
used (populate it in the serve() path after starting the Deno server), and
ensure other code uses this.url for logging; update the serve() method to set
the internal value used by the getter once the Deno server is listening so local
development logs can show the actual URL.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
95-100: Use the sharedresolvePortAndHostutility instead of duplicating port/hostname resolution logic.The bunny adapter duplicates logic that node, deno, and bun adapters all standardize via
resolvePortAndHostfrom_utils.ts. This duplication risks divergence if port/hostname handling changes elsewhere. The refactor is compatible with the Deno fallback path—resolvePortAndHostusesglobalThis.process?.envwith optional chaining, which is actually more Deno-compatible than the directprocessimport. As a bonus, you'll gain port range validation (0-65535).Proposed fix
+import { resolvePortAndHost } from "../_utils.ts"; ... - const _parsedPort = - typeof this.options.port === "number" - ? this.options.port - : Number.parseInt(this.options.port ?? process.env.PORT ?? ""); - const port = !Number.isNaN(_parsedPort) ? _parsedPort : 3000; - const hostname = this.options.hostname || process.env.HOST; + const { port, hostname } = resolvePortAndHost(this.options);Then remove the
node:processimport sinceresolvePortAndHostaccesses env variables safely via optional chaining onglobalThis.process.
src/adapters/bunny.ts
Outdated
| class BunnyServer implements Server<BunnyFetchHandler> { | ||
| readonly runtime = "bunny"; | ||
| readonly options: Server["options"]; | ||
| readonly fetch: BunnyFetchHandler; | ||
| private _denoServer?: Deno.HttpServer = undefined; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the Server interface definition
rg -n "interface Server" src/ -A 10Repository: h3js/srvx
Length of output: 1871
🏁 Script executed:
# Check how other adapters set the url property
rg -n "this\.url\s*=" src/adapters/*.ts -B 3 -A 2Repository: h3js/srvx
Length of output: 35
🏁 Script executed:
# Verify current BunnyServer implementation
cat -n src/adapters/bunny.ts | head -80Repository: h3js/srvx
Length of output: 2918
🏁 Script executed:
# Get the full Server interface definition
cat -n src/types.ts | sed -n '182,230p'Repository: h3js/srvx
Length of output: 1279
🏁 Script executed:
# Search for url property in Server interface more directly
rg -n "url" src/types.ts -B 2 -A 2Repository: h3js/srvx
Length of output: 334
🏁 Script executed:
# Search for how other adapters handle url - broader search
rg -n "\.url\s*=" src/adapters/ -B 3 -A 2Repository: h3js/srvx
Length of output: 778
🏁 Script executed:
# Also check if BunnyServer sets url anywhere in the file
cat -n src/adapters/bunny.ts | grep -i "url"Repository: h3js/srvx
Length of output: 110
🏁 Script executed:
# Check how other adapters implement the Server interface
rg -n "class.*Server.*implements.*Server" src/adapters/ -A 20Repository: h3js/srvx
Length of output: 10363
🏁 Script executed:
# Search specifically for url property in other adapter classes
rg -n "readonly url|this\.url" src/adapters/node.ts src/adapters/deno.ts src/adapters/bun.ts -B 2 -A 2Repository: h3js/srvx
Length of output: 921
🏁 Script executed:
# Find where other adapters set the url property
rg -n "this\.url\s*=" src/adapters/ -B 5 -A 2Repository: h3js/srvx
Length of output: 617
🏁 Script executed:
# Check BunnyServer.serve() method implementation
cat -n src/adapters/bunny.ts | sed -n '80,150p'Repository: h3js/srvx
Length of output: 1685
🏁 Script executed:
# Search more comprehensively for url property assignment in adapter classes
rg -n "url\s*=" src/adapters/*.ts -B 3 -A 1 | head -100Repository: h3js/srvx
Length of output: 35
🏁 Script executed:
# Check node.ts serve/listen methods for url setting
cat -n src/adapters/node.ts | sed -n '100,160p'Repository: h3js/srvx
Length of output: 2301
🏁 Script executed:
# Check deno.ts serve method for url setting
cat -n src/adapters/deno.ts | sed -n '60,120p'Repository: h3js/srvx
Length of output: 2136
Add missing url property to BunnyServer for the Deno fallback path.
The Server interface declares readonly url?: string, which other adapters implement as getters to expose the listening address. BunnyServer doesn't implement this, missing an opportunity to log the URL during local development when using the Deno fallback.
Proposed fix
class BunnyServer implements Server<BunnyFetchHandler> {
readonly runtime = "bunny";
readonly options: Server["options"];
readonly fetch: BunnyFetchHandler;
+ url?: string;
private _denoServer?: Deno.HttpServer = undefined;Then in serve(), after starting the Deno server:
this._denoServer = Deno.serve(
{
port,
hostname,
},
this.fetch,
);
+ const h = hostname || "localhost";
+ this.url = `http://${h}:${port}`;🤖 Prompt for AI Agents
In `@src/adapters/bunny.ts` around lines 45 - 49, BunnyServer is missing the
optional readonly url property declared by Server; add a getter readonly url?:
string on the BunnyServer class that returns the Deno.HttpServer listening
address when the Deno fallback is used (populate it in the serve() path after
starting the Deno server), and ensure other code uses this.url for logging;
update the serve() method to set the internal value used by the getter once the
Deno server is listening so local development logs can show the actual URL.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 83-116: The serve() method can be invoked twice (auto-called when
options.manual is false and again manually), causing duplicate Bunny.v1.serve
registrations or multiple Deno.serve instances; add a guard flag (e.g.,
this._serving or this._served) and check it at the top of serve() to return
early if already serving, set the flag when starting the server, and only assign
this._denoServer after creating the Deno server; ensure you reference serve(),
options.manual, this._denoServer and Bunny.v1.serve when implementing the guard
so you don't re-register handlers or leak previous Deno servers.
- Around line 118-123: The ready() implementation incorrectly awaits
this._denoServer.finished (Deno.HttpServer.finished) which resolves only when
the server closes, causing callers to hang until shutdown; change ready() to
resolve immediately instead of waiting on finished—i.e., if this._denoServer
exists, return Promise.resolve(this) (matching the Deno.serve() behavior that
starts listening synchronously) or alternatively use an onListen-like readiness
signal if you switch to Deno.serve() with an onListen callback; update the
ready() method (referencing ready(), this._denoServer, and the current use of
Deno.HttpServer.finished) so callers get an immediate resolved promise when the
server is ready to accept connections.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
9-10: Clarify the purpose ofFastURLandFastResponseexports.These are re-exports of global
URLandResponsewith no transformation. If they exist for Bunny Edge SDK compatibility, a brief doc comment would help future maintainers understand why they're needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 15-38: The onOriginResponse handler type in the Bunny.BunnySDKV1
declaration is incorrect; update the return type for onOriginResponse middleware
functions inside registerMiddlewares so each function signature returns
Promise<Response> (not Promise<Request> | Promise<Response> | undefined). Locate
the registerMiddlewares declaration and change the onOriginResponse array
element type to (ctx: { request: Request; response: Response }) =>
Promise<Response> to match `@bunny.net/edgescript-sdk` v0.12.1 and keep
onOriginRequest unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 41-42: The runtime guard in the top-level serve function is too
weak: it only checks typeof Bunny !== "undefined" but BunnyServer later
dereferences Bunny.v1.serve (see serve and class BunnyServer / method serve),
which will throw if v1 or v1.serve are missing; tighten the check to ensure
Bunny.v1 and Bunny.v1.serve exist (e.g., typeof Bunny !== "undefined" &&
Bunny.v1 && typeof Bunny.v1.serve === "function") or use optional chaining/null
checks inside BunnyServer.serve to safely access Bunny.v1.serve; apply the same
stronger guard where Bunny.v1.serve is referenced (the code around the
BunnyServer.serve implementation).
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
49-49:_denoServeris dead code — never assigned.Since the Deno fallback now routes through
serveDeno()at line 42,BunnyServeris only instantiated when the Bunny runtime is present. The_denoServerfield (line 49) is never assigned, making the shutdown check inclose()(lines 107–109) unreachable.Proposed cleanup
class BunnyServer implements Server { readonly runtime = "bunny"; readonly options: Server["options"]; readonly fetch: (request: Request) => MaybePromise<Response>; - private _denoServer?: Deno.HttpServer = undefined; private _started = false;async close(): Promise<void> { // Bunny runtime doesn't support closing the server - const promises = [this.#wait?.wait()]; - if (this._denoServer) { - promises.push(this._denoServer.shutdown()); - } - await Promise.all(promises); + await this.#wait?.wait(); }Also applies to: 104-111
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/adapters/bunny.ts`:
- Around line 148-150: The ready() method currently returns
Promise.resolve(this) and does not await the internal `#listeningPromise`, causing
callers (e.g., the Deno fallback which calls server.ready()) to get a false
ready signal; change ready() on the Server class to await this.#listeningPromise
(or return this.#listeningPromise.then(() => this)) so it only resolves when the
promise set by onListen in serve() completes, ensuring actual listening before
resolving.
🧹 Nitpick comments (1)
src/adapters/bunny.ts (1)
114-116: Dead code:this._denoServeris alwaysundefinedat this point.The
_startedguard (line 102) ensuresserve()executes only once. Since_denoServeris only assigned later at line 128, this branch can never be taken.Proposed fix — remove dead branch
if (!this.options.silent) { console.warn("[srvx] Bunny runtime not detected. Falling back to Deno for local use."); } - if (this._denoServer) { - return Promise.resolve(this.#listeningPromise).then(() => this); - } const onListenPromise = Promise.withResolvers<void>();
2 participants
Following other implementation from:
bunnypreset nitrojs/nitro#4025Summary by CodeRabbit
New Features
Documentation
Examples
Tests/Chores