-
Notifications
You must be signed in to change notification settings - Fork 69
fix(website): make pages cacheable in the browser and stabilize the ETag #1130
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
6 commits
Select commit
Hold shift + click to select a range
599857c
fix(website): make pages cacheable in the browser and stabilize the ETag
vivek7405 023eaae
docs: note render determinism as a precondition for conditional GET
vivek7405 9c5ff22
fix(website): keep the copy-cmd description inside the component
vivek7405 4302d93
fix(website): stop the hidden copy hint leaking into a manual selection
vivek7405 1352acc
fix(website): replace the hidden copy hint with a title attribute
vivek7405 26012ef
test(website): prove the caching contract end to end
vivek7405 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
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
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| /** | ||
| * The caching contract end to end (#1127): the served page must carry a | ||
| * browser-reusable Cache-Control, an ETag, and answer a matching | ||
| * If-None-Match with an empty 304. | ||
| * | ||
| * This is the test of record for the whole fix, through the real request | ||
| * pipeline rather than renderToString. It fails on either regression path: | ||
| * | ||
| * - Revert the header to `max-age=0` (or any no-store / private value) and | ||
| * the max-age assertion fails. Nothing else in the suite reads the header, | ||
| * so without this a one-line revert of the fix ships green. | ||
| * - Reintroduce any per-render nondeterminism (the copy-cmd counter class) | ||
| * and the replay fails: the second render hashes to a different ETag, the | ||
| * recorded validator no longer matches, and the expected 304 comes back | ||
| * 200 with a full body, which is exactly how the bug presented in | ||
| * production. render-determinism.test.ts diagnoses that failure precisely; | ||
| * this test proves its consequence at the HTTP layer. | ||
| */ | ||
|
vivek7405 marked this conversation as resolved.
|
||
| import test, { before } from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { resolve, dirname } from 'node:path'; | ||
| import { fileURLToPath } from 'node:url'; | ||
| import { createRequestHandler } from '@webjsdev/server'; | ||
|
|
||
| const WEBSITE_ROOT = resolve(dirname(fileURLToPath(import.meta.url)), '..', '..'); | ||
|
|
||
| let handle: (path: string, headers?: Record<string, string>) => Promise<Response>; | ||
|
|
||
| before(async () => { | ||
| const app = await createRequestHandler({ appDir: WEBSITE_ROOT, dev: false }); | ||
| await app.warmup?.(); | ||
| handle = (path, headers = {}) => app.handle(new Request('http://localhost' + path, { headers })); | ||
| }); | ||
|
|
||
| for (const route of ['/', '/docs/getting-started']) { | ||
| test(`${route} is browser-cacheable and revalidates to an empty 304`, async () => { | ||
| const first = await handle(route); | ||
| assert.equal(first.status, 200); | ||
|
|
||
| const cc = first.headers.get('cache-control') || ''; | ||
| const maxAge = Number(/(?:^|,)\s*max-age=(\d+)/.exec(cc)?.[1] ?? NaN); | ||
| assert.ok(maxAge > 0, | ||
| `max-age must be positive for the browser to ever reuse a stored copy, got: ${cc}`); | ||
| assert.ok(!/no-store|private/.test(cc), | ||
| `a no-store / private value opts the page out of the ETag path entirely, got: ${cc}`); | ||
|
|
||
| const etag = first.headers.get('etag'); | ||
| assert.ok(etag, 'a cacheable 200 carries a validator'); | ||
|
|
||
| const replay = await handle(route, { 'if-none-match': etag! }); | ||
| assert.equal(replay.status, 304, | ||
| 'replaying the ETag must revalidate; a 200 here means consecutive renders ' | ||
| + 'hash differently (see render-determinism.test.ts for the exact divergence)'); | ||
| assert.equal((await replay.text()).length, 0, 'a 304 has no body'); | ||
| assert.equal(replay.headers.get('etag'), etag, 'the validator survives the 304'); | ||
| }); | ||
| } | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| /** | ||
| * Render determinism, the precondition for conditional GET (#1127). | ||
| * | ||
| * The site opts every page into a public `Cache-Control` via the root layout, | ||
| * which also makes the framework attach a weak ETag and answer `If-None-Match` | ||
| * with a 304. That whole path is silently dead if a page does not render the | ||
| * same bytes twice: a different body hashes to a different ETag, the validator | ||
| * the browser holds never matches, and every revalidation ships the full | ||
| * document instead of an empty 304. | ||
| * | ||
| * Nothing else in the suite catches this. The page still renders, every | ||
| * assertion about its content still passes, and the only symptom is a caching | ||
| * layer that quietly never engages. The original offender was a module-scope | ||
| * counter minting `copy-cmd-hint-<n>` ids that never reset in a long-lived | ||
| * server, so consecutive renders of the home page differed by a handful of | ||
| * digits. | ||
| * | ||
| * This test is deliberately generic rather than a check for that one counter, | ||
| * because the failure mode is a CLASS: any `Date.now()`, `Math.random()`, | ||
| * incrementing id, or iteration-order wobble in any component a page renders | ||
| * reintroduces it. | ||
| */ | ||
| import test from 'node:test'; | ||
| import assert from 'node:assert/strict'; | ||
| import { renderToString } from '@webjsdev/core/server'; | ||
| import RootLayout from '#app/layout.ts'; | ||
| import LandingPage from '#app/page.ts'; | ||
| import WhatIsWebJs from '#app/what-is-webjs/page.ts'; | ||
| import WhyWebJs from '#app/why-webjs/page.ts'; | ||
| import GettingStarted from '#app/docs/getting-started/page.ts'; | ||
|
|
||
| // Render each page THROUGH the root layout. The ETag is computed over the whole | ||
| // served document, so a page-only render would miss anything the layout | ||
| // contributes, which is most of the risk surface: it is the layout that stamps | ||
| // the CSP nonce into four script tags and wraps every page's chrome. Guarding | ||
| // the page subtree alone would leave the exact file this test exists to protect | ||
| // (app/layout.ts) with no coverage at all. | ||
| const PAGES: Array<[string, () => unknown]> = [ | ||
| ['/', () => LandingPage()], | ||
| ['/what-is-webjs', () => WhatIsWebJs()], | ||
|
vivek7405 marked this conversation as resolved.
|
||
| ['/why-webjs', () => WhyWebJs()], | ||
| ['/docs/getting-started', () => GettingStarted()], | ||
| ]; | ||
|
|
||
| for (const [route, render] of PAGES) { | ||
| test(`${route} renders identical bytes twice (ETag stability)`, async () => { | ||
| const first = await renderToString(RootLayout({ children: render() }) as any); | ||
| const second = await renderToString(RootLayout({ children: render() }) as any); | ||
| assert.ok(first.length > 5000, 'renders a substantial full document, not a fragment'); | ||
| if (first !== second) { | ||
| // Surface the first divergence rather than dumping two large documents, | ||
| // so the failure names the offending markup directly. | ||
| const a = first.split('\n'); | ||
| const b = second.split('\n'); | ||
| const i = a.findIndex((line, n) => line !== b[n]); | ||
| assert.fail( | ||
| `${route} rendered different bytes on a second render, so its ETag changes every request ` | ||
| + `and a 304 is impossible. First divergence at line ${i + 1}:\n` | ||
| + ` render 1: ${a[i]?.trim()}\n render 2: ${b[i]?.trim()}`, | ||
| ); | ||
| } | ||
| }); | ||
| } | ||
Oops, something went wrong.
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.
Uh oh!
There was an error while loading. Please reload this page.