From 42c9b935e49fbbeff7065d1b2a31986c0b814956 Mon Sep 17 00:00:00 2001 From: Oscar Hermoso Date: Tue, 4 Jul 2023 17:04:21 +0800 Subject: [PATCH] feat: better Vary header support (#9993) - support caching of responses with `Vary` header (possible without any changes on the client because since #8754 we're taking headers into account for the cache key) - fix browser caching of adjacent pages/endpoints fixes #9780 --------- Co-authored-by: S. Elliott Johnson --- .changeset/shy-ears-report.md | 5 +++++ .changeset/strange-ladybugs-judge.md | 5 +++++ documentation/docs/20-core-concepts/10-routing.md | 3 ++- .../kit/src/runtime/server/page/serialize_data.js | 13 ++++++------- .../src/runtime/server/page/serialize_data.spec.js | 4 ++-- packages/kit/src/runtime/server/respond.js | 12 ++++++++++++ packages/kit/test/apps/basics/test/client.test.js | 4 +++- 7 files changed, 35 insertions(+), 11 deletions(-) create mode 100644 .changeset/shy-ears-report.md create mode 100644 .changeset/strange-ladybugs-judge.md diff --git a/.changeset/shy-ears-report.md b/.changeset/shy-ears-report.md new file mode 100644 index 000000000000..8f955038d8e8 --- /dev/null +++ b/.changeset/shy-ears-report.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': minor +--- + +feat: support caching of responses with `Vary` header (except for `Vary: *`) diff --git a/.changeset/strange-ladybugs-judge.md b/.changeset/strange-ladybugs-judge.md new file mode 100644 index 000000000000..9d8f383483a2 --- /dev/null +++ b/.changeset/strange-ladybugs-judge.md @@ -0,0 +1,5 @@ +--- +'@sveltejs/kit': patch +--- + +fix: include `Vary: Accept` header to fix browser caching of adjacent pages and endpoints diff --git a/documentation/docs/20-core-concepts/10-routing.md b/documentation/docs/20-core-concepts/10-routing.md index 4f10d26c36f0..5cb48bd52e4a 100644 --- a/documentation/docs/20-core-concepts/10-routing.md +++ b/documentation/docs/20-core-concepts/10-routing.md @@ -330,7 +330,8 @@ export async function POST({ request }) { `+server.js` files can be placed in the same directory as `+page` files, allowing the same route to be either a page or an API endpoint. To determine which, SvelteKit applies the following rules: - `PUT`/`PATCH`/`DELETE`/`OPTIONS` requests are always handled by `+server.js` since they do not apply to pages -- `GET`/`POST` requests are treated as page requests if the `accept` header prioritises `text/html` (in other words, it's a browser page request), else they are handled by `+server.js` +- `GET`/`POST` requests are treated as page requests if the `accept` header prioritises `text/html` (in other words, it's a browser page request), else they are handled by `+server.js`. +- Responses to `GET` requests will inlcude a `Vary: Accept` header, so that proxies and browsers cache HTML and JSON responses separately. ## $types diff --git a/packages/kit/src/runtime/server/page/serialize_data.js b/packages/kit/src/runtime/server/page/serialize_data.js index f176fdfb41a0..b898a8a4e910 100644 --- a/packages/kit/src/runtime/server/page/serialize_data.js +++ b/packages/kit/src/runtime/server/page/serialize_data.js @@ -46,7 +46,7 @@ export function serialize_data(fetched, filter, prerendering = false) { let cache_control = null; let age = null; - let vary = false; + let varyAny = false; for (const [key, value] of fetched.response.headers) { if (filter(key, value)) { @@ -54,8 +54,8 @@ export function serialize_data(fetched, filter, prerendering = false) { } if (key === 'cache-control') cache_control = value; - if (key === 'age') age = value; - if (key === 'vary') vary = true; + else if (key === 'age') age = value; + else if (key === 'vary' && value.trim() === '*') varyAny = true; } const payload = { @@ -89,10 +89,9 @@ export function serialize_data(fetched, filter, prerendering = false) { } // Compute the time the response should be cached, taking into account max-age and age. - // Do not cache at all if a vary header is present, as this indicates that the cache is - // likely to get busted. It would also mean we'd have to add more logic to computing the - // selector on the client which results in more code for 99% of people for the 1% who use vary. - if (!prerendering && fetched.method === 'GET' && cache_control && !vary) { + // Do not cache at all if a `Vary: *` header is present, as this indicates that the + // cache is likely to get busted. + if (!prerendering && fetched.method === 'GET' && cache_control && !varyAny) { const match = /s-maxage=(\d+)/g.exec(cache_control) ?? /max-age=(\d+)/g.exec(cache_control); if (match) { const ttl = +match[1] - +(age ?? '0'); diff --git a/packages/kit/src/runtime/server/page/serialize_data.spec.js b/packages/kit/src/runtime/server/page/serialize_data.spec.js index 6aeb284a4ac1..3a4c60e620f3 100644 --- a/packages/kit/src/runtime/server/page/serialize_data.spec.js +++ b/packages/kit/src/runtime/server/page/serialize_data.spec.js @@ -81,7 +81,7 @@ test('computes ttl using cache-control and age headers', () => { ); }); -test('doesnt compute ttl when vary header is present', () => { +test('doesnt compute ttl when vary * header is present', () => { const raw = 'an "attr" & a \ud800'; const escaped = 'an "attr" & a �'; const response_body = ''; @@ -93,7 +93,7 @@ test('doesnt compute ttl when vary header is present', () => { request_body: null, response_body, response: new Response(response_body, { - headers: { 'cache-control': 'max-age=10', vary: 'accept-encoding' } + headers: { 'cache-control': 'max-age=10', vary: '*' } }) }, () => false diff --git a/packages/kit/src/runtime/server/respond.js b/packages/kit/src/runtime/server/respond.js index 47b8d5c56b26..28a36dd33769 100644 --- a/packages/kit/src/runtime/server/respond.js +++ b/packages/kit/src/runtime/server/respond.js @@ -402,6 +402,18 @@ export async function respond(request, options, manifest, state) { throw new Error('This should never happen'); } + // If the route contains a page and an endpoint, we need to add a + // `Vary: Accept` header to the response because of browser caching + if (request.method === 'GET' && route.page && route.endpoint) { + const vary = response.headers + .get('vary') + ?.split(',') + ?.map((v) => v.trim().toLowerCase()); + if (!(vary?.includes('accept') || vary?.includes('*'))) { + response.headers.append('Vary', 'Accept'); + } + } + return response; } diff --git a/packages/kit/test/apps/basics/test/client.test.js b/packages/kit/test/apps/basics/test/client.test.js index 9bb897a44e61..ce3be3bf6b27 100644 --- a/packages/kit/test/apps/basics/test/client.test.js +++ b/packages/kit/test/apps/basics/test/client.test.js @@ -650,7 +650,9 @@ test.describe('data-sveltekit attributes', () => { test.describe('Content negotiation', () => { test('+server.js next to +page.svelte works', async ({ page }) => { - await page.goto('/routing/content-negotiation'); + const response = await page.goto('/routing/content-negotiation'); + + expect(response.headers()['vary']).toBe('Accept'); expect(await page.textContent('p')).toBe('Hi'); const pre = page.locator('pre');