Skip to content

Commit

Permalink
fix: remove __STATIC_CONTENT_MANIFEST from module worker env (#4505)
Browse files Browse the repository at this point in the history
  • Loading branch information
mrbbot authored Nov 27, 2023
1 parent 04a2d0e commit 1b34878
Show file tree
Hide file tree
Showing 5 changed files with 42 additions and 22 deletions.
7 changes: 7 additions & 0 deletions .changeset/soft-chairs-share.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
---
"miniflare": patch
---

fix: remove `__STATIC_CONTENT_MANIFEST` from module worker `env`

When using Workers Sites with a module worker, the asset manifest must be imported from the `__STATIC_CONTENT_MANIFEST` virtual module. Miniflare provided this module, but also erroneously added `__STATIC_CONTENT_MANIFEST` to the `env` object too. Whilst this didn't break anything locally, it could cause users to develop Workers that ran locally, but not when deployed. This change ensures `env` doesn't contain `__STATIC_CONTENT_MANIFEST`.
27 changes: 19 additions & 8 deletions packages/miniflare/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -62,7 +62,6 @@ import {
getDirectSocketName,
getGlobalServices,
kProxyNodeBinding,
maybeGetSitesManifestModule,
normaliseDurableObject,
} from "./plugins";
import {
Expand Down Expand Up @@ -104,6 +103,7 @@ import {
LogLevel,
Mutex,
SharedHeaders,
SiteBindings,
} from "./workers";
import { _formatZodError } from "./zod-format";

Expand Down Expand Up @@ -996,6 +996,7 @@ export class Miniflare {
for (let i = 0; i < allWorkerOpts.length; i++) {
const workerOpts = allWorkerOpts[i];
const workerName = workerOpts.core.name ?? "";
const isModulesWorker = Boolean(workerOpts.core.modules);

// Collect all bindings from this worker
const workerBindings: Worker_Binding[] = [];
Expand All @@ -1007,7 +1008,23 @@ export class Miniflare {
const pluginBindings = await plugin.getBindings(workerOpts[key], i);
if (pluginBindings !== undefined) {
for (const binding of pluginBindings) {
workerBindings.push(binding);
// If this is the Workers Sites manifest, we need to add it as a
// module for modules workers. For all other bindings, and in
// service workers, just add to worker bindings.
if (
key === "kv" &&
binding.name === SiteBindings.JSON_SITE_MANIFEST &&
isModulesWorker
) {
assert("json" in binding && binding.json !== undefined);
additionalModules.push({
name: SiteBindings.JSON_SITE_MANIFEST,
text: binding.json,
});
} else {
workerBindings.push(binding);
}

// Only `workerd` native bindings need to be proxied, the rest are
// already supported by Node.js (e.g. json, text/data blob, wasm)
if (isNativeTargetBinding(binding)) {
Expand All @@ -1032,12 +1049,6 @@ export class Miniflare {
}
}
}

if (key === "kv") {
// Add "__STATIC_CONTENT_MANIFEST" module if sites enabled
const module = maybeGetSitesManifestModule(pluginBindings);
if (module !== undefined) additionalModules.push(module);
}
}
}

Expand Down
1 change: 0 additions & 1 deletion packages/miniflare/src/plugins/kv/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -146,5 +146,4 @@ export const KV_PLUGIN: Plugin<
},
};

export { maybeGetSitesManifestModule } from "./sites";
export { KV_PLUGIN_NAME };
13 changes: 1 addition & 12 deletions packages/miniflare/src/plugins/kv/sites.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import assert from "assert";
import fs from "fs/promises";
import path from "path";
import SCRIPT_KV_SITES from "worker:kv/sites";
import { Service, Worker_Binding, Worker_Module } from "../../runtime";
import { Service, Worker_Binding } from "../../runtime";
import { globsToRegExps } from "../../shared";
import {
SharedBindings,
Expand Down Expand Up @@ -102,17 +102,6 @@ export async function getSitesNodeBindings(
};
}

export function maybeGetSitesManifestModule(
bindings: Worker_Binding[]
): Worker_Module | undefined {
for (const binding of bindings) {
if (binding.name === SiteBindings.JSON_SITE_MANIFEST) {
assert("json" in binding && binding.json !== undefined);
return { name: SiteBindings.JSON_SITE_MANIFEST, text: binding.json };
}
}
}

export function getSitesServices(options: SitesOptions): Service[] {
// `siteRegExps` should've been set in `getSitesBindings()`, and `options`
// should be the same object reference as before.
Expand Down
16 changes: 15 additions & 1 deletion packages/miniflare/test/fixtures/sites/modules.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,22 @@ import { getAssetFromKV } from "@cloudflare/kv-asset-handler";
import manifestJSON from "__STATIC_CONTENT_MANIFEST";
const manifest = JSON.parse(manifestJSON);

export default <ExportedHandler<{ __STATIC_CONTENT: KVNamespace }>>{
export default <
ExportedHandler<{
__STATIC_CONTENT: KVNamespace;
__STATIC_CONTENT_MANIFEST?: undefined;
}>
>{
async fetch(request, env, ctx) {
if (
"__STATIC_CONTENT_MANIFEST" in env ||
env.__STATIC_CONTENT_MANIFEST !== undefined
) {
return new Response(
"Expected __STATIC_CONTENT_MANIFEST to be undefined",
{ status: 500 }
);
}
return await getAssetFromKV(
{
request,
Expand Down

0 comments on commit 1b34878

Please sign in to comment.