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 the new Workers Static Assets feature from Cloudflare #13072

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

petebacondarwin
Copy link

Resolves #13071

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.

Breaking changes and migration

  • 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:

    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:

    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:

    assets = { directory = ".svelte-kit/cloudflare/client", serve_directly = false } 

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.

Copy link

changeset-bot bot commented Nov 28, 2024

🦋 Changeset detected

Latest commit: 30d1f46

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-workers Major

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

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 }
  ```
@petebacondarwin petebacondarwin force-pushed the update-cloudflare-workers-adapter branch from bc677da to 30d1f46 Compare November 28, 2024 11:56
@petebacondarwin
Copy link
Author

I made a breaking change here since the Workers Sites approach is deprecated and not recommended: previously we already recommended using Cloudflare Pages rather than Workers Sites; and soon we will recommend Workers Static Assets rather than Cloudflare Pages. But if this is too big a change, we could make the adapter resilient to work with either approach.

@eltigerchino
Copy link
Member

eltigerchino commented Nov 29, 2024

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.

Is this a change we should make for our Cloudflare Pages adapter too?

try {
const result = await esbuild.build({
platform: 'browser',
// https://github.com/cloudflare/workers-sdk/blob/a12b2786ce745f24475174bcec994ad691e65b0f/packages/wrangler/src/deployment-bundle/bundle.ts#L35-L36
conditions: ['workerd', 'worker', 'browser'],
sourcemap: 'linked',
target: 'es2022',
entryPoints: [`${tmp}/_worker.js`],
outfile: `${dest}/_worker.js`,
allowOverwrite: true,
format: 'esm',
bundle: true,
loader: {
'.wasm': 'copy',
'.woff': 'copy',
'.woff2': 'copy',
'.ttf': 'copy',
'.eot': 'copy',
'.otf': 'copy'
},
external,
alias: Object.fromEntries(compatible_node_modules.map((id) => [id, `node:${id}`])),
logLevel: 'silent'
});

@petebacondarwin
Copy link
Author

@eltigerchino - this is something that could be updated on the other Cloudflare adapters if you wish. Not a blocker - but it does mean that the adapter becomes simpler and is less brittle.

@eltigerchino

This comment was marked as outdated.

```toml
name = "<your-site-name>"
account_id = "<your-account-id>"
compatibility_date = "2021-11-12"`
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
compatibility_date = "2021-11-12"`
compatibility_date = "2021-11-12"

Comment on lines +7 to +8
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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.
This changes the adapter to stop using the old Workers Sites (kv-asset-handler) approach in favor of the new Workers Static Assets feature, which is embedded into Cloudflare natively.

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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Also this change removes the extra esbuild step that was being run inside the adapter, relying upon Wrangler to do the bundling.
Also, this change removes the extra esbuild step that was being run inside the adapter, instead relying upon Wrangler to do the bundling.

Comment on lines +11 to +12
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.
Copy link
Member

Choose a reason for hiding this comment

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

I think we can probably drop this to keep things simpler

Suggested change
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.

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:
Copy link
Member

Choose a reason for hiding this comment

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

is there any migration guide on the cloudflare site we could point to instead of including the details here? most of our changelog entries are fairly minimal


- 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.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
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.
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 they will include eTag headers that have proven (in Pages) to be an effective caching strategy for assets.

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.

Update adapter-cloudflare-workers to use the new native Cloudflare Workers Assets feature
3 participants