bun-types: declare MatchedRoute.scriptSrc - #39278
Conversation
|
Warning Review limit reached
Next review available in: 9 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 (2)
Comment |
|
Updated 12:48 AM PT - Aug 16th, 2026
✅ @robobun, your commit 2a48b0e3a795f5685f426e6a0e1d9cdf8959f791 passed in 🧪 To try this PR locally: bunx bun-pr 39278That installs a local version of the PR into your bun-39278 --bun |
|
Status: ready for review. Current revision (2a48b0e) is the declaration plus two fixture lines; the Reproduced on bun 1.4.0: The |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — declares a runtime property that already exists, marked deprecated in favor of src.
What was reviewed:
- Confirmed
scriptSrcis a real getter on theMatchedRouteprototype (filesystem_router.classes.ts:86), backed by the samegetScriptSrcassrcand sharing its cache slot. - The
tsc()helper refactor preserves the existingBun.mmaptest's checked program and assertion semantics (three.toBe→ one.toEqualon the same values). - Fixture additions match neighboring
expectTypelines; no changes to unrelated members.
Extended reasoning...
Overview
Adds readonly scriptSrc: string (with @deprecated JSDoc pointing at src) to the MatchedRoute interface in packages/bun-types/bun.d.ts. The runtime already installs this getter on the prototype (src/runtime/api/filesystem_router.classes.ts:86-93), backed by the same getScriptSrc implementation as src and kept for old bun-framework-next compatibility. The test file extracts the existing tsc-spawn logic into a shared tsc() helper and adds a new case for MatchedRoute.scriptSrc; the fixture file gains two expectType<string> lines.
Security risks
None. This is a .d.ts type declaration change plus test-only code — no runtime behavior is affected.
Level of scrutiny
Low. Type declarations only; the runtime property already exists and is verified by cross-referencing the .classes.ts proto table. The test refactor is a straightforward extraction: the Bun.mmap case's checked source string is byte-identical, and the assertion changed from three separate expect().toBe() calls to one .toEqual({stdout: "", stderr: "", exitCode: 0}) — semantically equivalent, and arguably a better failure message since all three fields are shown at once.
Other factors
- The
@deprecatedannotation follows the same shape asWebSocket.URLelsewhere in the file, and correctly steers users tosrcrather than presentingscriptSrcas a supported second name. - The PR description explicitly scopes out the pre-existing
origin/querytype inaccuracies and theassetPrefixruntime gap (handled by #33433), which is the right call for a focused change. - Tests cover both the release-build fixture path (all
typeTestcases now exercisescriptSrcviafsrouter.ts) and the debug-build spawned-tsc path, satisfying the "test fails without the fix" requirement per the PR description's before/after counts.
Bun.FileSystemRouter's MatchedRoute prototype installs scriptSrc as a second name for src (filesystem_router.classes.ts), but the MatchedRoute interface in bun.d.ts did not declare it, so reading it failed to type-check. Declare it as a deprecated alias of src, and read both names in the fsrouter fixture that bun-types.test.ts type-checks.
6146e64 to
2a48b0e
Compare
There was a problem hiding this comment.
LGTM — one-line type declaration matching the runtime MatchedRoute proto table, plus fixture coverage. The inline nit is description-accuracy only and doesn't block.
Extended reasoning...
Overview
This PR adds readonly scriptSrc: string (marked @deprecated in favor of src) to the MatchedRoute interface in packages/bun-types/bun.d.ts, and adds two expectType<string> reads for src and scriptSrc to the existing test/integration/bun-types/fixture/fsrouter.ts fixture. Total: 5 lines of declaration + 3 lines of fixture.
I verified the runtime claim against src/runtime/api/filesystem_router.classes.ts:86-93: the MatchedRoute proto table installs both scriptSrc and src getters, both backed by getScriptSrc and sharing the scriptSrc cache slot, with a comment noting scriptSrc is kept for compatibility with old bun-framework-next versions. The declaration accurately mirrors what the runtime installs, and marking it @deprecated pointing at src is the right call given the proto-table comment.
Security risks
None. This is a pure .d.ts type-declaration change plus a type-check fixture; no compiled code is touched.
Level of scrutiny
Low. Per CLAUDE.md, edits to packages/bun-types/**/*.d.ts don't touch compiled code and are covered by the bun-types.test.ts type-check harness. The change is mechanical (declare a member that exists at runtime), follows the exact shape of an existing precedent (WebSocket.URL deprecated alias), and CI passed (Build #99051).
Other factors
The bug-hunting system flagged one nit: the PR description claims a bun-types.test.ts change (a Bun.FileSystemRouter case with a shared tsc-spawning helper) that isn't in the diff — only bun.d.ts and the fixture were committed. This is description-accuracy only; the shipped fixture coverage is sufficient under CLAUDE.md's explicit bun-types exception, and the fixture is exercised by every type-check case in bun-types.test.ts on release builds. The nit doesn't affect the correctness of what shipped.
1 participant
Problem
router.match(...)returns aMatchedRoutewhose prototype has ascriptSrcgetter next tosrc(src/runtime/api/filesystem_router.classes.ts:86-93; both are backed byget_script_srcinsrc/runtime/api/filesystem_router.rs:933and share one cache slot), somatch.scriptSrcis a real string at runtime: on bun 1.4.0,r.match("/").scriptSrcis"index.js"and=== r.match("/").src.interface MatchedRouteinpackages/bun-types/bun.d.ts:8400does not declare it, so code that reads it fails to compile witherror TS2339: Property 'scriptSrc' does not exist on type 'MatchedRoute'.*.classes.tsproto tables againstbun-types, not from a report; the alias has been installed and undeclared sinceBun.FileSystemRouterwas added (d21aee5). The same comparison showsFileSystemRouter.assetPrefixdeclared but missing at runtime; that half is being fixed by Bun.FileSystemRouter: expose the assetPrefix getter on the prototype #33433 (a runtime getter), so theassetPrefixdeclaration is left alone here. bun-types: declare FileSystemRouter.origin as string | null and MatchedRoute.query values as string | string[] #39282 fixes the return types oforiginandqueryin the same two declarations; the three PRs do not overlap, and this one merges cleanly with bun-types: declare FileSystemRouter.origin as string | null and MatchedRoute.query values as string | string[] #39282 in either order.Fix
readonly scriptSrc: stringonMatchedRoute, marked@deprecatedin favor ofsrc, in the same shape asWebSocket.URL(bun.d.ts:4656).bun-framework-nextversions, so the declaration points atsrcinstead of presenting it as a second supported name.test/integration/bun-types/fixture/fsrouter.tsreadssrcandscriptSrc. Every type-checking case intest/integration/bun-types/bun-types.test.ts(with and withoutlib.dom, no lib, tsgo) checks that fixture, which is what.github/workflows/bun-types.ymlruns on a release bun.bun.d.tshunk: 9 of the file's 15 cases fail, all with the TS2339 above (fsrouter.ts(13,27)). With it: 15 of 15 pass.skipIf(isDebug), and this file is verified with a release bun by design (CLAUDE.md'sbun-typesnote). Notsc-spawning case is added for debug builds: test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270 is replacing the one such block in the file with a whole-fixture run plus a lint allowing exactly one compiler spawn, so a new block would conflict with it and then fail the lint. The assertions live only in the fixture, which is the shape test(bun-types): replace the tsgo and Bun.mmap spawns with one whole-fixture tsc run, enforced by a lint #39270 asks types PRs to use, and this PR merges cleanly with it.Background
src/**/*.classes.tsfiles are the input tosrc/codegen/generate-classes.ts; eachprotoentry becomes a property of the generated prototype, so theMatchedRouteproto table is the list of members the runtime installs.packages/bun-typesis the published@types/bunsurface. It is written by hand, not generated from the class tables, which is how a member can exist at runtime without being declared (or the reverse, as withassetPrefix).test/integration/bun-types/bun-types.test.tspacksbun-typesand type-checksfixture/*.tsagainst it through the TypeScript language service in-process; those cases are skipped under debug builds because driving that API in a debug build is very slow.Earlier revision
The first push also refactored the
Bun.mmapblock inbun-types.test.tsinto a sharedtsc()helper and added aBun.FileSystemRoutercase that spawnedtscover a three-line program, so that a debug-build run of the file exercised the declaration. That hunk conflicted with #39270 (which deletes the block it refactored and lints the file down to one spawn site) and duplicated the fixture assertions, so it was dropped. The declaration is unchanged from that revision; the fixture lines moved up two lines so the file also merges cleanly with #39282.