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

Add cloudflare cache #4412

Merged
merged 5 commits into from
Mar 24, 2022
Merged

Conversation

johnwalshuk
Copy link
Contributor

@johnwalshuk johnwalshuk commented Mar 21, 2022

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 pnpx changeset and following the prompts. All changesets should be patch until SvelteKit 1.0

Resolves #3862.

Add cache to dynamically generated pages at the edge by using the cloudflare cache api. Pages that have a cache policy (maxage) will be stored.

@changeset-bot
Copy link

changeset-bot bot commented Mar 21, 2022

🦋 Changeset detected

Latest commit: f877449

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

This PR includes changesets to release 1 package
Name Type
@sveltejs/adapter-cloudflare 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

@PH4NTOMiki
Copy link
Contributor

@johnwalshuk You aren't checking for the max-age(time nor even is it present) as far as i can see

@johnwalshuk
Copy link
Contributor Author

@johnwalshuk You aren't checking for the max-age(time nor even is it present) as far as i can see

Cloudflare inspects the response headers automatically. See https://developers.cloudflare.com/workers/runtime-apis/cache/#headers

@PH4NTOMiki
Copy link
Contributor

Didn't know that, but still, relying on it isn't really advisable.

@lukeed
Copy link
Member

lukeed commented Mar 21, 2022

@PH4NTOMiki why?

The only things to be wary of are invalid parameters and errors. But there's a try/catch in place already so both can be ignored in this case.

Personally I manually check before any calls to cache.put just to be sure, but what's here (after suggestion) should be okay for general use. If anything needs to be added, it's appending 'private=Set-Cookie' when there's a cookie present.

Co-authored-by: Luke Edwards <luke.edwards05@gmail.com>
@PH4NTOMiki
Copy link
Contributor

@lukeed okay, if you say that, then I stand corrected. but as you said personally, checking it is nice, and it's clearer what's going on in the code.

platform: { env, context },
getClientAddress() {
return req.headers.get('cf-connecting-ip');
// @ts-ignore
Copy link
Member

@benmccann benmccann Mar 22, 2022

Choose a reason for hiding this comment

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

Can we use ServiceWorkerGlobalScope from @cloudflare/workers-types so that we don't have to @ts-ignore this?

If there's no solution then it should be turned into @ts-expect-error along with a comment explaining why it cannot be addressed, but I would hope that it can be

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey @benmccann, I tried to add the package above but it started to throw ~30 errors in the JS files so for now I've used @ts-expect-error with a comment as to why.

@benmccann benmccann merged commit 08bb98c into sveltejs:master Mar 24, 2022
@jahir9991
Copy link

how can I disable this feature?

@mastermakrela
Copy link
Contributor

mastermakrela commented Apr 16, 2022

@jahir9991

how can I disable this feature?

I'm not sure if it's the right solution (or if we had the same problem 😅).
But after adding the following headers, the Responses aren't cached any more:

headers: {
  "Cache-Control": "no-store",
}

Edit: removed "max-age": "no-cache" header, as it's not needed.

@lukeed
Copy link
Member

lukeed commented Apr 16, 2022

The cache-control header is the only relevant header here

@jahir9991
Copy link

how can configure it for disable cache in global layer

@lukeed
Copy link
Member

lukeed commented Apr 20, 2022

You can use a global handle hook to ensure Cache-Control headers are doing what you want:

// src/hooks.js

/** @type {import('@sveltejs/kit').[Handle](https://kit.svelte.dev/docs/types#sveltejs-kit-handle)} */
export async function handle({ event, resolve }) {
  event.request = new Request(event.request);
  event.request.headers.set('cache-control', 'no-cache');
 
  let response = await resolve(event);

  response = new Response(response.body, response);
  response.headers.set('cache-control', 'no-cache');

  return response;
}

untested, but should work based on docs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[adapter-cloudflare-workers] Support writing rendered page to Cloudflare cache to achieve ISR
6 participants