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

text_blobs/Text module support for service worker format in local mode #735

Merged
merged 1 commit into from
Mar 31, 2022
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.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 7 additions & 0 deletions .changeset/short-cougars-approve.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"wrangler": patch
---

`text_blobs`/Text module support for service worker format in local mode

This adds support for `text_blobs`/Text module support in local mode. Now that https://github.com/cloudflare/miniflare/pull/228 has landed in miniflare (thanks @caass!), we can use that in wrangler as well.
2 changes: 1 addition & 1 deletion packages/wrangler/src/__tests__/configuration.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ describe("normalizeAndValidateConfig()", () => {
expect(diagnostics.hasWarnings()).toBe(false);
});

it("should error on invalid `wasm_module` paths", () => {
it("should error on invalid `wasm_modules` paths", () => {
const expectedConfig = {
wasm_modules: {
MODULE_1: 111,
Expand Down
6 changes: 3 additions & 3 deletions packages/wrangler/src/__tests__/publish.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -732,7 +732,7 @@ export default{
expect(std.err).toMatchInlineSnapshot(`""`);
});

it("when using a service worker type, it should add an asset manifest as a text_blob, and bind to a namespace", async () => {
it("when using a service-worker type, it should add an asset manifest as a text_blob, and bind to a namespace", async () => {
const assets = [
{ filePath: "assets/file-1.txt", content: "Content of file-1" },
{ filePath: "assets/file-2.txt", content: "Content of file-2" },
Expand Down Expand Up @@ -2902,7 +2902,7 @@ export default{
expect(std.warn).toMatchInlineSnapshot(`""`);
});

it("should error when detecting service workers implementing durable objects", async () => {
it("should error when detecting a service-worker worker implementing durable objects", async () => {
writeWranglerToml({
durable_objects: {
bindings: [
Expand All @@ -2918,7 +2918,7 @@ export default{

await expect(runWrangler("publish index.js")).rejects
.toThrowErrorMatchingInlineSnapshot(`
"You seem to be trying to use Durable Objects in a Worker written with Service Worker syntax.
"You seem to be trying to use Durable Objects in a Worker written as a service-worker.
You can use Durable Objects defined in other Workers by specifying a \`script_name\` in your wrangler.toml, where \`script_name\` is the name of the Worker that implements that Durable Object. For example:
{ name = EXAMPLE_DO_BINDING, class_name = ExampleDurableObject } ==> { name = EXAMPLE_DO_BINDING, class_name = ExampleDurableObject, script_name = example-do-binding-worker }
Alternatively, migrate your worker to ES Module syntax to implement a Durable Object in this Worker:
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/config/validation.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1140,7 +1140,7 @@ const validateR2Binding: ValidatorFn = (diagnostics, field, value) => {
*
* We don't want to have, for example, a KV namespace named "DATA"
* and a Durable Object also named "DATA". Then it would be ambiguous
* what exactly would live at `env.DATA` (or in the case of service workers,
* what exactly would live at `env.DATA` (or in the case of service-workers,
* the `DATA` global).
*/
const validateBindingsHaveUniqueNames = (
Expand Down
6 changes: 1 addition & 5 deletions packages/wrangler/src/create-worker-upload-form.ts
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ export function toMimeType(type: CfModuleType): string {
export interface WorkerMetadata {
/** The name of the entry point module. Only exists when the worker is in the ES module format */
main_module?: string;
/** The name of the entry point module. Only exists when the worker is in the Service Worker format */
/** The name of the entry point module. Only exists when the worker is in the service-worker format */
body_part?: string;
compatibility_date?: string;
compatibility_flags?: string[];
Expand Down Expand Up @@ -172,10 +172,6 @@ export function createWorkerUploadForm(worker: CfWorkerInit): FormData {
);
// And then remove it from the modules collection
modules = modules?.filter((m) => m !== module);
} else if (module.type === "buffer") {
throw new Error(
'The "buffer" module type is not yet supported in service worker format workers'
);
Comment on lines -175 to -178
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this change specific to this PR? It kind of feels like it is an independent fix? But I might be misunderstanding its importance in terms of text_blobs?

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 just rounds out the story for module system support. With module workers, we can import modules of all the supported types: js, wasm, text, and data. With service-workers, we can import js, wasm, text, but not data. We can detect its usage and throw an error. I simply moved the error to earlier in the pipeline, right when we're building the worker, instead of waiting for when we create the worker form upload.

I don't expect this error to trigger often; there are no service-worker workers right now that import a Data module (because there's no equivalent like wasm_modules or text_blobs for data.) The usecase this will probably trigger is when a third party library imports a Data module, and is used in a service-worker (It'll work fine with a module worker). If that happens, and it's a good/common library, then the thing to do will be to convince the runtime team to add a new type of binding (data_buffers?). We can then leverage that to add support for Data modules for service-workers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just checked internally, and we do have support for a data_blob binding. Filed #740

}
}
}
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/dev/dev.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,7 @@ export function DevImplementation(props: DevProps): JSX.Element {

if (props.public && props.entry.format === "service-worker") {
throw new Error(
"You cannot use the service worker format with a `public` directory."
"You cannot use the service-worker format with a `public` directory."
);
}

Expand Down
10 changes: 10 additions & 0 deletions packages/wrangler/src/dev/local.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,7 @@ function useLocalWorker({
const scriptPath = realpathSync(bundle.path);

const wasmBindings = { ...bindings.wasm_modules };
const textBlobBindings = { ...bindings.text_blobs };
if (format === "service-worker") {
for (const { type, name } of bundle.modules) {
if (type === "compiled-wasm") {
Expand All @@ -91,6 +92,13 @@ function useLocalWorker({
// characters with an underscore.
const identifier = name.replace(/[^a-zA-Z0-9_$]/g, "_");
wasmBindings[identifier] = name;
} else if (type === "text") {
// In service-worker format, text modules are referenced by global identifiers,
// so we convert it here.
// This identifier has to be a valid JS identifier, so we replace all non alphanumeric
// characters with an underscore.
const identifier = name.replace(/[^a-zA-Z0-9_$]/g, "_");
textBlobBindings[identifier] = name;
}
}
}
Expand Down Expand Up @@ -130,6 +138,7 @@ function useLocalWorker({
: undefined,
bindings: bindings.vars,
wasmBindings,
textBlobBindings,
sourceMap: true,
logUnhandledRejections: true,
};
Expand Down Expand Up @@ -227,6 +236,7 @@ function useLocalWorker({
publicDirectory,
rules,
bindings.wasm_modules,
bindings.text_blobs,
]);
return { inspectorUrl };
}
2 changes: 1 addition & 1 deletion packages/wrangler/src/entry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export async function getEntry(

if (format === "service-worker" && localBindings.length > 0) {
const errorMessage =
"You seem to be trying to use Durable Objects in a Worker written with Service Worker syntax.";
"You seem to be trying to use Durable Objects in a Worker written as a service-worker.";
const addScriptName =
"You can use Durable Objects defined in other Workers by specifying a `script_name` in your wrangler.toml, where `script_name` is the name of the Worker that implements that Durable Object. For example:";
const addScriptNameExamples = generateAddScriptNameExamples(localBindings);
Expand Down
7 changes: 6 additions & 1 deletion packages/wrangler/src/module-collection.ts
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ export default function createModuleCollector(props: {
modules.splice(0);
});

// ~ start legacy module specifier support ~
// ~ start legacy module specifier support ~

// This section detects usage of "legacy" 1.x style module specifiers
// and modifies them so they "work" in wrangler v2, but with a warning
Expand Down Expand Up @@ -218,6 +218,11 @@ export default function createModuleCollector(props: {
build.onLoad(
{ filter: globToRegExp(glob) },
async (args: esbuild.OnLoadArgs) => {
if (rule.type === "Data") {
throw new Error(
"Data modules are not supported in the service-worker format"
);
}
return {
// We replace the the module with an identifier
// that we'll separately add to the form upload
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/src/publish.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export default async function publish(props: Props): Promise<void> {
if (props.experimentalPublic && format === "service-worker") {
// TODO: check config too
throw new Error(
"You cannot publish in the service worker format with a public directory."
"You cannot publish in the service-worker format with a public directory."
);
}

Expand Down
4 changes: 2 additions & 2 deletions packages/wrangler/src/worker.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,15 +75,15 @@ interface CfKvNamespace {
}

/**
* A binding to a wasm module (in service worker format)
* A binding to a wasm module (in service-worker format)
*/

interface CfWasmModuleBindings {
[key: string]: string;
}

/**
* A binding to a text blob (in service worker format)
* A binding to a text blob (in service-worker format)
*/

interface CfTextBlobBindings {
Expand Down
2 changes: 1 addition & 1 deletion packages/wrangler/templates/static-asset-facade.js
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export default {
// if an error is thrown then serve from actual worker
return worker.fetch(request);
// TODO: throw here if worker is not available
// (which implies it may be a service worker)
// (which implies it may be a service-worker)
}
},
};