Repository navigation
Conversation
… on-disk spelling
`pages/Blog/[Slug].tsx` is listed as `/Blog/[Slug]`, but `match("/Blog/x")`
returned null. Only `/blog/x` matched. `Pattern::match_` compares the static
segments of the lowercased route name to the URL byte for byte, and nothing
lowercases the URL.
`match_dynamic` now tries each dynamic route with its lowercased name, then
with its on-disk name. These are the two spellings the static route table
already keys. The retry clears `params` when it fails, so it adds no stale
params for the next candidate.
Contributor
|
Warning Review paused — included plan limit reachedKeep your review moving with free on-demand reviews.
On-demand reviews are free for the next 4 days.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Promotion and pricing detailsOn-demand reviews are free for the next 4 days. After that, they cost $0.25 per reviewed file. Review limit detailsOr wait 1 second for your next included review. Limit details: You’ve used all 10 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
Collaborator
Author
|
Status Reproduction, on bun 1.4.3 (Linux x64): import { mkdtempSync, mkdirSync, writeFileSync } from "node:fs";
import { tmpdir } from "node:os";
const d = mkdtempSync(tmpdir() + "/fsr-case-");
mkdirSync(d + "/Blog");
writeFileSync(d + "/Blog/[Slug].tsx", "export default 1;\n");
const r = new Bun.FileSystemRouter({ style: "nextjs", dir: d });
console.log(r.match("/Blog/x")?.name ?? null); // null, expected /Blog/[Slug]
console.log(r.match("/blog/x")?.name ?? null); // /Blog/[Slug]With this branch both lines print
|
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
pages/Blog/[Slug].tsxis listed byrouter.routesas/Blog/[Slug], butmatch("/Blog/x")returnsnull. Only/blog/xmatches. With a second route[section]/[id].tsx,/Users/1matches that route and notUsers/[id].tsx.Pattern::match_(src/router/lib.rs:1187). It compares the static segments of the lowercased route name to the URL byte for byte. Nothing lowercases the URL.append_route(:413) keysAbout.tsxbyAboutand byabout.Fix
Routes::match_dynamictries each dynamic route with its lowercased name. If the route has an uppercase letter, it then tries the on-disk name./BLOG/xstill does not match. A route tree with no uppercase letter takes the same path as before.paramswhen it fails, because a failedPattern::match_can leave params in the list (Bun.FileSystemRouter router match unknown behaviour #12206, open fix in FileSystemRouter: reset params on every failed candidate and fix single-char trailing segments #36680). Without the clear,/help/Settingsagainst[user]/Settings/[...rest].tsxplushelp/[...topic].tsxfails withassertion failed: name_slice.length > 0.test/js/bun/util/filesystem_router.test.ts. The new test fails on 1.4.3, and all 36 tests pass with the fix. Alsotest/regression/issue/18242.test.ts.Background
Route.nameis the route name as spelled on disk.Route.match_nameis the same string in lowercase, with the same length, so param offsets apply to both.[id],[...rest],[[...rest]]) live in a sorted list.match_dynamicreturns the first one that matches.paramsis one scratch list that all candidates in that walk share.Notes
Repro
/About/About/About/about/About/About/Blog/xnull/Blog/[Slug]/blog/x/Blog/[Slug]/Blog/[Slug]History. The Zig router had the same logic, so this is not a port regression. Commit 8d03e0c (2021) added the second hash table key for static routes with uppercase letters. Dynamic routes did not get an equivalent.
Probes, 1.4.3 against this branch. I ran 10 route trees and 55 URLs: index plus dynamic below an uppercase directory, uppercase only in the param name, query strings, percent-encoded paths,
Requestinputs, absolute URL strings, nested uppercase segments, a root catch-all next to an uppercase dynamic route, an uppercase file extension, and an optional catch-all. The only outputs that change are URLs in the on-disk spelling. Each one now gives the result that its lowercased spelling already gives. That includes the existing behavior that/blogmatchesBlog/[Slug].tsxwithSlug: ""./Blognow does the same.Why the retry cannot see stale params. Both attempts walk the same pattern structure. Only the static comparisons can differ. So the retry can match only if the first attempt failed at a static segment, and that exit clears
params.Why not full case-insensitive matching. It changes which route wins in trees with no uppercase letter. Example:
about.tsxplus[id].tsx. Today/ABOUTmatches/[id]withid: "ABOUT". That is a separate decision, and the change to make later is small: compare the static segments without case inPattern::match_and drop the retry.Related open PRs. #36680 rewrites
Pattern::match_so that a failed match truncatesparams. After it lands, theparams.clear()here is redundant and harmless. #33444 fixes the#fragmentbug on the samematch()entry point.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