[breaking] respect cache-control max-age on the client #6461
Conversation
...for initially fetched responses Closes #4625 The proposed etag logic is not implemented because it sounds too complex and would add needless code for 99% of users This is a breaking change insofar that people could have relied on the old behavior somehow
🦋 Changeset detectedLatest commit: 748ddee The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
I think we can simplify the client-side logic a bit by doing more work on the server (i.e. adding a |
|
So this is interesting (and by 'interesting' I mean 'very annoying') — Chrome respects Screen.Recording.2022-08-31.at.3.36.59.PM.movWhich means that if we do this, we'll arguably be broken in Firefox and Safari. Thoughts? |
|
What the ... why don't they do that? Isn't that the most important part of the cache header? |
|
yeah, idk. very surprising to have different behaviour between browsers for something this fundamental |
|
Huh, I guess there's something else going on here. If I create this Node server... import { createServer } from 'http';
createServer((req, res) => {
if (req.url === '/') {
res.writeHead(200, { 'Content-Type': 'text/html' });
res.end(`
<!DOCTYPE html>
<body>
<h1>${new Date().toString()}</h1>
<button>update</button>
<script>
document.querySelector('button').addEventListener('click', async () => {
const res = await fetch('/time');
document.querySelector('h1').innerText = await res.text();
});
</script>
</body>
`);
} else if (req.url === '/time') {
res.writeHead(200, {
'cache-control': 'max-age=5'
});
res.end(new Date().toString());
} else {
res.writeHead(404);
res.end('not found');
}
}).listen(3000);...then all three browsers behave the same way (i.e. respecting the cache header). |
| export { | ||
| vite_config_default as default | ||
| }; | ||
| //# sourceMappingURL=data:application/json;base64,ewogICJ2ZXJzaW9uIjogMywKICAic291cmNlcyI6IFsidml0ZS5jb25maWcuanMiXSwKICAic291cmNlc0NvbnRlbnQiOiBbImNvbnN0IF9fdml0ZV9pbmplY3RlZF9vcmlnaW5hbF9kaXJuYW1lID0gXCIvVXNlcnMvcmljaC9EZXZlbG9wbWVudC9TVkVMVEUvS0lUL2tpdC9wYWNrYWdlcy9hZGFwdGVyLXN0YXRpYy90ZXN0L2FwcHMvcHJlcmVuZGVyZWRcIjtjb25zdCBfX3ZpdGVfaW5qZWN0ZWRfb3JpZ2luYWxfZmlsZW5hbWUgPSBcIi9Vc2Vycy9yaWNoL0RldmVsb3BtZW50L1NWRUxURS9LSVQva2l0L3BhY2thZ2VzL2FkYXB0ZXItc3RhdGljL3Rlc3QvYXBwcy9wcmVyZW5kZXJlZC92aXRlLmNvbmZpZy5qc1wiO2NvbnN0IF9fdml0ZV9pbmplY3RlZF9vcmlnaW5hbF9pbXBvcnRfbWV0YV91cmwgPSBcImZpbGU6Ly8vVXNlcnMvcmljaC9EZXZlbG9wbWVudC9TVkVMVEUvS0lUL2tpdC9wYWNrYWdlcy9hZGFwdGVyLXN0YXRpYy90ZXN0L2FwcHMvcHJlcmVuZGVyZWQvdml0ZS5jb25maWcuanNcIjtpbXBvcnQgeyBzdmVsdGVraXQgfSBmcm9tICdAc3ZlbHRlanMva2l0L3ZpdGUnO1xuXG4vKiogQHR5cGUge2ltcG9ydCgndml0ZScpLlVzZXJDb25maWd9ICovXG5jb25zdCBjb25maWcgPSB7XG5cdGJ1aWxkOiB7XG5cdFx0bWluaWZ5OiBmYWxzZVxuXHR9LFxuXHRwbHVnaW5zOiBbc3ZlbHRla2l0KCldXG59O1xuXG5leHBvcnQgZGVmYXVsdCBjb25maWc7XG4iXSwKICAibWFwcGluZ3MiOiAiO0FBQThhLFNBQVMsaUJBQWlCO0FBR3hjLElBQU0sU0FBUztBQUFBLEVBQ2QsT0FBTztBQUFBLElBQ04sUUFBUTtBQUFBLEVBQ1Q7QUFBQSxFQUNBLFNBQVMsQ0FBQyxVQUFVLENBQUM7QUFDdEI7QUFFQSxJQUFPLHNCQUFROyIsCiAgIm5hbWVzIjogW10KfQo= |
There was a problem hiding this comment.
I dunno why this happened again. Are you aborting tests midway and then committing temporary files? Should we add a gitignore for these to be safe?
There was a problem hiding this comment.
Oh, you deleted it in a later commit. Still, what's creating these?
There was a problem hiding this comment.
that's probably where they're coming from, yeah
|
Ok, digging deeper: it seems that browsers invalidate the cache for a given URL when you POST to it, which seems like reasonable behaviour. I think this wasn't happening before in Chrome because the response was a 500 (because the But it means that our behaviour is wrong — we also need to invalidate the cache if a non-GET request is made. It also means we'll need to split the test between two URLs |
|
Just realised this probably shouldn't apply during prerendering |
|
... I'm not sure what should happen during prerendering. Do we want to permanently cache the responses from endpoints? If we're going to request them the second and later times we visit a page, why not the first time? I know there was an issue floating around about being able to prerender endpoints, and I don't know how that interacts with this. |
|
Endpoints are prerendered, unless they're marked as unprerenderable, in which case prerendering fails. So a prerendered page wouldn't be able to benefit from the cache-control header on the data, but also the data wouldn't require recomputing anyway. I don't think it makes sense to change this (i.e. allow a page to be prerendered without its data also being prerendered) because if the data can become stale between deploys, you shouldn't be prerendering the page in the first place. |
The trick would be knowing when to invalidate it (i.e. when a deploy happened). I tend to think this is best left to CDNs — a 304 is better than a stale 200 from cache. |
...for initially fetched responses
Closes #4625
The proposed etag logic is not implemented because it sounds too complex and would add needless code for 99% of users
This is a breaking change insofar that people could have relied on the old behavior somehow
I implemented this after consulting the MDN docs and concluding that only the max-age and age headers are relevant for us in this case.
Please don't delete this checklist! Before submitting the PR, please make sure you do the following:
Tests
pnpm testand lint the project withpnpm lintandpnpm checkChangesets
pnpm changesetand following the prompts. All changesets should bepatchuntil SvelteKit 1.0