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

feat: use Workers Static Assets #13427

Draft
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

astralhpi
Copy link

@astralhpi astralhpi commented Feb 6, 2025

closes #13360
closes #13071
closes #13005
closes #12813
closes #13579

This PR builds upon the excellent work by @petebacondarwin in PR #13072, focusing on incorporating reviewer feedback and refining the documentation.

Since the original PR seemed to have stalled, I wanted to help move things forward by addressing the feedback. I didn’t make any code changes—just focused on improving the documentation.
Huge thanks to @petebacondarwin for the initial implementation and to the reviewers for their insights.

Looking forward to any additional feedback to get this merged!

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. These adapters don't appear to have tests.

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. Changesets that add features should be minor and those that fix bugs should be patch. Please prefix changeset messages with feat:, fix:, or chore:.

Edits

  • Please ensure that 'Allow edits from maintainers' is checked. PRs without this option may be closed.

petebacondarwin and others added 9 commits November 28, 2024 11:56
This changes the adapter to stop using the old Workers Sites (kv-asset-handler) approach.
Instead, making use of the new Workers Static Assets feature, which is embedded into Cloudflare natively.

Also this change removes the extra esbuild step that was being run inside the adapter, relying upon Wrangler to do the bundling.
The extra esbuild step required a hardcoded list of Node.js compatible modules.
This is no longer needed since Wrangler now manages all of that.

- This version of the adapter requires Wrangler version 3.87.0 or later.

  Run `npm add -D wrangler@latest` (or similar) in your project to update Wrangler.
- The user's Wrangler configuration (`wrangler.toml`) must be migrated from using Workers Sites to using Workers Assets.

  Previously a user's `wrangler.toml` might look like:

  ```toml
  name = "<your-site-name>"
  account_id = "<your-account-id>"
  compatibility_date = "2021-11-12"
  main = "./.cloudflare/worker.js"

  # Workers Sites configuration
  site.bucket = "./.cloudflare/public"
  ```

  Change it to to look like:

  ```toml
  name = "<your-site-name>"
  account_id = "<your-account-id>"
  compatibility_date = "2021-11-12"`
  main = ".svelte-kit/cloudflare/server/index.js"

  # Workers Assets configuration
  assets = { directory = ".svelte-kit/cloudflare/client" }
  ```

- Workers Assets defaults to serving assets directly for a matching request, rather than routing it through the Worker code.

  The previous adapter would add custom headers to assets responses (such as `cache-control`, `content-type`, and `x-robots-tag`. Such direct asset responses no longer contain these headers - but the will include eTag headers that have proven (in Pages) to be an effective caching strategy for assets.

  If you wish to always run the Worker before every request then add `serve_directly = false` to the assets configuration section. For example:

  ```toml
  assets = { directory = ".svelte-kit/cloudflare/client", serve_directly = false }
  ```
- Removed detailed examples to keep the entry concise.
- Provided documentation links for reference.
Copy link

changeset-bot bot commented Feb 6, 2025

🦋 Changeset detected

Latest commit: 906c110

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

This PR includes changesets to release 2 packages
Name Type
@sveltejs/adapter-cloudflare-workers Major
@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

@svelte-docs-bot
Copy link

@eltigerchino eltigerchino added documentation Improvements or additions to documentation pkg:adapter-cloudflare-workers and removed documentation Improvements or additions to documentation labels Feb 7, 2025
@eltigerchino eltigerchino changed the title docs: Follow-up on PR #13072: Enhancements to @sveltejs/adapter-cloudflare-worker Based on Review Feedback feat: use Workers Static Assets Feb 7, 2025
@eltigerchino
Copy link
Member

I don't think the original PR stalled. I'm guessing it's just not ready to be merged or to be continued yet since the Workers Static Assets feature is still in beta. @petebacondarwin what are your thoughts on this? Should we try to get support out during the beta period?

@GauBen
Copy link
Contributor

GauBen commented Feb 25, 2025

It seems like we are no longer bundling the output ourselves with this PR, it's a good thing! The embedded list of compatible libraries is out of sync with the current list, letting wrangler bundle for us will solve that. Thanks for your work

@victormihalache
Copy link

So adapter-cloudflare will keep existing—for supporting Pages, as long as Cloudflare doesn't deprecate it—whilst this will drop Sites in favour of Worker Assets. Am I understanding this correctly?

@GauBen
Copy link
Contributor

GauBen commented Mar 5, 2025

AFAIK, adapter-cloudflare already uses Worker Assets:

res = await env.ASSETS.fetch(req);

adapter-cloudlfare-workers not yet, it pushes and fetches assets from a KV

async function get_asset_from_kv(req, env, context, map = mapRequestToAsset) {

@victormihalache
Copy link

It does make sense given that Cloudflare's own example does use the adapter-cloudflare but what's the purpose of having two adapters that use the same thing?

@emily-shen
Copy link

emily-shen commented Mar 5, 2025

Since we (Cloudflare) are going to be supporting _headers and _redirects files in Workers + Assets soon (not routes.json though), you could totally consolidate this to one adaptor-cloudflare, because both Pages/Workers Assets need the adaptor to output roughly the same files. (That's why we're currently using the "Pages" adaptor for Workers Assets right now - definitely a bit confusing!).

Looks like to me there's two ways forwards, depending on what the Svelte team prefers :)

  1. Use the current "Pages" adaptor for both , and remove _headers and _redirects from the .assetsignore file when that's ready on our side. Only Workers Assets uploads will read that file. Separetely, workers sites is getting deprecated so that adaptor should probably too.
  2. Add headers + redirects from the Pages adaptor to this PR and keep the two adaptors.

Feel free to let me know if there's any help you need from the Cloudflare side to get this through!

cc @petebacondarwin

@astralhpi
Copy link
Author

As @emily-shen mentioned, this seems to be a decision for the Svelte team. :)

From a cautious personal standpoint, Cloudflare Pages and Cloudflare Workers remain separate services. While efforts are underway to unify their interfaces, the process is still ongoing. Given this, I believe adapter-cloudflare-worker remains relevant. Until full unification is achieved, any resulting duplication is incidental rather than intentional, and I think it is reasonable to allow it for now.

@eltigerchino
Copy link
Member

eltigerchino commented Mar 21, 2025

Thanks for this. I've also made some changes to more closely align the two adapters, namely:

adapter-cloudflare:

  • re-implement serving _app/* files from the worker so that when we eventually land the middleware feature it will correctly serve these files since the request will invoke the worker.

adapter-cloudflare-workers:

  • add caching via worktop similar to adapter-cloudflare
  • add support for _headers and _redirects

both:

  • remove the x-robots-tag: noindex header from files in _app/*. This header isn't added in any of our other adapters so it makes sense to standardise this.

},
"dependencies": {
"@cloudflare/workers-types": "^4.20250312.0",
"esbuild": "^0.24.0"
"worktop": "0.8.0-next.18"
Copy link
Member

Choose a reason for hiding this comment

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

why is this pinned to a prerelease and whats the benefit over the probably much leaner esbuild?

Copy link
Member

@eltigerchino eltigerchino Mar 21, 2025

Choose a reason for hiding this comment

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

Worktop is an abstraction of Cloudflare’s caching. It’s already used in the Cloudflare Pages worker but was never added to the workers sites worker so I copied over the implementation

esbuild is still being used, but only as a dev dependency now that we delegate bundling to Wrangler. Esbuild is only used to bundle worktop into the worker before we publish the package

Copy link
Member

Choose a reason for hiding this comment

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

is worktop bundled into the adapter with esbuild or is esbuild still used by the adapter to add worktop in the users build? if the latter esbuild must remain a dependency.

@lukeed it looks like the last -next release of worktop was a year ago and last code change more like 2. is this still the right tool here? what's missing for a "stable" release

Copy link
Member

Choose a reason for hiding this comment

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

Change frequency has nothing to do with stability.

The submodule used is here. The "stable" worktop release has a worktop/cache module too, but the one in this @next build is Cloudflare specific. I was experimenting with target-specific platform builds (new territory at the time) then Hono ran with it as their own, so that was deflating.

My 2¢ is that it's still the right tool but you can inline it if the version name is uncomfortable. No other framework has/offers standalone submodules that you can use w/o inheriting the rest of the framework.

Worktop has nothing to do with esbuild. Looks like this PR just moves esbuild to a devdep.. tho wrangler does want to build/bundle (with their own internal esbuild) by default anwyay

Copy link
Member

@eltigerchino eltigerchino Mar 22, 2025

Choose a reason for hiding this comment

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

is worktop bundled into the adapter with esbuild or is esbuild still used by the adapter to add worktop in the users build? if the latter esbuild must remain a dependency.

It's the former. We use esbuild to bundle worktop into a worker file then publish this to npm. Finally, the adapter uses this file in the user's build output.

'@sveltejs/adapter-cloudflare': patch
---

fix: remove `x-robots-tag: noindex` header for `_app/*` files
Copy link
Member

Choose a reason for hiding this comment

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

what is the issue this is fixing, is it safe to remove?

Copy link
Member

Choose a reason for hiding this comment

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

This was added a long time ago but it wasn’t clear why. I did some googling on it and I don’t think it does anything since search engines mostly index pages only I think? If this is useful, we should be adding it to all the adapters anyway

Copy link
Member

Choose a reason for hiding this comment

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

can we move this to a separate PR then and find out what it was used for? Initial hunch is that crawlers hit these files and incurred cost, so removing it could have negative impact on users that still expect this to work.

@eltigerchino eltigerchino marked this pull request as draft March 21, 2025 17:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants