Skip to content

Commit 73b0ba1

Browse files
committed
feat(serve): let a page set its own status, and stop 404ing on a trailing slash
definePageMeta({ status }) is read out of the source before anything runs, which is no use to a page that is only sometimes an error page. A page addressed by a dynamic segment cannot know whether the thing exists until it has looked, so it rendered "no such repository" under a 200 - which tells a crawler, a cache and an uptime check that the page is fine. setResponseStatus(code) is the missing half. The render cache now carries the status with the HTML, so a cached page answers what the first render did; without that a cached not-found page would have gone back to 200 on its second request, a bug that only appears once a page is popular enough to be cached. Separately, a trailing slash named a page nothing could match: /docs/ looked for docs//index.stx and matched no dynamic route either. It is the same page.
1 parent 9a4196b commit 73b0ba1

3 files changed

Lines changed: 220 additions & 2 deletions

File tree

docs/guide/script-types.md

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -50,10 +50,42 @@ definePageMeta({
5050

5151
- **Data fetching**: `fetch()`, database queries, file reads
5252
- **SEO**: `useSeoMeta()`, `definePageMeta()`
53+
- **The response status**: `setResponseStatus()`, when the page is the only thing that knows it
5354
- **Variable declarations**: All `const`, `let`, `var`, and `function` declarations are automatically available to `{{ }}` expressions
5455
- **Use secrets**: API keys, database credentials -- this code never reaches the browser
5556
- **Import server modules**: Bun APIs, Node.js builtins, server-only npm packages
5657

58+
### Setting the status from the page
59+
60+
`definePageMeta({ status: 404 })` is read out of the source before anything
61+
runs, which is right for a page that is always an error page. A page addressed
62+
by a dynamic segment -- a repository, a user, an order -- cannot know whether
63+
the thing exists until it has looked, so it needs to say so afterwards:
64+
65+
```html
66+
<!-- views/[owner]/[repository]/index.stx -->
67+
<script server>
68+
const repository = await findRepository(params.owner, params.repository)
69+
70+
if (!repository)
71+
setResponseStatus(404)
72+
</script>
73+
74+
@if (repository)
75+
<h1>{{ repository.name }}</h1>
76+
@else
77+
<h1>No such repository</h1>
78+
@endif
79+
```
80+
81+
Without it the page renders "no such repository" under a 200, which tells a
82+
crawler, a cache and an uptime check that the page is fine.
83+
84+
The last call wins, so a page can decide late. A status outside the HTTP range
85+
is ignored rather than thrown: it is not worth failing a page that has already
86+
rendered. The render cache carries the status with the HTML, so a cached page
87+
answers what the first render did.
88+
5789
### What You Cannot Do in `<script server>`
5890

5991
- Access `document`, `window`, `localStorage`, or any browser API

packages/bun-plugin/src/serve.ts

Lines changed: 38 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -937,6 +937,14 @@ export async function serve(options: ServeOptions): Promise<void> {
937937
interface HtmlCacheEntry {
938938
html: string
939939
signature: Map<string, number>
940+
/**
941+
* The status the render settled on, so a cached hit answers what the
942+
* uncached render did. A page that calls `setResponseStatus` decides its
943+
* status inside a server script the cache fast path skips, so without
944+
* this a "not found" page would be served as 200 the moment it was
945+
* rendered twice - the kind of bug that only appears under load.
946+
*/
947+
status: number
940948
}
941949
const htmlCache = new Map<string, HtmlCacheEntry>()
942950
// Opt-in because generic server scripts may read external state that no
@@ -1318,6 +1326,24 @@ export async function serve(options: ServeOptions): Promise<void> {
13181326
}
13191327
context.__stxServeContext = full
13201328
;(globalThis as { __stxServeContext?: ServeRequestContext }).__stxServeContext = full
1329+
1330+
// The status, decided while rendering rather than declared in the source.
1331+
//
1332+
// `definePageMeta({ status })` is read out of the file before anything
1333+
// runs, which is right for a page that is always an error page and no use
1334+
// to a page that only sometimes is. A page addressed by a dynamic segment
1335+
// - a repository, a user, an order - cannot know whether the thing exists
1336+
// until it has looked, and until now it had no way to say so: it rendered
1337+
// "no such repository" under a 200, which tells a crawler, a cache and a
1338+
// monitor that the page is fine.
1339+
//
1340+
// Last call wins, so a page can decide late. Anything outside the HTTP
1341+
// range is ignored rather than thrown, because a status is not worth
1342+
// failing a rendered page over.
1343+
context.setResponseStatus = (status: number): void => {
1344+
if (Number.isInteger(status) && status >= 100 && status <= 599)
1345+
full.responseStatus = status
1346+
}
13211347
}
13221348

13231349
/** Render cache must vary by locale/host/cookies — same `.stx` file can serve different `t()`/host/cookie-gated output. */
@@ -1754,8 +1780,11 @@ export async function serve(options: ServeOptions): Promise<void> {
17541780
if (ENABLE_HTML_CACHE && !skipCacheHint) {
17551781
const cacheKey = htmlCacheKey(filePath, reqCtx)
17561782
const cachedEntry = htmlCache.get(cacheKey)
1757-
if (cachedEntry && await templateSignatureFresh(cachedEntry.signature))
1783+
if (cachedEntry && await templateSignatureFresh(cachedEntry.signature)) {
1784+
if (reqCtx)
1785+
reqCtx.responseStatus = cachedEntry.status
17581786
return cachedEntry.html
1787+
}
17591788
}
17601789

17611790
// Extract server script bodies for variable extraction, and remove only
@@ -1880,7 +1909,7 @@ export async function serve(options: ServeOptions): Promise<void> {
18801909
&& isRenderableCacheCandidate(output)
18811910
) {
18821911
const signature = await buildTemplateSignature(filePath, dependencies)
1883-
htmlCache.set(htmlCacheKey(filePath, reqCtx), { html: output, signature })
1912+
htmlCache.set(htmlCacheKey(filePath, reqCtx), { html: output, signature, status: reqCtx?.responseStatus ?? 200 })
18841913
}
18851914

18861915
return output
@@ -1897,6 +1926,13 @@ export async function serve(options: ServeOptions): Promise<void> {
18971926
// Normalize the request path
18981927
let normalizedPath = requestPath.startsWith('/') ? requestPath.slice(1) : requestPath
18991928

1929+
// A trailing slash names the same page. Without this, `/docs/` looked for
1930+
// `docs//index.stx` and matched no dynamic regex either, so a link somebody
1931+
// wrote with a slash on the end, or a browser that added one, 404'd on a
1932+
// page that plainly exists. The root is already the empty string here, so
1933+
// there is nothing to strip off it.
1934+
normalizedPath = normalizedPath.replace(/\/+$/, '')
1935+
19001936
// Try to find matching file with various strategies
19011937
const possibleFiles: string[] = []
19021938

Lines changed: 150 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,150 @@
1+
import { afterAll, beforeAll, describe, expect, it, setDefaultTimeout } from 'bun:test'
2+
import { mkdtemp, rm } from 'node:fs/promises'
3+
import { tmpdir } from 'node:os'
4+
import path from 'node:path'
5+
6+
/**
7+
* A page deciding its own status while it renders.
8+
*
9+
* `definePageMeta({ status })` is read out of the source before anything runs,
10+
* which is right for a page that is always an error page and no use to a page
11+
* that is only sometimes one. A page addressed by a dynamic segment - a
12+
* repository, a user, an order - cannot know whether the thing exists until it
13+
* has looked, and had no way to say so afterwards: it rendered "no such
14+
* repository" under a 200, which tells a crawler, a cache and an uptime check
15+
* that the page is fine.
16+
*
17+
* `setResponseStatus(code)` is that missing half, and the render cache carries
18+
* the status with the HTML so a second request answers what the first did.
19+
*/
20+
21+
setDefaultTimeout(60_000)
22+
23+
const PORT = 43_100 + (process.pid % 400)
24+
const BASE = `http://localhost:${PORT}`
25+
const SERVE_SRC = path.join(import.meta.dir, '..', 'src', 'serve.ts')
26+
27+
let dir: string
28+
let proc: ReturnType<typeof Bun.spawn> | null = null
29+
30+
beforeAll(async () => {
31+
dir = await mkdtemp(path.join(tmpdir(), 'stx-serve-status-'))
32+
33+
// The case this exists for: one file, two answers, and only the render knows
34+
// which. `known` is a thing that exists; anything else is not.
35+
await Bun.write(path.join(dir, 'views', 'thing', '[id].stx'), `<script server>
36+
const found = id === 'known'
37+
if (!found)
38+
setResponseStatus(404)
39+
</script>
40+
<main>{{ found ? 'here it is' : 'no such thing' }}</main>
41+
`)
42+
43+
// A static page, to prove the API is not dynamic-route-only.
44+
await Bun.write(path.join(dir, 'views', 'gone.stx'), `<script server>
45+
setResponseStatus(410)
46+
</script>
47+
<main>gone</main>
48+
`)
49+
50+
// Out of range, and the wrong type. Neither is worth failing a page that has
51+
// already rendered, so both are ignored and the page answers 200.
52+
await Bun.write(path.join(dir, 'views', 'nonsense.stx'), `<script server>
53+
setResponseStatus(999)
54+
setResponseStatus(-1)
55+
</script>
56+
<main>still fine</main>
57+
`)
58+
59+
// Last call wins, so a page can look, decide, then change its mind.
60+
await Bun.write(path.join(dir, 'views', 'reconsidered.stx'), `<script server>
61+
setResponseStatus(404)
62+
setResponseStatus(200)
63+
</script>
64+
<main>found after all</main>
65+
`)
66+
67+
await Bun.write(path.join(dir, 'driver.ts'), `import { serve } from ${JSON.stringify(SERVE_SRC)}
68+
69+
serve({
70+
patterns: ['views'],
71+
port: ${PORT},
72+
renderCache: true,
73+
})
74+
`)
75+
76+
proc = Bun.spawn(['bun', 'driver.ts'], { cwd: dir, stdout: 'pipe', stderr: 'pipe' })
77+
78+
const deadline = Date.now() + 30_000
79+
while (true) {
80+
try {
81+
await fetch(`${BASE}/definitely-not-a-page`)
82+
break
83+
}
84+
catch {
85+
if (Date.now() > deadline)
86+
throw new Error('serve() never came up')
87+
await Bun.sleep(120)
88+
}
89+
}
90+
})
91+
92+
afterAll(async () => {
93+
proc?.kill()
94+
await rm(dir, { recursive: true, force: true })
95+
})
96+
97+
describe('setResponseStatus', () => {
98+
it('lets one dynamic route answer 200 or 404 depending on what it found', async () => {
99+
const found = await fetch(`${BASE}/thing/known`)
100+
const missing = await fetch(`${BASE}/thing/anything-else`)
101+
102+
expect(found.status).toBe(200)
103+
expect(await found.text()).toContain('here it is')
104+
105+
expect(missing.status).toBe(404)
106+
expect(await missing.text()).toContain('no such thing')
107+
})
108+
109+
it('works on a static page too', async () => {
110+
expect((await fetch(`${BASE}/gone`)).status).toBe(410)
111+
})
112+
113+
it('ignores a status that is not one, rather than failing the page', async () => {
114+
const res = await fetch(`${BASE}/nonsense`)
115+
116+
expect(res.status).toBe(200)
117+
expect(await res.text()).toContain('still fine')
118+
})
119+
120+
it('takes the last call, so a page can decide late', async () => {
121+
expect((await fetch(`${BASE}/reconsidered`)).status).toBe(200)
122+
})
123+
124+
/**
125+
* A slash on the end names the same page. A link written with one, or a
126+
* browser that added one, used to 404 on a page that plainly exists.
127+
*/
128+
it('answers the same on a path with a trailing slash', async () => {
129+
const withSlash = await fetch(`${BASE}/thing/known/`)
130+
131+
expect(withSlash.status).toBe(200)
132+
expect(await withSlash.text()).toContain('here it is')
133+
134+
expect((await fetch(`${BASE}/gone/`)).status).toBe(410)
135+
})
136+
137+
/**
138+
* The one that would have shipped broken. The cache fast path returns before
139+
* any server script runs, so a cached "not found" page would have answered
140+
* 200 from its second request onward - a bug that only appears once a page
141+
* is popular enough to be cached.
142+
*/
143+
it('keeps the status when the render is served from cache', async () => {
144+
for (let attempt = 0; attempt < 3; attempt++)
145+
expect((await fetch(`${BASE}/thing/still-missing`)).status).toBe(404)
146+
147+
for (let attempt = 0; attempt < 3; attempt++)
148+
expect((await fetch(`${BASE}/thing/known`)).status).toBe(200)
149+
})
150+
})

0 commit comments

Comments
 (0)