bun-types: declare FileSystemRouter.origin as string | null and MatchedRoute.query values as string | string[] - #39282
bun-types: declare FileSystemRouter.origin as string | null and MatchedRoute.query values as string | string[]#39282robobun wants to merge 2 commits into
Conversation
…ry values can be string[] The origin getter returns null when the router was constructed without an origin (or with an empty one), and a query string name that appears more than once is reported as an array of its values. Declare both the way the runtime behaves, document the query shape, and assert the declarations in the bun-types fixture.
|
Warning Review limit reached
Next review available in: 16 minutes Limit details: You’ve used all 5 included reviews currently available under your plan. 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: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
Comment |
|
Status: reproduced on the released bun 1.4.0 ( Current shape after self-review: Verification: |
There was a problem hiding this comment.
LGTM — declarations now match the runtime.
Checked get_origin in src/runtime/api/filesystem_router.rs (returns JSValue::NULL when unset) and JSC__JSObject__putRecord in bindings.cpp (builds a JSArray when valuesLen != 1) — both new types are accurate.
The new test case mirrors the existing Bun.mmap debug-build tsc case exactly, and utilities.ts is copied alongside fsrouter.ts so the relative import resolves.
Docs examples match the runtime probe output in the PR description; params deliberately left as Record<string, string> per the stated rationale.
Extended reasoning...
Overview
This PR widens two type declarations in packages/bun-types/bun.d.ts to match long-standing runtime behavior: FileSystemRouter.origin becomes string | null and MatchedRoute.query becomes Record<string, string | string[]>. It also updates docs/runtime/file-system-router.mdx with two new examples and the corrected reference type, tightens the fixture assertions in test/integration/bun-types/fixture/fsrouter.ts to use exact-type equality (.is<T>()), and adds a debug-build-friendly tsc case to bun-types.test.ts that follows the existing Bun.mmap pattern verbatim. No runtime code is touched.
Security risks
None. Pure .d.ts, docs, and test-fixture changes.
Level of scrutiny
Low. This is a docs/types correction with no compiled-code impact. I verified both runtime claims directly against source: get_origin at filesystem_router.rs:662 returns JSValue::NULL when this.origin is None, and putRecord at bindings.cpp:2808 sets a JSArray when more than one value is passed. The widened types are strictly more accurate than what they replace and match Next.js's own query typing.
Other factors
The new test case is a near-copy of the adjacent Bun.mmap block (same typeRoots wiring, same spawn-tsc shape, same stdout/stderr/exitCode assertion order). The fixture copies utilities.ts into the check dir so the ./utilities import resolves even though tsconfig.include lists only fsrouter.ts — tsc pulls it in transitively. The PR description documents that the fixture fails against the old declarations and passes against the new ones under both release and debug builds, and explicitly leaves params and the assetPrefix/scriptSrc gaps to their own PRs. The bug-hunting system found no issues.
There was a problem hiding this comment.
I reviewed this PR and didn't find any bugs. The declarations match the runtime (get_origin returns JSValue::NULL when unset; putRecord builds a JSArray for repeated names), the docs examples reproduce, and the fixture assertions pin the exact widened types. Deferring only because packages/bun-types/ has a CODEOWNERS entry and the widening is a user-visible TypeScript break — worth a human sign-off on the API-design call (widen the types vs. change the getter/query builder).
What was reviewed
get_origininsrc/runtime/api/filesystem_router.rsandJSC__JSObject__putRecordinbindings.cpp— both confirm the declared shapes.paramsdeliberately left asRecord<string, string>— catch-all is a joined string; the only array case is the duplicate-segment shape the PR argues is out of scope.- Fixture edit is additive around the existing
paramsline; no conflict with the neighboring #39278 append point.
Extended reasoning...
Overview
This PR touches three files: packages/bun-types/bun.d.ts (widens FileSystemRouter.origin to string | null and MatchedRoute.query to Record<string, string | string[]>, adds JSDoc), docs/runtime/file-system-router.mdx (matching reference-block type plus two new examples), and test/integration/bun-types/fixture/fsrouter.ts (exact-type assertions via expectType(...).is<T>() and an Array.isArray narrowing check). No runtime code changes.
Verification
I read the native sources the PR cites. get_origin (filesystem_router.rs:662) returns JSValue::NULL when this.origin is None, and the constructor only stores non-empty origins, so null is the actual return value for the no-origin / empty-origin case. create_query_object calls putRecord, and JSC__JSObject__putRecord (bindings.cpp:2808) sets a jsString when valuesLen == 1 and a JSArray otherwise — so repeated query names do produce arrays. The PR's decision to leave MatchedRoute.params as Record<string, string> is defensible: catch-all params are joined strings, and the duplicate-[a]/[a] array case is a route-authoring error Next.js rejects.
Security risks
None. Type declarations and documentation only; no code path, parsing, or validation is altered.
Level of scrutiny
Low-to-medium on correctness (mechanical, source-verified), but packages/bun-types/ is under a CODEOWNERS entry and widening a public declaration is a breaking change for downstream TypeScript consumers — existing router.origin.length or match.query.foo.toUpperCase() call sites will start failing tsc. That is the intended outcome (those call sites already throw at runtime), and it mirrors Next.js's ParsedUrlQuery typing, but the widen-vs-change-runtime trade-off is an API design decision the code owner should confirm.
Other factors
The fixture uses the strict .is<T>() form so a future accidental narrowing or further widening will fail the bun-types test. The docs additions were spot-checked against the runtime probe in the PR description. The bug-hunting pass surfaced no findings. No prior review from me on this PR.
Problem
bun-typesdeclaresFileSystemRouter.originasstring, but the getter returnsnullwhenever the router was constructed without anoriginor withorigin: ""(get_origininsrc/runtime/api/filesystem_router.rs:662; the constructor only stores a non-empty origin,:325).router.origin.lengthtype-checks and throws at runtime.bun-typesdeclaresMatchedRoute.queryasRecord<string, string>, but a query string name that appears more than once is reported as an array of its values:match("/?a=1&a=2&b=3").queryis{ a: ["1", "2"], b: "3" }(create_query_objectat:883hands every value for a name toputRecord, which builds aJSArraywhen there is more than one,src/jsc/bindings/bindings.cpp:2808).match.query.a.toUpperCase()type-checks and throws on a request the client controls.Bun.FileSystemRouter(d21aee5); the declarations have never matched them. Bun.FileSystemRouter router match unknown behaviour #12206 (mainly about the param-leak bug) also asks for the array case to be "properly defined".Fix
FileSystemRouter.originis declaredstring | null, with JSDoc saying when it isnull.MatchedRoute.queryis declaredRecord<string, string | string[]>, with JSDoc describing the array case and that route params are merged in (a param wins over a query entry of the same name, which is what the runtime does on purpose: see the Next.js reference inQueryStringMap::init_with_scanner,src/url/lib.rs:1050).docs/runtime/file-system-router.mdxgets the same type in its reference block plus two examples; every example was run against the released binary (output below).nullhas been the "no origin" value since the API shipped, and the array shape is how the router keeps every value of a repeated name (Next.js, whosequerythis mirrors, types it asstring | string[]as well). Returning""or dropping values would change what existing callers get back; widening the declarations only turns code that already throws at runtime into a type error.MatchedRoute.paramsstaysRecord<string, string>on purpose. A catch-all param is one joined string (rest: "a/b/c"), and the only ways to get an array there are the param-leak bug that FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 fixes (Bun.FileSystemRouter router match unknown behaviour #12206, Array instead of string inMatchedRoutewhen specific file structure #15554) and a route that names two segments the same ([a]/[a], which Next.js rejects outright). Route shapes are under the author's control; query strings are not, which is whyqueryis widened andparamsis not. The existing fixture line keeps pinningparamstoRecord<string, string>.styleis declaredstringand the runtime returns"nextjs", which is imprecise but does not let wrong code type-check, so it is not part of this fix; theassetPrefixgetter (Bun.FileSystemRouter: expose the assetPrefix getter on the prototype #33433) and thescriptSrcdeclaration (bun-types: declare MatchedRoute.scriptSrc #39278) are separate gaps owned by those PRs. This PR no longer touchesbun-types.test.ts, and the fixture edit leaves the line bun-types: declare MatchedRoute.scriptSrc #39278 appends after untouched, so it merges cleanly with bun-types: declare MatchedRoute.scriptSrc #39278 and with test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270 in either order (checked withgit merge-tree).test/integration/bun-types/bun-types.test.ts, run with a release bun as CLAUDE.md describes for.d.tschanges:fixture/fsrouter.tsnow asserts the exact types oforiginandqueryand narrows aqueryvalue withArray.isArray. Withpackages/at main, 9 of the 15 cases fail with the threeTS2344errors below; with this change all 15 pass. Under a debug build this file's type-checking cases are skipped on main today; with test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270's whole-fixture case checked out alongside,bun bd teston this file fails on the same three errors without the.d.tschange and passes with it.Background
Bun.FileSystemRouterresolves URL paths against a Next.js-stylepagesdirectory.match()returns aMatchedRoutewhoseparamsare the values of the route's dynamic segments and whosequeryis the parsed query string merged with those params. Both objects are built by the same native helper, which turns a name with one value into a string and a name with several values into an array.originis an optional constructor option used to buildMatchedRoute.src; the getter reports the configured value back.test/integration/bun-types/bun-types.test.tspackspackages/bun-types, installs it into a copy oftest/integration/bun-types/fixture/, and type-checks the fixture;fixture/*.tsfiles hold the assertions, withexpectType(x).is<T>()failing unless the type ofxis exactlyT. CI runs this file with a release bun (.github/workflows/bun-types.yml; Buildkite excludes it), which is where the fixture assertions are checked.Runtime probe (bun 1.4.0 release, unchanged by this PR)
paramsprobes:cat/[...rest].jsmatched against/cat/a/b/cgives{ rest: "a/b/c" };dup/[a]/[a].jsagainst/dup/x/ygives{ a: ["x", "y"] }(the duplicate-name shape discussed above).Fixture failure against the unchanged declarations
bun test test/integration/bun-types/bun-types.test.ts(release bun): 9 fail / 6 pass withpackages/at main, 15 pass with this change. Same three errors, then a pass, frombun bd teston this file when #39270'sbun-types.test.tsis checked out next to it.Earlier revision
The first push also added a
describe("Bun.FileSystemRouter")case tobun-types.test.tsthat spawnedtscoverfixture/fsrouter.tsso the file had a non-skipped case under debug builds. It duplicated what every release case already checks, and #39270 replaces that pattern with one whole-fixture case plus a lint holding the file at a single spawn site, which the extra case would have tripped once both landed. It was removed; the assertions only ever lived in the fixture.