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

Update and modernize adapter-cloudflare-workers #4576

Conversation

pzuraq
Copy link
Contributor

@pzuraq pzuraq commented Apr 8, 2022

I realize that adapter-cloudflare is the preferred solution here since Cloudflare Pages has much more functionality than Workers, but we're stuck in a bind at the moment. Pages is still in beta, and we have more than 100k daily requests for several of our worker apps, more than the beta limit. We've reached out to Cloudflare to see if they could raise the limit for us, but we haven't heard back at all.

In the meantime, we're building out more and more infrastructure based on adapter-cloudflare-workers, which has some key differences from adapter-cloudflare:

  • Namespaces are bound globally instead of passed via the platform API
  • There is no context object passed via the platform API, making it impossible to use useful libraries like Toucan.js
  • Options are not passed to the adapter, so there's no way for us to customize the esbuild pipeline when needed (e.g. for Node polyfills).

This PR updates adapter-cloudflare-workers to use the modules API instead of the standard service-worker setup. This gives it a functionally identical API to adapter-cloudflare, which will allow us to build our infrastructure in a forward-compatible way that will make transitioning much easier once Pages are out of beta.

If this is too big of a change, we can maintain this adapter separately, but I know things are moving quickly and ideally we wouldn't have to keep up with all of the changes as SvelteKit approaches v1. Let me know if this looks good or if anything needs to change, definitely open to feedback and alternatives! Also happy to continue helping out with maintenance for this adapter until Pages is out of beta.

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. N/A (I think)

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

@changeset-bot
Copy link

changeset-bot bot commented Apr 8, 2022

🦋 Changeset detected

Latest commit: 9b8a9a4

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


Generate this file using `wrangler` from your project directory
```toml
[build.upload]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This approach is more opinionated than the last one, which IMO is a better developer experience. Given adapter-cloudflare builds to .svelte-kit/cloudflare and requires the user to set it to that path in Pages, I don't think this leaking any more details. That said, I can revert it if the old style, user-provided config is preferred.

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 it's better if it respects the values in wrangler.toml — the example config we provide in the README/error messages could be the values specified here, but I don't think there's any real benefit to preventing them going somewhere else. For one thing, outDir is configurable

@dominikg
Copy link
Member

dominikg commented Apr 9, 2022

this overlaps with #4276

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 9, 2022

@dominikg indeed it does, should have looked through PRs before working on it 😅

@benmccann @Rich-Harris what can we do to get this PR or #4276 merged? Any blockers I can help out with?

@f5io
Copy link
Contributor

f5io commented Apr 10, 2022

does this also fix the reproducible bug i discovered here #4467

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 10, 2022

It does include the change to add esbuild options, but doesn't include the custom mapping function for kv assets. Don't see why we couldn't include that though, I'll add it.

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 10, 2022

@f5io updated to include the changes from #4467 as well

import { getAssetFromKV, mapRequestToAsset } from '@cloudflare/kv-asset-handler';

// Note: This does not get bundled
import manifestJSON from '__STATIC_CONTENT_MANIFEST';
Copy link
Member

Choose a reason for hiding this comment

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

as mentioned here, I don't really understand what this is for. It seems duplicative with the data that's already in the SSR manifest, so if it's possible to use that then I think that would be preferable (and if it's not possible, it would be useful to understand why so we can fix it)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Replied on the other issue here: #4276 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rich-Harris is the explanation I provided reasonable?

@pzuraq
Copy link
Contributor Author

pzuraq commented Apr 14, 2022

Seems like #4276 is going to take precedence her so going to close this.

@pzuraq pzuraq closed this Apr 14, 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.

4 participants