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

Fix browser caching of endpoints instead of page #9780

Closed
oscarhermoso opened this issue Apr 27, 2023 · 5 comments · Fixed by #9993
Closed

Fix browser caching of endpoints instead of page #9780

oscarhermoso opened this issue Apr 27, 2023 · 5 comments · Fixed by #9993
Labels
feature / enhancement New feature or request

Comments

@oscarhermoso
Copy link
Contributor

oscarhermoso commented Apr 27, 2023

Describe the bug

Set up: Visit a page with client side navigation, where the destination +page.svelte file is in the same directory as a +server.js GET request handler, and a +page.js universal load function.

Then duplicate the tab.

Expected: An identical HTML page is rendered in the new browser tab.
Actual: The JSON response from the +server.js is rendered.

This also happens when using back/forwards browser navigation

Reproduction

Minimal repo:

https://github.com/oscarhermoso/bug-sveltekit-duplicate-tab/tree/main

Recording:

bug-sveltekit-duplicate-tab

Logs

N/A

System Info

npx envinfo --system --binaries --browsers --npmPackages "{svelte,@sveltejs/*,vite}"

  System:
    OS: Linux 6.2 Pop!_OS 22.04 LTS
    CPU: (12) x64 AMD Ryzen 5 3600 6-Core Processor
    Memory: 4.62 GB / 15.56 GB
    Container: Yes
    Shell: 5.1.16 - /bin/bash
  Binaries:
    Node: 16.14.2 - ~/.nvm/versions/node/v16.14.2/bin/node
    Yarn: 1.22.18 - /usr/local/bin/yarn
    npm: 8.5.0 - ~/.nvm/versions/node/v16.14.2/bin/npm
  Browsers:
    Chrome: 112.0.5615.165
    Chromium: 112.0.5615.49
    Firefox: 112.0.1
  npmPackages:
    @sveltejs/adapter-auto: ^2.0.0 => 2.0.1 
    @sveltejs/kit: ^1.5.0 => 1.15.9 
    svelte: ^3.54.0 => 3.58.0 
    vite: ^4.3.0 => 4.3.3

Severity

annoyance

Additional Information

This isn't related to any recent Sveltekit version - I am able to reproduce on Sveltekit 1.0.

It seems to be because of browser caching of the fetch request made on client load, because I can work-around the issue by adding Cache-Control headers to the GET request handler

setHeaders({
    'Cache-Control': 'No-Store',
  });

No response

@Conduitry
Copy link
Member

This sounds like the expected - if unfortunate in this case - behavior to me. Browsers seem to cache fairly aggressively when duplicating tabs. If the request does make it to SvelteKit, then it will render either the page or endpoint according to the Accept header - per https://kit.svelte.dev/docs/routing#server-content-negotiation. So if you want your endpoint's response to be cached not only by URL (which is the same as your page) but also by the Accept header currently being used to request it, you can specify the Vary: Accept header in your response if you don't want to forego caching altogether. I don't know that there's anything SvelteKit could reasonably do here.

@oscarhermoso
Copy link
Contributor Author

oscarhermoso commented Apr 27, 2023

@Conduitry - Very nice, the Vary: Accept header resolves the issue and is definitely a better solution than 'Cache-Control': 'No-Store'. Perhaps it should be a default response header for routes with adjacent +page.svelte and +server.js files?

@Conduitry Conduitry added the feature / enhancement New feature or request label Apr 27, 2023
@Conduitry
Copy link
Member

It does seem useful, but I can't decide how I feel about quietly mutating the Response explicitly returned from an endpoint handler and returning a different response with an additional header. Maybe this behavior ought to be opt-in? I'll let other maintainers weigh in here.

@oscarhermoso
Copy link
Contributor Author

oscarhermoso commented Apr 28, 2023

I think it makes perfect sense for the Vary: Accept header to be implicitly added where we serve different data on the same URL path.

For example, Django REST Framework does this for their REST API - which has a friendly HTML interface when viewed in the browser.

https://github.com/encode/django-rest-framework/blob/1ce0853ac51635526dc1a285e6b83c9848002f0e/rest_framework/views.py#L153-L160

If the header was included in Sveltekit responses, we might need to address this block too:

https://github.com/sveltejs/kit/blob/526a2ed5676905807d83ace90d3853027f17f265/packages/kit/src/runtime/server/page/serialize_data.js#LL91C1-L102C1

@eltigerchino
Copy link
Member

It does seem useful, but I can't decide how I feel about quietly mutating the Response explicitly returned from an endpoint handler and returning a different response with an additional header. Maybe this behavior ought to be opt-in? I'll let other maintainers weigh in here.

It makes sense to add it in when content-negotiation is happening. It'll be good to document it as well so that it can be opt-out / overridden / removed ?

@oscarhermoso oscarhermoso changed the title Duplicating tab renders JSON payload instead of page Fix browser caching of enpoints instead of page May 20, 2023
@oscarhermoso oscarhermoso changed the title Fix browser caching of enpoints instead of page Fix browser caching of endpoints instead of page May 20, 2023
dummdidumm pushed a commit that referenced this issue Jul 4, 2023
- 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 <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature / enhancement New feature or request
Projects
None yet
3 participants