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

+page.server.js no longer uses/respects setHeaders #6477

Closed
homerjam opened this issue Aug 31, 2022 · 10 comments · Fixed by #7532
Closed

+page.server.js no longer uses/respects setHeaders #6477

homerjam opened this issue Aug 31, 2022 · 10 comments · Fixed by #7532
Milestone

Comments

@homerjam
Copy link

homerjam commented Aug 31, 2022

Describe the bug

Since this commit the +page.server.js endpoints return a js file rather than json. This js file response no longer uses the headers set with the setHeaders function. This means we can't use cache-control etc.

Reproduction

To reproduce, hover over the 'TODOS' menu item and observer the '__data.js' response headers. There should be a header as defined in routes/todos/+page.server.js;

https://stackblitz.com/edit/sveltejs-kit-template-default-8tt4be?file=src/routes/todos/+page.server.js

Screenshot 2022-08-31 at 17 29 32

Logs

No response

System Info

System:
    OS: macOS 12.5
    CPU: (8) arm64 Apple M2
    Memory: 155.45 MB / 24.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 16.16.0 - ~/.nvm/versions/node/v16.16.0/bin/node
    Yarn: 1.22.19 - /opt/homebrew/bin/yarn
    npm: 8.11.0 - ~/.nvm/versions/node/v16.16.0/bin/npm
  Browsers:
    Chrome: 104.0.5112.101
    Safari: 15.6
  npmPackages:
    @sveltejs/adapter-auto: 1.0.0-next.70 => 1.0.0-next.70 
    @sveltejs/kit: 1.0.0-next.456 => 1.0.0-next.456 
    svelte: ^3.49.0 => 3.49.0 
    vite: 3.1.0-beta.1 => 3.1.0-beta.1

Severity

annoyance

Additional Information

No response

@benmccann benmccann added this to the 1.0 milestone Sep 4, 2022
@TranscendentalRide
Copy link

I noticed this in particular when trying to overwrite some headers with throw redirect(301, '/todos'). In particular want the redirect to look like it is external not internal; no luck.

example

setHeaders({
    'sec-fetch-mode': 'navigate',
    'sec-fetch-dest': 'document',
})
...

throw redirect(301,'/todos')

Doesn't work, actual headers

    'sec-fetch-mode' => 'cors',
    'sec-fetch-dest' => 'script'

@homerjam
Copy link
Author

This is very frustrating as it means making an unnecessary request to the page endpoint when going 'back' a page. As a workaround is there a way to intercept this somehow and prevent it happening (using an in memory store somewhere instead)?

@dummdidumm
Copy link
Member

Not sure how to solve this with regards to cache headers. Other headers seem easy, just add them back. Cache headers however are in conflict with other feature such as await parent() - how to ensure that what you request is really something for which a cache hit should occur? If the parent +layout.server.js changed, or your +page.server.js depends on url, setting cache headers would result in wrong cache hits. One could argue that's on the developer to ensure cache headers are set appropriately, but I'm not sure this is just a lazy answer.

@homerjam
Copy link
Author

The issue right now is that we can't add headers at all (we also seemingly can't prevent calling the page endpoint unnecessarily even if the data is held in memory). I think it's sensible not to add cache headers by default and leave that to the developer. Caching is, in general, a nightmare of complexity but I just want a simple 15min max-age to make the once-a-day visitor to this portfolio site have a slightly better experience.

Side note, but also now that the request goes to __data.ja it doesn't appear in the fetch/xhr panel in dev tools (where I would expect it!).

dummdidumm added a commit that referenced this issue Sep 14, 2022
Add all headers except cache-control which is always no-store
Fixes #6458
Fixes #6477
@Rich-Harris
Copy link
Member

Cache headers won't work with __data.js, because SvelteKit needs to add a ?__id={x} query parameter otherwise it will always use stale data. Since it's using import rather than fetch, it can't access the headers, and so can't not add __id. Furthermore, if the response was cached then you might reload the page and start getting the wrong data entirely, just because __id matched a previous response for a different URL.

The solution here is for SvelteKit to read cache headers when generating the __data.js file and add something along these lines:

window.__sveltekit_ttl = 300;

The client can read that value and cache the data for 300 seconds, reusing it if the user navigates back to the same URL. (Though there is a footgun here — if the data was mutated, the mutation would persist.)

@homerjam
Copy link
Author

Would this solution work per page?

Where would SvelteKit read the headers from exactly?

Could we add __ttl to the object returned by the load function in the *.server.js file to make things more explicit?

I'm basically looking to avoid requesting/importing the data if I've already visited the page. I hope I'm understanding this correctly - but it seems like we're on the same page (no pun intended : )

@dummdidumm
Copy link
Member

dummdidumm commented Sep 23, 2022

Another thought I just had: What if the id was made up of two things? One unique per page session (full page reload -> new ID), one unique per request. The latter is stored in some url->id map, so if you go back to an already visited page that id is reused. That way the data request is unique per full-page-reload but reusable per client-side page visit:

const urlId = map.get(url) ?? id++;  // maybe we can even use event.routeId here and spare us the map?
//...
__data.js?__id={uniqueId}_{urlId}

This then enables us to store the cache header on the request.

@Rich-Harris
Copy link
Member

Where would SvelteKit read the headers from exactly?

If you call setHeaders({ 'cache-control': 'max-age: 300' }) in one of your load functions, SvelteKit would turn that into window.__sveltekit_ttl = 300. Very similar to this...

if (!prerendering && fetched.method === 'GET' && cache_control) {
const match = /s-maxage=(\d+)/g.exec(cache_control) ?? /max-age=(\d+)/g.exec(cache_control);
if (match) {
const ttl = +match[1] - +(age ?? '0');
attrs.push(`data-ttl="${ttl}"`);
}
}
...which allows fetch to be aware of cache headers applied to responses that are inlined into the HTML.

This would only re-use data for back/forward navigations — it wouldn't help if you visited /a, navigated to /b, then in a new session visited /a then /b/b/__data.js would be generated in both cases. If we wanted to make that work, I think we'd have to rethink the decision to use devalue.

@dummdidumm that wouldn't work because the data would be stale when revisiting a URL — you wouldn't be loading data from the server, you'd be loading it from the module cache (except you wouldn't, because we delete window.__sveltekit_data to prevent memory leaks)

@homerjam
Copy link
Author

If you call setHeaders({ 'cache-control': 'max-age: 300' }) in one of your load functions, SvelteKit would turn that into window.__sveltekit_ttl = 300

This would only re-use data for back/forward navigations

Great. If I'm understanding correctly this seems to solve my issue/goal precisely. In a real world scenario I wouldn't expect a visitor to refresh the page/start a new session and even if they I think it's fine to not use the browser cache.

But I'm still wondering if this works per page/route? For instance there may be some pages that I would like to be "refreshed" each time (although I guess it might be possible to manually trigger an update onmount for example each time the page is visited). Does the __sveltekit_ttl value get applied/removed each navigation?

@Rich-Harris
Copy link
Member

It would apply per-URL, and data would be reused until it was stale. Manual invalidation would override ttl

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 added a commit that referenced this issue Nov 7, 2022
...using the Vary header, which allows on cached response for each variation of the x-sveltekit-invalidated header, ensuring we cache matching responses for all invalidation scenarios
Closes #6477
Rich-Harris added a commit that referenced this issue Nov 7, 2022
* [feat] enable caching for __data.json requests

...using the Vary header, which allows on cached response for each variation of the x-sveltekit-invalidated header, ensuring we cache matching responses for all invalidation scenarios
Closes #6477

* Update .changeset/clean-sheep-run.md

* use append

Co-authored-by: Rich Harris <richard.a.harris@gmail.com>
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

Successfully merging a pull request may close this issue.

5 participants