Repository navigation
Conversation
|
Updated 4:35 AM PT - Oct 18th, 2026
❌ @robobun, your commit 2db5879 has 2 failures in
🧪 To try this PR locally: bunx bun-pr 45466That installs a local version of the PR into your bun-45466 --bun |
|
Status: ready for review. The tests of this change pass on every lane. CI is red on tests that this change does not touch. How I reproduced it (bun 1.4.2 and main at 116dd64, empty files are enough): const router = new Bun.FileSystemRouter({ style: "nextjs", dir });
console.log(Object.keys(router.routes));
On this branch the declaration files are not routes, and the constructor does not throw. CI
PR: #45466 |
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 seconds. View limit details
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
WalkthroughBoth filesystem router implementations now skip TypeScript declaration files ending in ChangesRouter declaration-file filtering
Suggested reviewers: Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation |
|---|---|---|
| Linked Issues check |  |
Check skipped because no linked issues were found for this pull request. |
| Out of Scope Changes check | |
Check skipped because no linked issues were found for this pull request. |
| Title check | |
The title clearly and concisely describes the main change: both FileSystemRouter and bake no longer load TypeScript declaration files as routes. |
| Description check | |
The description explains the problem, the fix, scope, downsides, related work, and verification results. It does not use the exact template headings, but it provides the required change summary and ve… |
Full details: Description check
-
Explanation
The description explains the problem, the fix, scope, downsides, related work, and verification results. It does not use the exact template headings, but it provides the required change summary and verification details in equivalent sections.
✨ Finishing Touches: 💡 1
-
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Nothing blocking. The comments below are optional suggestions. There is no need to push a fix for them before merging.
Beyond the inline findings, I also checked the placement of both skips: in src/router/lib.rs the new check sits inside the matched-extension branch, so dotfiles and files with unlisted extensions take exactly the pre-existing path, and reload() goes through the same RouteLoader::load, so there is no second scan site to patch. In src/runtime/bake/FrameworkRouter.rs the skip runs only after the found check inside the non-empty-extensions block, matching the stated "*" behavior.
Extended reasoning...
The change adds a byte-exact .d.ts/.d.mts/.d.cts predicate in src/paths/lib.rs and calls it from the two route scanners (Bun.FileSystemRouter and bake FrameworkRouter) so declaration files are dropped after the extension list selects them, plus docs, JSDoc and tests. It touches no security-sensitive surface. It is an unflagged user-facing behavior change (routes like /about.d disappear even under an explicit fileExtensions: [".ts"]), and inline findings were posted, so a human should weigh the API-design call rather than this being approved automatically.
…outes Both route loaders compare only the last dot-suffix of a file name with the extension list. So `about.d.ts` is selected as `.ts` and becomes the route `/about.d`. Add `bun_paths::is_typescript_declaration` (`.d.ts`, `.d.mts`, `.d.cts`). `RouteLoader::load` and `FrameworkRouter::scan_inner` skip such a file after the extension list selects it. Next.js does not make a `.d.ts` file a page. A declaration file beside a dynamic page also has a route name with text after the `]`, for example `docs/[...slug].d`. The route-name parsers do not accept that name: `new Bun.FileSystemRouter()` throws "Catch-all route must be at the end of the path", and the bake scan reports "Parameters must take up the entire file name". The skip removes these failures for declaration files only. Other names with text after a `]` behave as before.
0fb60bb to
563e100
Compare
Related to #7210
Problem
Bun.FileSystemRouterand the bake pages scan load TypeScript declaration files as routes:about.d.tsbecomes/about.d.src/router/lib.rs:602,src/runtime/bake/router/mod.rs:622), so.d.tspasses as.ts.docs/[...slug].dis one the parser rejects, so the constructor throwsCatch-all route must be at the end of the path.Fix
.d.ts,.d.mtsor.d.ctsfile (bun_paths::is_typescript_declaration) after the extension list selects it. Bakeextensions: "*"keeps every file..d.tspages (ignore .d.ts files inside pages folder vercel/next.js#30728).fileExtensions: [".d.ts"]selects no file today, so no opt-in is lost.test/js/bun/util/filesystem_router.test.ts(3 new tests),test/bake/framework-router.test.ts(2 new). All 5 fail on main (116dd64, release) and pass on this branch.Background
fileExtensionsselects the page files of aFileSystemRouter. The default list has.ts.Bun.serve({ app })server. Only a custom framework lists.ts.RouteLoader::loadonly: the bake scan keeps the fault. Suffix matching (FileSystemRouter extensions with prefix (like .page.tsx) #5822) needs a maintainer decision.Downsides
/about.dis no longer a route, also underfileExtensions: [".ts"]..textgrows 256 bytes.]still fail:[...slug].test.tsxthrows,[id].x.tsxtakes/7from a catch-all page.Notes
Repro (empty files are enough, 1.4.2 and main at 116dd64)
Bake, with the test-only
FrameworkRouterclass (nextjs-pages):about.d.tsis the page/about.d, and[id].d.tsfails the scan withParameters must take up the entire file name.What is specific to declaration files, and what is not
/about.d,/index.d) is specific to declaration files. It is the case that Add a way to exclude extensions toBun.FileSystemRouter#7210 reports.Pattern::init_maybe_hashandPattern::validateinsrc/router/lib.rs). The parser does the same for every name with text after a]. This change removes those failures for declaration files only, because the files are no longer loaded./users/7by the route/users/[id].dis the off-by-one that FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 corrects for every name (the open matcher PR for Bun.FileSystemRouter router match unknown behaviour #12206). With FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 alone, the file matches/users/7/d. This change removes the route in both cases, and the new test asserts both URLs.docs/[...slug].test.tsx,.d.tsxand.d.css.tsbeside the page make the constructor throw.users/[id].x.tsxtakes/users/7fromusers/[...rest].tsx. In bake,[id].test.tsxfails the scan.Bun.FileSystemRouterreads it in four different ways. The skip is necessary under each rule: with the Next.js rule,docs/[...slug].d.tswould be the static route/docs/[...slug].d.Bun.FileSystemRouter. The self-review asked for one. Bun.FileSystemRouter router match unknown behaviour #12206 and FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680 hold the current record of these names.Bake scope
.ts,.mtsor.cts. That is the default list of a custom framework (src/runtime/bake/config/framework.rs:496). The built-in React framework lists.tsxand.jsxand never had the fault.appandbun build --appare experimental (canary build, debug build, orBUN_FEATURE_FLAG_EXPERIMENTAL_BAKE=1).[id].d.tson main, per entry point: the dev server prints"routes/[id].d.ts" is not a valid routeand continues. The test-only class throws anAggregateError.bun build --appaborts on each route name that the scan rejects. bake: report route scan errors instead of aborting, and fail bun build --app on them #42836 replaces that abort, and this change does not touch it.extensions: []parses to the same empty list as"*", on main and on this branch. So declaration files stay pages there. The self-review asked for a tracking issue on what[]means. It is not filed.Landing order and related PRs
[id].d.tsthat expects the route/[id].d. Git reports no conflict, but the row fails with this change. The PR that lands second removes that row (and the[id].d.tsbullet in the Downsides of FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680).RouteLoader::load. The PR that lands second keeps the skip in its rebase.test/bake/framework-router.test.ts. The new bake tests are abovediscovers from filesystem paths, not at the end of the file, to keep those diffs applicable.Sites with a similar test that this change leaves alone
src/resolver/resolver.rsis_type_only_tsconfig_path: case-insensitive, pinned by its tests.src/runtime/cli/pm_diff_normalize.rskind_for: the same three suffixes today, for a different rule (a file that holds no statements). With a shared helper, a change made for the routers would also changebun pm diff, and no test covers that branch.src/lint/eslint/rules/no_shadow.rs: a port of ESLint'sisDeclareInDTSFile.src/sema/resolve.rsSUPPORTED_DECLARATION_EXTENSIONS: the type checker's table. It follows TypeScript and also needs the matched suffix.src/runtime/cli/run_command.rscompletions: acontainstest.x.d.tsx,x.d.css.tsandx.D.tsstay routes. Next.js testsendsWith(".d.ts")only.Measurements
Release builds of main at 116dd64 (A) and of this change on top of it (B), same checkout path. Instruction counts are user-space instructions of the main thread from a hardware counter (
perf_event_open), read around the measured loop throughbun:ffi. The container has noperf,valgrind,straceorbloaty.FileSystemRouterload, per page file (.tsx)FileSystemRouterload, per non-page file (.css)FileSystemRouterload, per declaration file.tsx).css)openat,readlink,close)Route::parsecalls per declaration file per loadmatch(), instructions per call (median of 7 runs).text(size -A, strippedbun).rodatareload(), median of 5 runs. The spread between runs is at most 0.008 %.Entry::kindcall in the changed loop. The predicate does not run for these files.RouteLoader::load(+81 bytes) andFrameworkRouter::scan_inner(+128 bytes). The predicate is inlined.Route::parsecalls:gdbcatchpoints and a breakpoint with ignore counts, 50 pages plus 20 declaration files, construct and 5 reloads. A tree without declaration files gives equal counts on both builds.Self-review
"*"cases of the bakeextensionsoption, the/users/7/dassertion, the per-entry wording inbake.d.ts, and one-line comments.kind_forinbun pm diffto the new helper. It was a refactor in an unrelated command.]inBun.FileSystemRouter, and a tracking issue for bakeextensions: []. Both cases are described above.Suites
filesystem_router.test.ts40 pass,test/bake/framework-router.test.ts37 pass.cargo test -p bun_paths --lib: 32 pass, with the newtypescript_declaration_suffixes. CI runs this crate in the Miri job.test/integration/bun-types/bun-types.test.ts: 22 pass.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/js/bun/util/filesystem_router.test.ts