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

Always avoid caching form submission on enhanced forms #7350

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

Tal500
Copy link
Contributor

@Tal500 Tal500 commented Oct 22, 2022

Due to the return of regular JSON requests of #7177, issue #6357 is relevant back again. Although now I couldn't reproduce the behavior, it stills problematic and needed to be fixed.

Clearly, when you simulate regular form submission, you should check that the browser never used a cached result of previous POST request.

It is unclear how browsers choose to handle this situation, see this stackoverflow question.
Since browsers seems to be more careful on programically fetch POST requests, I couldn't catch a browser really use caching for this "no cache-control header" responses.

Currently on form submission from enhanced forms, there is no cache control header from the server, no cache guideline on the fetch function, so I think that the best solution is to guide the browser in the fetch method of the submission, to not use previously cached results.

fixes #6357 (in some sense)

Please don't delete this checklist! Before submitting the PR, please make sure you do the following:

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with pnpm test and lint the project with pnpm lint and pnpm check

Changesets

  • If your PR makes a change that should be noted in one or more packages' changelogs, generate a changeset by running pnpm changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

@changeset-bot
Copy link

changeset-bot bot commented Oct 22, 2022

🦋 Changeset detected

Latest commit: 7192cf3

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@sveltejs/kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@@ -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

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!

Co-authored-by: Rich Harris <hello@rich-harris.dev>
@Rich-Harris Rich-Harris merged commit d6a909c into sveltejs:master Oct 24, 2022
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 this pull request may close these issues.

Invalidatation should always query the invalid server nodes with fetch({ cache: 'no-cache' })
2 participants