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.

Sign up for GitHub

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

Jump to bottom

Always avoid caching form submission on enhanced forms #7350

Merged
merged 2 commits into from
Oct 24, 2022
Merged

Always avoid caching form submission on enhanced forms #7350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9369fc5
Always avoid caching form submission on enhanced forms
Tal500 Oct 22, 2022
7192cf3
Change `no-cache` policy to `no-store`
Tal500 Oct 23, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/smart-dolphins-mix.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@sveltejs/kit': patch
---

Always avoid caching form submission on enhanced forms
1 change: 1 addition & 0 deletions packages/kit/src/runtime/app/forms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,7 @@ export function enhance(form, submit = () => {}) {
accept: 'application/json',
'x-sveltekit-action': 'true'
},
cache: 'no-cache',
Copy link
Member

@Rich-Harris Rich-Harris Oct 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to use no-cache rather than no-store? My understanding is that no-cache (confusingly) means 'cache but don't use the cached result', while no-store means 'don't cache', which seems more... thorough. But if no-cache is preferred I will confess ignorance

Suggested change
cache: 'no-cache',
cache: 'no-store',

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm also not sure what is the best practice, I also know only the shallow definitions.

One theoretical advantage of no-cache comparing to no-store, is that if we have the following story:

  1. The user post a guess for sverdle, and it's not a valid word, so the server returns "bad guess"
  2. The user post another guess for sverdle, and it's also not a valid word, so the server returns "bad guess" back again.

If the cache is stored, then the browser can use the ETag HTTP header to check if the server response will be different, and then avoid full download.

Clearly it makes no sense in this small response, but in other cases (like server data processing) it might be useful.
On the other side, in small responses, the cache will be so small, so "who cares".

What do you think? Should it be no-store by default, but let the user override the cache parameter if he wishes?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, interesting — didn't consider the ETag case. Since the action would have to generate those ETags manually (it doesn't happen automatically, unlike with pages, though maybe we could change that? Unsure if worth it), I suspect this is more of a theoretical benefit than anything, whereas no-store feels a bit more belt-and-braces. I see you changed it to no-store in the meantime — let's roll with that for now. Thanks!

body: data,
signal: controller.signal
});
Expand Down