Skip to content

Commit ef96ffe

Browse files
committed
fix(serve): a real public file beats any dynamic route, not just a catch-all
The disk check added in 0.2.174 only dropped `[...catch-all]` candidates, on the assumption that those are the routes broad enough to swallow an asset. That assumption holds for a shallow route tree and fails completely for an app whose routes start at the root: a forge with `[owner]/[repository]` claims every two-segment path and `[owner]` claims every one-segment path, so `/js/mermaid.js` and `/favicon.ico` both rendered a page and **the entire publicDir was unreachable**. Found by loading a page that dynamically imports `/js/mermaid.js` and getting 72KB of HTML where the 3.4MB bundle should have been - every mermaid diagram in that product silently not drawing, on the served boot as well as the dev server, with the fallback leaving the diagram source on screen so it read as "no diagram here" rather than as a failure. A file that really exists at exactly the requested path is a stronger signal than any route pattern, so it now wins over all of them. Nothing is dropped when publicDir has nothing there, which is what keeps `/owner/repo/tree/main/src/index.ts` resolving to the code browser. The resolver test mirrored the old rule and would have kept passing while the server did something else; it mirrors the new one now, with the forge's own route shape as the case.
1 parent 43a9693 commit ef96ffe

2 files changed

Lines changed: 38 additions & 14 deletions

File tree

packages/bun-plugin/src/serve.ts

Lines changed: 13 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2423,17 +2423,25 @@ function __stxOverlay(errs){
24232423
// the path carries a non-page file extension, drop catch-all candidates so
24242424
// the request falls through to publicDir (and then the real 404 page).
24252425
// Specific routes may still match (rare, but legitimate). stacksjs/stx#1841.
2426-
// A catch-all is dropped when publicDir really holds this file, rather
2427-
// than whenever the path merely looks like an asset: the extension is a
2428-
// guess about intent, and it refuses every legitimate catch-all page
2429-
// whose path carries a dot (see publicFileExists).
2426+
// A dynamic route is dropped when publicDir really holds this file,
2427+
// rather than whenever the path merely looks like an asset: the extension
2428+
// is a guess about intent, and it refuses every legitimate page whose
2429+
// path carries a dot (see publicFileExists).
2430+
//
2431+
// **Every** dynamic route, not only catch-alls. Restricting it to `[...x]`
2432+
// assumed a shallow route tree, and an app whose routes start at the root
2433+
// does not have one: a forge serving `[owner]/[repository]` claims every
2434+
// two-segment path and `[owner]` claims every one-segment path, so
2435+
// `/js/mermaid.js` and `/favicon.ico` both rendered a page and the whole
2436+
// publicDir was unreachable. A real file at exactly this path is a
2437+
// stronger signal than any route pattern.
24302438
const isAssetRequest = publicFileExists(`/${normalizedPath}`, publicDir)
24312439
const dynamicFiles = files
24322440
.filter((f) => {
24332441
const nf = f.replace(/^\.\//, '').replace(/\\/g, '/')
24342442
if (!nf.includes('['))
24352443
return false
2436-
if (isAssetRequest && /\[\.\.\./.test(nf))
2444+
if (isAssetRequest)
24372445
return false
24382446
return true
24392447
})

packages/bun-plugin/test/routes-catch-all.test.ts

Lines changed: 25 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -110,13 +110,13 @@ describe('isStaticAssetPath — catch-all never shadows a static asset (#1841)',
110110
expect(isStaticAssetPath(p)).toBe(false)
111111
})
112112

113-
// Mirrors getRoute: catch-all candidates are dropped when publicDir really
113+
// Mirrors getRoute: every dynamic candidate is dropped when publicDir really
114114
// holds the file, so the resolver returns null and publicDir serves it.
115115
// `exists` stands in for the disk.
116116
const resolveAsset = (target: string, files: string[], exists: string[] = []): string | null => {
117117
const asset = exists.includes(target)
118118
const candidates = files
119-
.filter(f => f.includes('[') && !(asset && /\[\.\.\./.test(f)))
119+
.filter(f => f.includes('[') && !asset)
120120
.sort((a, b) => routeSpecificity(b) - routeSpecificity(a))
121121
for (const file of candidates) {
122122
for (const regex of buildDynamicRouteRegexes(file.replace(/\.(stx|md|html)$/, ''))) {
@@ -138,18 +138,34 @@ describe('isStaticAssetPath — catch-all never shadows a static asset (#1841)',
138138
expect(resolveAsset('some/missing/page', files)).toBe('[...all].stx')
139139
})
140140

141-
it('a specific route may still match an extensioned path', () => {
141+
it('a specific route may still match an extensioned path publicDir lacks', () => {
142142
const files = ['[...all].stx', 'download/[file].stx']
143143
expect(resolveAsset('download/report.pdf', files)).toBe('download/[file].stx')
144144
})
145145

146146
/*
147-
* The regression the disk check exists for. An app whose catch-all serves
148-
* paths that carry extensions - a code browser rendering
149-
* `/owner/repo/tree/main/src/index.ts` - had every such page refused,
150-
* because the extension test called it an asset and dropped the only route
151-
* that could answer. Nothing is at that path in publicDir, so nothing
152-
* should be dropped.
147+
* A forge's routes start at the root: `[owner]` claims every one-segment
148+
* path and `[owner]/[repository]` every two-segment one. Restricting the
149+
* drop to catch-alls assumed a shallow tree, so `/favicon.ico` and
150+
* `/js/mermaid.js` rendered pages and the whole publicDir was unreachable.
151+
*/
152+
it('a plain dynamic route does not shadow a real public file either', () => {
153+
const files = ['[owner]/[repository].stx', '[owner].stx']
154+
expect(resolveAsset('js/mermaid.js', files, ['js/mermaid.js'])).toBeNull()
155+
expect(resolveAsset('favicon.ico', files, ['favicon.ico'])).toBeNull()
156+
})
157+
158+
it('but a dynamic route still answers when publicDir has nothing there', () => {
159+
const files = ['[owner]/[repository].stx', '[owner].stx']
160+
expect(resolveAsset('stacks/stacks', files)).toBe('[owner]/[repository].stx')
161+
})
162+
163+
/*
164+
* The other half of the disk check. An app whose catch-all serves paths that
165+
* carry extensions - a code browser rendering
166+
* `/owner/repo/tree/main/src/index.ts` - had every such page refused, because
167+
* the extension test called it an asset. Nothing is at that path in
168+
* publicDir, so nothing should be dropped.
153169
*/
154170
it('an extensioned path publicDir does not have still resolves to the catch-all', () => {
155171
const files = ['[...all].stx', 'foo/[id].stx']

0 commit comments

Comments
 (0)