Skip to content
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

Can't cache a page load (setting "cache-control" with setHeaders doesn't seem to work) #7125

Closed
maximedupre opened this issue Oct 2, 2022 · 6 comments

Comments

@maximedupre
Copy link

maximedupre commented Oct 2, 2022

Describe the bug

For example, setting the cache here has no effect.

export const load: PageLoad = async ({ fetch, setHeaders }) => {
    const res = await fetch(`/api/users`);

    if (!res.ok) throw error(res.status);

    const users: UserV2[] = await res.json();

    setHeaders({
        'cache-control': 'max-age=86400',
    });

    return {
        users,
    };
};

Maybe it's because the initial request to the page contains "Cache-Control: max-age=0"?

So the current behavior is that even if I set the "cache-control" header, the request is always processed by the server (instead of being served by disk/memory cache).

The expected behaviour is that the request is served from cache after being cached (obviously)

Reproduction

Unfortunately, I can't really debug "cache-control" using node.new 🤔 The network panel doesn't give me access to the "sandboxed" requests.

Logs

Raw response headers

HTTP/1.1 304 Not Modified
Access-Control-Allow-Origin: *
cache-control: max-age=86400
etag: "1ons99h"
Date: Sun, 02 Oct 2022 23:00:59 GMT
Connection: keep-alive
Keep-Alive: timeout=5

Raw request headers

GET /top-secret HTTP/1.1
Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8
Accept-Encoding: gzip, deflate, br
Accept-Language: en-US,en;q=0.9
Cache-Control: max-age=0
Connection: keep-alive
Cookie: ...
Host: localhost:3000
If-None-Match: "1ons99h"
Referer: http://localhost:3000/top-secret
Sec-Fetch-Dest: document
Sec-Fetch-Mode: navigate
Sec-Fetch-Site: same-origin
Sec-GPC: 1
Upgrade-Insecure-Requests: 1
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/106.0.0.0 Safari/537.36

System Info

System:
    OS: macOS 12.6
    CPU: (8) arm64 Apple M2
    Memory: 2.30 GB / 24.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.7.0 - ~/.nvm/versions/node/v18.7.0/bin/node
    npm: 8.16.0 - ~/.nvm/versions/node/v18.7.0/bin/npm
  Browsers:
    Brave Browser: 106.1.44.105
    Safari: 16.0
  npmPackages:
    @sveltejs/adapter-node: ^1.0.0-next.96 => 1.0.0-next.96 
    @sveltejs/kit: ^1.0.0-next.504 => 1.0.0-next.504 
    svelte: ^3.50.1 => 3.50.1 `

Severity

serious, but I can work around it

Additional Information

I guess a temporary workaround is to do in-memory caching?

@MindDesign
Copy link

Could this be related to #6477?

@Rich-Harris
Copy link
Member

Related, though that issue is primarily about client-side caching. Unfortunately I don't think there's a good way to cache at the CDN/browser HTTP cache level with the current solution. We might need to investigate alternatives.

@maximedupre
Copy link
Author

@Rich-Harris Do you know what's currently preventing the browser to cache the response? The appropriate headers are sent, but there must be a reason why the browser is ignoring them.

@Rich-Harris
Copy link
Member

Yes, it's all explained in #6477 (comment).

I've just added stringify and parse functions to devalue: https://github.com/Rich-Harris/devalue#stringify-and-parse. This would allow us to use a more conventional approach to serializing non-JSON-stringifiable values if we adopted it in SvelteKit. Payloads would get slightly larger, and we'd have to add 0.5kb of parsing logic, but the benefits probably outweigh the costs.

dummdidumm added a commit that referenced this issue Oct 7, 2022
using devalue's stringify/parse methods
Part of #7125 and #6477
@geoffrich geoffrich mentioned this issue Oct 7, 2022
5 tasks
Rich-Harris added a commit that referenced this issue Oct 10, 2022
* [fix] load data through regular json request

using devalue's stringify/parse methods
Part of #7125 and #6477

* fixes

* more idiomatic usage

* changeset

Co-authored-by: Rich Harris <hello@rich-harris.dev>
@dummdidumm
Copy link
Member

I don't quite understand the issue - at least I can't reproduce this. When testing this myself I see the cache header being respected. Your response log indicates the same, it says "304 not modified" with the cache-control header set accordingly. From https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/304 : "Note: Many developer tools' network panels of browsers create extraneous requests leading to 304 responses, so that access to the local cache is visible to developers."

@Rich-Harris
Copy link
Member

Yeah, I think we fixed it a while back but didn't close this issue. Closing now

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants