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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions .changeset/few-walls-obey.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
---
'@sveltejs/adapter-cloudflare-workers': patch
---

[Breaking] refactor implementation from "Service Worker" pattern to "Module Worker" used in adapter-cloudflare

#### add the following to your wrangler.toml
```toml
[build.upload]
format = "modules"
main = "./worker.mjs"
```
8 changes: 6 additions & 2 deletions packages/adapter-cloudflare-workers/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,6 @@ Then configure your sites build directory and your account-details in the config
```toml
account_id = 'YOUR ACCOUNT_ID'
zone_id = 'YOUR ZONE_ID' # optional, if you don't specify this a workers.dev subdomain will be used.
site = {bucket = "./build", entry-point = "./workers-site"}

type = "javascript"

Expand All @@ -62,7 +61,12 @@ type = "javascript"
command = ""

[build.upload]
format = "service-worker"
format = "modules"
main = "./worker.mjs"

[site]
bucket = "./.cloudflare/assets"
entry-point = "./.cloudflare/worker"
```

It's recommended that you add the `build` and `workers-site` folders (or whichever other folders you specify) to your `.gitignore`.
Expand Down
8 changes: 3 additions & 5 deletions packages/adapter-cloudflare-workers/ambient.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,9 +9,7 @@ declare module 'MANIFEST' {
export const prerendered: Set<string>;
}

declare abstract class FetchEvent extends Event {
readonly request: Request;
respondWith(promise: Response | Promise<Response>): void;
passThroughOnException(): void;
waitUntil(promise: Promise<any>): void;
declare module '__STATIC_CONTENT_MANIFEST' {
const json: string;
export default json;
}
122 changes: 81 additions & 41 deletions packages/adapter-cloudflare-workers/files/entry.js
Original file line number Diff line number Diff line change
@@ -1,61 +1,101 @@
import { Server } from 'SERVER';
import { manifest, prerendered } from 'MANIFEST';
import { getAssetFromKV } from '@cloudflare/kv-asset-handler';
import static_asset_manifest_json from '__STATIC_CONTENT_MANIFEST';
const static_asset_manifest = JSON.parse(static_asset_manifest_json);

const server = new Server(manifest);

const prefix = `/${manifest.appDir}/`;

addEventListener('fetch', (/** @type {FetchEvent} */ event) => {
event.respondWith(handle(event));
});
export default {
/**
* @param {Request} req
* @param {any} env
* @param {any} context
*/
async fetch(req, env, context) {
const url = new URL(req.url);

/**
* @param {FetchEvent} event
* @returns {Promise<Response>}
*/
async function handle(event) {
const { request } = event;

const url = new URL(request.url);

// generated assets
if (url.pathname.startsWith(prefix)) {
const res = await getAssetFromKV(event);
return new Response(res.body, {
headers: {
'cache-control': 'public, immutable, max-age=31536000',
'content-type': res.headers.get('content-type')
// static assets
if (url.pathname.startsWith(prefix)) {
/** @type {Response} */
const res = await get_asset_from_kv(req, env, context);
if (is_error(res.status)) {
return res;
}
});
}
return new Response(res.body, {
headers: {
// include original cache headers, minus cache-control which
// is overridden, and etag which is no longer useful
'cache-control': 'public, immutable, max-age=31536000',
'content-type': res.headers.get('content-type'),
'x-robots-tag': 'noindex'
}
});
}

// prerendered pages and index.html files
const pathname = url.pathname.replace(/\/$/, '');
let file = pathname.substring(1);
// prerendered pages and index.html files
const pathname = url.pathname.replace(/\/$/, '');
let file = pathname.substring(1);

try {
file = decodeURIComponent(file);
} catch (err) {
// ignore
}
try {
file = decodeURIComponent(file);
} catch (err) {
// ignore
}

if (
manifest.assets.has(file) ||
manifest.assets.has(file + '/index.html') ||
prerendered.has(pathname || '/')
) {
return get_asset_from_kv(req, env, context);
}

if (
manifest.assets.has(file) ||
manifest.assets.has(file + '/index.html') ||
prerendered.has(pathname || '/')
) {
return await getAssetFromKV(event);
// dynamically-generated pages
try {
return await server.respond(req, {
platform: { env, context },
getClientAddress() {
return req.headers.get('cf-connecting-ip');
}
});
} catch (e) {
return new Response('Error rendering route: ' + (e.message || e.toString()), { status: 500 });
}
}
};

// dynamically-generated pages
/**
* @param {Request} req
* @param {any} env
* @param {any} context
*/
async function get_asset_from_kv(req, env, context) {
try {
return await server.respond(request, {
getClientAddress() {
return request.headers.get('cf-connecting-ip');
return await getAssetFromKV(
{
request: req,
waitUntil(promise) {
return context.waitUntil(promise);
}
},
{
ASSET_NAMESPACE: env.__STATIC_CONTENT,
ASSET_MANIFEST: static_asset_manifest
}
});
);
} catch (e) {
return new Response('Error rendering route:' + (e.message || e.toString()), { status: 500 });
const status = is_error(e.status) ? e.status : 500;
return new Response(e.message || e.toString(), { status });
}
}

/**
* @param {number} status
* @returns {boolean}
*/
function is_error(status) {
return status > 399;
}
58 changes: 52 additions & 6 deletions packages/adapter-cloudflare-workers/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,19 +6,22 @@ import toml from '@iarna/toml';
import { fileURLToPath } from 'url';

/** @type {import('.')} */
export default function () {
export default function (options = {}) {
return {
name: '@sveltejs/adapter-cloudflare-workers',

async adapt(builder) {
const { site } = validate_config(builder);
const { site, build } = validate_config(builder);

// @ts-ignore
const { bucket } = site;

// @ts-ignore
const entrypoint = site['entry-point'] || 'workers-site';

// @ts-ignore
const main_path = build.upload.main;

const files = fileURLToPath(new URL('./files', import.meta.url).href);
const tmp = builder.getBuildDirectory('cloudflare-workers-tmp');

Expand Down Expand Up @@ -50,14 +53,20 @@ export default function () {
);

await esbuild.build({
target: 'es2020',
platform: 'browser',
...options,
entryPoints: [`${tmp}/entry.js`],
outfile: `${entrypoint}/index.js`,
outfile: `${entrypoint}/${main_path}`,
bundle: true,
target: 'es2020',
platform: 'browser'
external: ['__STATIC_CONTENT_MANIFEST', ...(options?.external || [])],
format: 'esm'
});

writeFileSync(`${entrypoint}/package.json`, JSON.stringify({ main: 'index.js' }));
writeFileSync(
`${entrypoint}/package.json`,
JSON.stringify({ main: main_path, type: 'module' })
);

builder.log.minor('Copying assets...');
builder.writeClient(bucket);
Expand Down Expand Up @@ -86,6 +95,36 @@ function validate_config(builder) {
);
}

// @ts-ignore
if (!wrangler_config.build || !wrangler_config.build.upload) {
throw new Error(
'You must specify build.upload options in wrangler.toml. Consult https://github.com/sveltejs/kit/tree/master/packages/adapter-cloudflare-workers'
);
}

// @ts-ignore
if (wrangler_config.build.upload.format !== 'modules') {
throw new Error('build.upload.format in wrangler.toml must be "modules"');
}

// @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.

const main_file_ext = main_file?.split('.').slice(-1)[0];
if (main_file_ext && main_file_ext !== 'mjs') {
// @ts-ignore
const upload_rules = wrangler_config.build?.upload?.rules;
// @ts-ignore
const matching_rule = upload_rules?.find(({ globs }) =>
// @ts-ignore
globs.find((glob) => glob.endsWith(`*.${main_file_ext}`))
);
if (!matching_rule) {
throw new Error(
'To support a build.upload.main value not ending in .mjs, an upload rule must be added to build.upload.rules. Consult https://developers.cloudflare.com/workers/cli-wrangler/configuration/#build'
);
}
}

return wrangler_config;
}

Expand All @@ -104,6 +143,13 @@ function validate_config(builder) {
route = ""
zone_id = ""

[build]
command = ""

[build.upload]
format = "modules"
main = "./worker.mjs"

[site]
bucket = "./.cloudflare/assets"
entry-point = "./.cloudflare/worker"`
Expand Down