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

Respect cache options in subsequent_fetch #7971

Closed
cristovao-trevisan opened this issue Dec 6, 2022 · 4 comments · Fixed by #8024
Closed

Respect cache options in subsequent_fetch #7971

cristovao-trevisan opened this issue Dec 6, 2022 · 4 comments · Fixed by #8024
Labels
bug Something isn't working

Comments

@cristovao-trevisan
Copy link
Contributor

cristovao-trevisan commented Dec 6, 2022

Describe the bug

Found two bugs in cache implementation at client-side fetcher.js:

  • Current implementation of subsequent_fetch considers only max-age and ignores the cache option
  • Cache delete and get are using different keys

About the cache option (RequestCache from RequestInit), I think it should redo the request if it's set to 'no-store', 'no-cache' or 'reload'.
Another option is to let the browser handle the cache entirely, but I assume there's a reason to be a cache object (and that's why I didn't create a PR).

Reproduction

I've created a very simple example to explain the issue that I have in my private app. To better explain the issue here a few facts of my application (which should be a very common case):

  • I have a separate backend (not running in kit) that provides user data (among other stuff), but the example uses +server.ts to simulate the backend
  • Authentication is done using http-only cookies
  • To check auth, kit calls an API that returns user data (name, image, etc). In the example only name is used
  • I want to load this user data in the layout and cache it (don't need to request at every navigation)
  • I use max-age in the API to cache this data
  • There's a profile page in which the user can update it's own info, e.g. it's name
  • The profile update form sends a POST/PUT request to the backend, then it reloads user data (layout load through invalidation)

Here the problem arises: If max-age is set, the user data is not being reloaded by invalidateAll even if I set { cache: 'reload' } in fetch options.

Example

The example bellow does 3 things to demonstrate this:

  • Calls invalidateAll on form submit (use:enhance)
  • Calls invalidateAll on page load (just to make sure)
  • Calls fetch directly on page load, using cache: 'reload', to show that the response is different from what's on the screen (thus returned by +page.js)

https://github.com/cristovao-trevisan/kit-issue-report

Submitting the form with a different name does not update the page data. By navigating back and forward and looking at the console you can see that the invalidateAll function does not take effect (because the resource is cached), even after reloading the HTTP cache.
Reloading the page does update the data.

image

image

Logs

No response

System Info

System:
    OS: Linux 6.0 Arch Linux
    CPU: (4) x64 Intel(R) Core(TM) i3-5010U CPU @ 2.10GHz
    Memory: 1.19 GB / 7.68 GB
    Container: Yes
    Shell: 5.9 - /usr/bin/zsh
  Binaries:
    Node: 18.12.1 - ~/.nvm/versions/node/v18.12.1/bin/node
    npm: 8.19.2 - ~/.nvm/versions/node/v18.12.1/bin/npm
  Browsers:
    Brave Browser: 108.1.46.134
    Firefox: 107.0.1
  npmPackages:
    @sveltejs/adapter-auto: next => 1.0.0-next.90 
    @sveltejs/kit: next => 1.0.0-next.572 
    svelte: ^3.53.1 => 3.53.1 
    vite: ^3.2.4 => 3.2.5

Severity

serious, but I can work around it

The current work around is to remove the max-age header and request user info at every navigation

Additional Information

No response

@dummdidumm
Copy link
Member

dummdidumm commented Dec 6, 2022

I don't fully understand the issue. To clarify: In your reproduction, you set the cache control header inside +server.js and you expect invalidateAll() to bust the cache - am I understanding this correctly? If so: invalidateAll only relates to load functions in +page(.server).js. If not: Could you clarify the issue?

@cristovao-trevisan
Copy link
Contributor Author

cristovao-trevisan commented Dec 6, 2022

Thanks for que quick answer!!

I've updated the Reproduction section to better explain my issue

@dummdidumm dummdidumm added bug Something isn't working and removed awaiting submitter labels Dec 9, 2022
dummdidumm added a commit that referenced this issue Dec 9, 2022
@dummdidumm
Copy link
Member

dummdidumm commented Dec 9, 2022

Thanks, I understood the problem now.

@cristovao-trevisan
Copy link
Contributor Author

Just saw your PR and it's exactly what I meant. Thanks a lot!

Rich-Harris pushed a commit that referenced this issue Dec 9, 2022
* [fix] respect fetch cache option

fixes #7971

* fix cache key computation
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants