refactor: use Module Worker pattern instead of Service Worker, add error handling #4276

dominikg · 2022-03-08T21:52:16Z

as discussed on discord.

switch to ModuleWorker pattern
add error handling for static kv requests
pass platform to server.respond

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

changeset-bot · 2022-03-08T21:52:19Z

🦋 Changeset detected

Latest commit: 9b70922

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

dominikg · 2022-03-08T21:53:10Z

note: pnpm check returned an unrelated error for adapter-netlify when i ran it locally, pnpm test failed a lot with create page timeouts. lets see what ci shows here.

Rich-Harris · 2022-03-09T21:25:48Z

packages/adapter-cloudflare-workers/files/entry.js

+			{
+				ASSET_NAMESPACE: env.__STATIC_CONTENT,
+				ASSET_MANIFEST: static_asset_manifest
+			}


mentioned this in discord, but for posterity: would be good to understand why (if) this is necessary

taken directly from the docs of kv-asset-handler: https://github.com/cloudflare/kv-asset-handler#es-modules

maybe someone from CF or with more experience using it can chime in why it is done that way?

The binding name to the KV Namespace populated with key/value entries of files for the Worker to serve. By default, Workers Sites uses a binding to a Workers KV Namespace named __STATIC_CONTENT.

Awkward, but is what it is. Both are required

I don't understand how the existing adapter works if this is true? Is it a different between the service worker and module formats? (Also that link is broken btw :)

@Rich-Harris Service workers get KV namespaces and env variables bound to global, while for module workers, they are provided in an env parameter passed to the exported fetch function. The current implementation for service workers doesn't pass any options to getAssetFromKV, so it defaults to using the globally bound __STATIC_CONTENT.
Sorry to butt in, just looking forward to this getting merged ;)

Handling cache headers / etags is needed. The current approach of far-future caching hashed assets is good, this PR had just added some error handling to it.

If we were to replace kvAssetHandler with our own logic, do we need to handle edge caching too? What about prerender results? At some point we're ending up replementing a lot of things that are mostly generic.

Would it be more useful to do this in a reusable/external way, eg in worktop or via sending PRs to kvAssetHandler. Or even having a separate "cloudflare-adapter-utils" library that can be shared between the two cf adapters.

I'm no longer confident we can easily merge the two because of the fundamental difference that one needs a local build step and the other one builds on cf.

We should serve prerender results, yeah. As for ETags, we probably want to generate content hashes at build time, which has implications for the shape of SSRManifest

__STATIC_CONTENT_MANIFEST is also necessary because Wrangler adds an additional fingerprint to every asset it uploads, and it contains the mapping from the requested asset to the fingerprinted one.

What kind of fingerprint?

Here's an example with a test app I deployed:

I'm unsure what adds these extra fingerprints, or why they need to exist, but without the __STATIC_CONTENT_MANIFEST the files will not be found.

This is definitely not ideal, but I think it's acceptable given as soon as Pages is out of beta, everyone is going to drop Workers and migrate over. If for some reason that doesn't happen, I'd be happy to dig in further and try to optimize this more, but until that's certain I think it's best to use the standard Cloudflare methodology for the time being.

hmnd · 2022-03-10T09:11:31Z

BTW this should also fix #2844

hmnd

Really excited to finally get env var support for this adapter.
Some more changes are needed to index.js for this to work. See my fork here. I've added support for using the value of build.upload.main in there too.

…eltejs#4276 (review)

dominikg · 2022-03-10T20:35:06Z

Really excited to finally get env var support for this adapter. Some more changes are needed to index.js for this to work. See my fork here. I've added support for using the value of build.upload.main in there too.

thank you for this! Unfortunately it didn't show me a button to accept your changes. so i copied them over.

Readme and changeset probably need a few more words too.

dominikg · 2022-03-10T20:37:39Z

packages/adapter-cloudflare-workers/index.js

@@ -86,6 +94,24 @@ function validate_config(builder) {
 			);
 		}

+		// @ts-ignore
+		const main_file = wrangler_config.build?.upload?.main;


is this needed or could/should we hardcode/use the filename for main instead?

The checks below feel a bit brittle, what if there is no glob but an excact matching rule instead?

May be a bug with wrangler, but main seems to be treated according to the rules, regardless of the specified format. My build kept failing to upload until I changed the name to .mjs or added a rule to treat *.js as ESM. build.upload.main is actually required, so we can probably get rid of all the ?. there.

dominikg · 2022-03-10T20:39:46Z

On a more general note/question. How should breaking changes like this be handled once 1.0 is reached?
In kit we did add logging for some required changes, should the adapter read existing wrangler.toml and warn if it is not type module?

dominikg · 2022-03-10T21:17:21Z

this could also help adding DO support: see #1712 (comment)

benmccann · 2022-03-21T21:15:11Z

@dominikg it looks like this PR will need a rebase

…eltejs#4276 (review)

dominikg · 2022-03-21T21:38:47Z

@dominikg it looks like this PR will need a rebase

done, thx.

isaac-mcfadyen · 2022-03-23T13:36:03Z

BTW this should also fix #2844

That should already be closed... unless I'm misunderstanding what you mean by environment the current adapter-cloudflare-workers has the environment variables as global variables (because that's what the Service Workers format does).

pzuraq · 2022-04-14T12:30:09Z

packages/adapter-cloudflare-workers/index.js

-				platform: 'browser'
+				platform: 'browser',
+				bundle: true,
+				external: ['__STATIC_CONTENT_MANIFEST'],


One thing I'd like to see from #4576 is the ability to pass in esbuild options, which is something adapter-cloudflare supports. We occasionally need to mark external dependencies for legacy libraries that reference Node utils, or provide polyfills.

added and rebased. cf adapter allows overriding platform and target so it is allowed here too or should it be fixed for workers?

…ror handling

…eltejs#4276 (review)

rauleite · 2022-04-15T21:14:50Z

I had to add this line too, in [build.upload]:

dir = "./.cloudflare/worker"

Otherwise it will try to use the default "./dist" directory.

dominikg requested a review from lukeed March 8, 2022 21:52

vercel bot deployed to Preview – kit-svelte-dev March 8, 2022 21:52 View deployment

vercel bot deployed to Preview – kit March 8, 2022 21:52 View deployment

vercel bot deployed to Preview – kit-svelte-dev March 8, 2022 23:23 View deployment

vercel bot deployed to Preview – kit March 8, 2022 23:23 View deployment

dominikg added the breaking change label Mar 8, 2022

Rich-Harris reviewed Mar 9, 2022

View reviewed changes

hmnd suggested changes Mar 10, 2022

View reviewed changes

dominikg added a commit to dominikg/kit that referenced this pull request Mar 10, 2022

fix: update adapter-cloudflare-workers to build module package see sv…

1aa65db

…eltejs#4276 (review)

vercel bot deployed to Preview – kit March 10, 2022 20:32 View deployment

vercel bot deployed to Preview – kit-svelte-dev March 10, 2022 20:32 View deployment

dominikg commented Mar 10, 2022

View reviewed changes

Rich-Harris mentioned this pull request Mar 11, 2022

make manifest.mimeTypes part of the public API #4302

Merged

5 tasks

dominikg added a commit to dominikg/kit that referenced this pull request Mar 21, 2022

fix: update adapter-cloudflare-workers to build module package see sv…

b1ff9fe

…eltejs#4276 (review)

dominikg force-pushed the refactor/adapter-cloudflare-workers branch from 1aa65db to b1ff9fe Compare March 21, 2022 21:37

vercel bot deployed to Preview – kit-svelte-dev March 21, 2022 21:37 View deployment

vercel bot deployed to Preview – kit March 21, 2022 21:37 View deployment

dominikg mentioned this pull request Mar 31, 2022

fix: adapter-cloudflare-workers prerendering bug [w/ reproduction] #4467

Closed

5 tasks

dominikg mentioned this pull request Apr 9, 2022

Update and modernize adapter-cloudflare-workers #4576

Closed

5 tasks

vercel bot deployed to Preview – kit April 13, 2022 18:35 View deployment

vercel bot deployed to Preview – kit-svelte-dev April 13, 2022 18:35 View deployment

pzuraq reviewed Apr 14, 2022

View reviewed changes

dominikg and others added 6 commits April 14, 2022 17:48

refactor: use Module Worker pattern instead of Service Worker, add er…

75b3280

…ror handling

chore: add changeset

06197a6

chore: improve changeset

f102255

fix: update adapter-cloudflare-workers to build module package see sv…

3e34bb0

…eltejs#4276 (review)

more validation

f0c820b

feat: allow custom esbuild options for workers adapter

9b70922

dominikg force-pushed the refactor/adapter-cloudflare-workers branch from bf9e478 to 9b70922 Compare April 14, 2022 15:49

vercel bot deployed to Preview – kit April 14, 2022 15:49 View deployment

vercel bot deployed to Preview – kit-svelte-dev April 14, 2022 15:49 View deployment

Rich-Harris approved these changes Apr 15, 2022

View reviewed changes

Rich-Harris merged commit 6d67173 into sveltejs:master Apr 15, 2022

Rich-Harris mentioned this pull request Apr 15, 2022

Fix adapter-cloudflare-workers prerendering bug #4626

Merged

5 tasks

github-actions bot mentioned this pull request Apr 15, 2022

Version Packages (next) #4632

Merged

dominikg mentioned this pull request Apr 15, 2022

adapters shouldn't pass arbitrary options to esbuild #4639

Merged

5 tasks

ghostdevv mentioned this pull request Jun 8, 2023

adapter-cloudflare-workers: Throws KVError if asked for a nonexisting asset #9288

Closed

eltigerchino mentioned this pull request Dec 20, 2023

fix(adapter-cloudflare-workers): throws status 404 if KVError instanceof #9509

Closed

5 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor: use Module Worker pattern instead of Service Worker, add error handling #4276

refactor: use Module Worker pattern instead of Service Worker, add error handling #4276

dominikg commented Mar 8, 2022

changeset-bot bot commented Mar 8, 2022 •

edited

Loading

dominikg commented Mar 8, 2022

Rich-Harris Mar 9, 2022

dominikg Mar 9, 2022

lukeed Mar 10, 2022

Rich-Harris Mar 10, 2022

hmnd Mar 10, 2022 •

edited

Loading

dominikg Mar 12, 2022

Rich-Harris Mar 14, 2022

pzuraq Apr 9, 2022

Rich-Harris Apr 11, 2022

pzuraq Apr 11, 2022

hmnd commented Mar 10, 2022

hmnd left a comment •

edited

Loading

dominikg commented Mar 10, 2022

dominikg Mar 10, 2022

hmnd Mar 10, 2022

dominikg commented Mar 10, 2022

dominikg commented Mar 10, 2022

benmccann commented Mar 21, 2022

dominikg commented Mar 21, 2022

isaac-mcfadyen commented Mar 23, 2022 •

edited

Loading

pzuraq Apr 14, 2022

dominikg Apr 14, 2022

rauleite commented Apr 15, 2022 •

edited

Loading

refactor: use Module Worker pattern instead of Service Worker, add error handling #4276

refactor: use Module Worker pattern instead of Service Worker, add error handling #4276

Conversation

dominikg commented Mar 8, 2022

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

Tests

Changesets

changeset-bot bot commented Mar 8, 2022 • edited Loading

🦋 Changeset detected

dominikg commented Mar 8, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmnd Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hmnd commented Mar 10, 2022

hmnd left a comment • edited Loading

Choose a reason for hiding this comment

dominikg commented Mar 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominikg commented Mar 10, 2022

dominikg commented Mar 10, 2022

benmccann commented Mar 21, 2022

dominikg commented Mar 21, 2022

isaac-mcfadyen commented Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rauleite commented Apr 15, 2022 • edited Loading

changeset-bot bot commented Mar 8, 2022 •

edited

Loading

hmnd Mar 10, 2022 •

edited

Loading

hmnd left a comment •

edited

Loading

isaac-mcfadyen commented Mar 23, 2022 •

edited

Loading

rauleite commented Apr 15, 2022 •

edited

Loading