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

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

Merged

Conversation

dominikg
Copy link
Member

@dominikg dominikg commented Mar 8, 2022

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

@dominikg dominikg requested a review from lukeed March 8, 2022 21:52
@changeset-bot
Copy link

changeset-bot bot commented Mar 8, 2022

🦋 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
Copy link
Member Author

dominikg commented Mar 8, 2022

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.

Comment on lines 79 to 87
{
ASSET_NAMESPACE: env.__STATIC_CONTENT,
ASSET_MANIFEST: static_asset_manifest
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

@hmnd hmnd Mar 10, 2022

Choose a reason for hiding this comment

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

@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 ;)

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

What kind of fingerprint?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Screen Shot 2022-04-11 at 10 26 19 AM

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
Copy link
Contributor

hmnd commented Mar 10, 2022

BTW this should also fix #2844

Copy link
Contributor

@hmnd hmnd left a comment

Choose a reason for hiding this comment

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

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.

@dominikg
Copy link
Member Author

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.

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

// @ts-ignore
const main_file = wrangler_config.build?.upload?.main;
Copy link
Member Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

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

@benmccann
Copy link
Member

@dominikg it looks like this PR will need a rebase

@dominikg
Copy link
Member Author

@dominikg it looks like this PR will need a rebase

done, thx.

@isaac-mcfadyen
Copy link
Contributor

isaac-mcfadyen commented Mar 23, 2022

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

platform: 'browser'
platform: 'browser',
bundle: true,
external: ['__STATIC_CONTENT_MANIFEST'],
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

@rauleite
Copy link

rauleite commented Apr 15, 2022

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

dir = "./.cloudflare/worker"

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

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.

9 participants