feat: v1 api freeze#230
Conversation
Breaking-shaped API stabilization landing together for v1: - F8: wire `errorPlugin` on the Bun and Deno adapters so the `error` option runs on every adapter; rename the docs section `onError` -> `error`. - F9: drop the unimplemented `gracefulShutdown.forceTimeout` type; honor `gracefulTimeout: 0` (use nullish check, not `||`); document `gracefulShutdown` (seconds, default 5, `SERVER_SHUTDOWN_TIMEOUT`). - F10: standardize `Server.serve()` on `Promise<Server<Handler>>`; void-returning adapters now `Promise.resolve(this)`. - F11: drop `@experimental` from `toFetchHandler` / `fetchNodeHandler` (frozen at v1); `srvx/tracing` stays experimental. - F17: `NodeResponse` throws (like native `Response`) for a non-null body with a null-body status (101/204/205/304). - F26/F18: Bun/Deno `serve()` never throws; listen errors (EADDRINUSE) surface via `ready()`, matching Node. If `ready()` is never awaited, the error is re-surfaced as an unhandled rejection instead of leaving a dead server. - F51: lazily initialize `request.context` once in shared code so `request.context.x = ...` works everywhere. - F52: remove the unused `netlify` / `stormkit` / `vercel` runtime context keys. - D6: keep `ServerPlugin` return type sync-only (`void`). Adds node-adapter tests for each behavioral change. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Warning Review limit reached
Next review available in: 48 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
✨ 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: |
pi0x
left a comment
pi0x
left a comment
There was a problem hiding this comment.
Independent review — v1 API freeze (PR #230)
Verdict: Approve (with one minor follow-up). Every locked hard spec is correctly implemented and I verified each one end-to-end (unit tests + manual EADDRINUSE/null-body repros run on Node, Bun and Deno). Full suite is green: 923 passed | 35 skipped, no type errors. The single failing test in a fresh checkout (test/loader.test.ts) is purely environmental — it needs a built dist/ for the import("srvx/node") self-import and passes after pnpm build; loader.ts is untouched by this PR.
Per-finding compliance checklist
| Finding | Status | Evidence |
|---|---|---|
F8/D2 — option stays error, actually wired on every adapter |
PASS | errorPlugin(this) runs in node/bun/deno/cloudflare/aws-lambda/generic/bunny/service-worker, always before wrapFetch. Verified a thrown handler is caught and mapped (500 "handled: boom"). Docs 5.options.md rewritten ### onError → ### error; body-limit cross-ref updated. (The remaining onError at 5.options.md:263 is Deno's native deno.onError option — correct.) |
F9/D3 — seconds, 0 honoured, forceTimeout deleted, documented |
PASS* | _plugins.ts:27 uses == null not ` |
F10 — serve(): Promise<Server<Handler>> |
PASS | types.ts:280 narrowed; every void adapter returns Promise.resolve(this) / Promise.resolve().then(() => this). |
F11/D5 — drop @experimental on toFetchHandler/fetchNodeHandler; tracing stays experimental |
PASS | Tags removed from _node/adapter.ts + _node/web/fetch.ts; @experimental remains only on src/tracing.ts. |
D6 — ServerPlugin stays sync void |
PASS | types.ts:47 (server: Server) => void, not widened. |
F17 — NodeResponse throws for body + null-body status |
PASS | Throws for 101/204/205/304 whenever body != null (covers strings and streams). Matches native Response (confirmed empty-string and stream bodies both throw natively). |
F18 — unawaited ready() still surfaces listen error |
PASS | reportUnhandledListenError re-surfaces via a deferred unhandled rejection unless ready() claimed it. Repro: second server on a busy port, ready() never awaited → EADDRINUSE surfaced loudly (Node exits 1; Bun prints), not swallowed. |
F26 — serve() never throws, ready() rejects (Bun+Deno) |
PASS | Bun.serve/Deno.serve wrapped; verified on both runtimes: serve() did not throw synchronously, ready() rejected (EADDRINUSE / AddrInUse). No spurious unhandled rejection when ready() is awaited. Knock-on checked: CLI (cli/serve.ts:81) and all docs/examples read server.url only after await ready() — no sync server.url regressions. |
F51 — request.context lazy-init in shared code |
PASS | ??= {} lives once in wrapFetch (both branches), which every adapter routes through. req.context.user = "alice" works (verified); native Bun/Deno Requests are extensible so assignment succeeds there too. |
F52 — remove netlify/stormkit/vercel any-typed keys |
PASS | Removed from ServerRuntimeContext; no dangling references in src/. |
Findings
Minor — SERVER_SHUTDOWN_TIMEOUT=0 env var is silently ignored (docs/behavior mismatch). src/_plugins.ts:28 — Number.parseInt(process.env.SERVER_SHUTDOWN_TIMEOUT || "") || 5. The || 5 is the exact falsy bug F9 fixed for the option, still present on the env path: SERVER_SHUTDOWN_TIMEOUT=0 resolves to 5, not 0. The docs this PR adds say the timeout "can also be set via the SERVER_SHUTDOWN_TIMEOUT environment variable" and "Set it to 0 to force close immediately", so a user setting 0 via env gets a 5s wait instead. Suggest const env = process.env.SERVER_SHUTDOWN_TIMEOUT; const fallback = env == null || env === "" ? 5 : Number.parseInt(env); (or apply the same ?? / == null guard). Not blocking — the primary option path is correct.
Info — PR branch is stale. Base is f2edab1; main is now d8523dc (#225/#227/#228/#234 landed since). Everything applies cleanly and tests pass, but a rebase before merge is advisable so CI runs against current main.
Info (out of scope, flagging for the freeze) — docs/1.guide/3.server.md:40-48 still documents server.addr / server.port, which don't exist on Server or any adapter. That's F54 (Docs scope, not this PR), but it's semver-adjacent doc debt worth clearing before the freeze tag.
Nice touch: the added test/v1-api-freeze.test.ts covers error-wiring, lazy context, promise return, EADDRINUSE-via-ready, null-body throw, and the gracefulTimeout: 0 regression directly — good behavioral (not just type) coverage that pins these decisions.
2 participants
API stabilization batch for v1. All breaking-shaped changes land together.
errorPluginon the Bun and Deno adapters so theerroroption runs on every adapter; docs section renamedonError->errorto match the real option.gracefulShutdown.forceTimeouttype; fix the falsy check sogracefulTimeout: 0is honored; documentgracefulShutdown(unit is seconds, default5,SERVER_SHUTDOWN_TIMEOUTenv var).Server.serve()onPromise<Server<Handler>>; adapters that returnedvoidnow returnPromise.resolve(this).@experimentalfromtoFetchHandler/fetchNodeHandler(stable/frozen at v1);srvx/tracingstays experimental.NodeResponsenow throws (like nativeResponse) for a non-null body with a null-body status (101/204/205/304) instead of emitting an illegal body + content-length.serve()never throws; listen errors (e.g. EADDRINUSE) surface viaready(), matching Node. Ifready()is never awaited, the error is re-surfaced as an unhandled rejection (Node's default handler reports it) rather than leaving a live process with a dead server.request.context(??= {}) once in shared code sorequest.context.x = ...works on every adapter.any-typednetlify/stormkit/vercelruntime context keys (removal after v1 would itself be breaking).ServerPluginreturn type sync-only (void), not widened toPromise<void> | void.Adds node-adapter tests for each behavioral change (
errorruns,gracefulTimeout: 0honored,serve()returns aPromise<Server>,NodeResponsethrows on null-body-status + body,ready()rejects on EADDRINUSE,request.contextworks). Bun/Deno-only behaviors are covered by the existing bun/deno suites.Verified:
pnpm vitest run --exclude '**/{deno,bun}*.test.ts' test/(860 passed / 32 skipped),pnpm lint,pnpm typecheckall green.🤖 Generated with Claude Code