new config miniflare attempt 3 - #14994
Conversation
🦋 Changeset detectedLatest commit: 73ec8a5 The changes in this PR will be included in the next version bump. This PR includes changesets to release 10 packages
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 |
|
UnknownError: ProviderInitError |
|
@emily-shen Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
✅ All changesets look good |
17612e4 to
1c14de0
Compare
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/config
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-pool-workers
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
| enable_timer: z.boolean().optional(), | ||
| }); | ||
|
|
||
| const MiniflareWorkflowBindingSchema = z.strictObject({ |
There was a problem hiding this comment.
leaving this in the "old" env only format for now
8612a3c to
e4c796b
Compare
|
Codeowners approval required for this PR:
Show detailed file reviewers
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
e4c796b to
e8514a8
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
45dbce5 to
9a7db84
Compare
edmundhung
added
ci:test-all-node-versions
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
penalosa
left a comment
penalosa
left a comment
There was a problem hiding this comment.
Some AI comments that seem valid:
-
Compatibility date default
convertV4MiniflareOptions() defaults an omitted compatibility date to 2023-07-24, whereas Miniflare previously defaulted to 2000-01-01.
This silently enables compatibility-date-gated runtime behaviour for existing callers that omit compatibilityDate. -
Dev registry registration default
unsafeRegisterWorker previously defaulted to true. The converter now leaves it undefined, which the new registration check treats as false.
Named workers using the dev registry without explicitly setting this option will no longer be advertised to other Miniflare instances. -
Assets configuration
The converter drops supported fields from assets.routerConfig and assets.assetConfig, including existing static routing rules and inline headers/redirects.
These values were previously passed to the assets workers, so existing asset routing and response behaviour may change. -
Workflow compatibility flags
The v4 schema and converter omit each workflow’s compatibilityFlags. The workflow plugin instead applies the user Worker’s compatibility flags to every workflow engine.
This both removes explicitly configured workflow flags and introduces unrelated Worker flags into the engine. -
Manual module path resolution
Relative paths in manually supplied modules were previously resolved against rootPath; modulesRoot only determined their runtime module names.
The converter now reads those files relative to modulesRoot, which can load the wrong file or fail with ENOENT whenever rootPath and modulesRoot differ. -
Container egress image
The converter forwards containerEgressInterceptorImage, but the new Miniflare schema does not include it. Final validation therefore strips the field.
Callers specifying a custom egress image silently receive the default image instead.
The dropped fields are now resolved once in miniflare instead of separately by consumers.
the workflow compat flags are the user compat flags, just previously the plugin didn't have access to those options so they were copied over.
devin had the same comment, i still don't really understand what the problem is here. what is the expected user facing breakage due to this change? fixing the rest |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Example project: new Miniflare({
rootPath: "/project",
modulesRoot: "src",
modules: [{ type: "ESModule", path: "src/index.mjs" }],
});Previously:
Now:
|
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
Some findings from AI review 😅
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
edmundhung
left a comment
edmundhung
left a comment
There was a problem hiding this comment.
We are getting there! Thanks for the hard work. 🙌🏼
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
c60a2f5 to
73ec8a5
Compare
| const MiniflareBrowserBindingSchema = BrowserBindingSchema.extend({ | ||
| headful: z.boolean().optional(), | ||
| }); | ||
|
|
||
| /** | ||
| * `s3Credentials` is a local-dev-only field (used to expose the bucket via the | ||
| * S3-compatible endpoint), so it lives here rather than in the shared config | ||
| * schema. The credentials shape is inlined (rather than a named schema) to keep | ||
| * it out of the bundled public API surface; consumers derive the type from the | ||
| * R2 binding via `Extract<MiniflareBinding, { type: "r2" }>`. | ||
| */ | ||
| const MiniflareR2BindingSchema = R2BindingSchema.extend({ | ||
| s3Credentials: z | ||
| // Allow internal source metadata used when checking duplicate credentials. | ||
| .object({ | ||
| accessKeyId: z.string(), | ||
| secretAccessKey: z.string(), | ||
| }) | ||
| .optional() satisfies z.ZodType<S3Credentials | undefined>, | ||
| }); | ||
|
|
||
| const MiniflareHyperdriveBindingSchema = HyperdriveBindingSchema.omit({ | ||
| localConnectionString: true, | ||
| }).extend({ localConnectionString: z.string() }); |
There was a problem hiding this comment.
Are these definitely all right? It seems like some could be in the config schema itself, in e.g. a dev block (localConnectionString is today, for instance). Not going to block on this though
| enable_timer: z.boolean().optional(), | ||
| }); | ||
|
|
||
| const MiniflareWorkflowBindingSchema = z.strictObject({ |
| /** Whether this Worker is 'public' - whether should be advertised in the dev registry | ||
| * and whether it should be included in local obs capture. Defaults to `true`. */ | ||
| unsafeRegisterWorker: z.boolean().default(true), | ||
| hasAssetsAndIsVitest: z.boolean().optional(), |
There was a problem hiding this comment.
In a followup we should remove this
| .optional(), | ||
| unsafeTriggerHandlers: z.boolean().optional(), | ||
| unsafeRuntimeEnv: z.record(z.string(), z.string()).optional(), | ||
| unsafeLocalExplorer: z.boolean().optional(), |
There was a problem hiding this comment.
We should make this always on in a followup
| script: "export default {};", | ||
| assets: { | ||
| directory: "./public", | ||
| run_worker_first: ["/api/*", "!/api/asset"], |
There was a problem hiding this comment.
This test masks a conversion gap by supplying both representations. convertV4MiniflareOptions() reads run_worker_first but ignores routerConfig.static_routing, so removing this line changes the result even though v4 previously honoured static_routing. Please test static_routing on its own and either convert it or reject it explicitly rather than silently losing the routing rules.
| )})`; | ||
| } | ||
|
|
||
| // Rewrite a source map's `sources` to absolute paths. esbuild emits `sources` |
There was a problem hiding this comment.
This rewrites the input to work around the new source-map anchoring rather than testing normal esbuild output. Relative sources are the common case, and production is supposed to resolve them against the map location. Making them absolute allows broken manifest/source-map anchoring to pass. Can we keep the emitted map unchanged, fix the production resolution, and restore the inspector assertion for this manifest-provided map?
| }, | ||
| }, | ||
| exports: { | ||
| Counter: { type: "durable-object", storage: "sqlite" }, |
There was a problem hiding this comment.
The base test omitted useSQLite, which selected legacy-KV storage; this changes it to SQLite. The same switch occurs across the generic DO and dev-registry tests, removing legacy-KV coverage for persistence, reload, eviction, cross-worker access, RPC and restart paths. Backend-neutral migrations should use storage: "legacy-kv"; SQLite should remain explicit only where the test requires it.
| config: { | ||
| type: "worker", | ||
| name: "producer", | ||
| compatibilityDate: "2025-05-01", |
There was a problem hiding this comment.
The base test used Miniflare's 2000-01-01 fallback. This date enables queues_json_messages, so the migrated batching/retry/dead-letter/delay tests now use JSON rather than the V8 structured-clone path they previously exercised. Please preserve 2000-01-01 for parity, or deliberately parameterise the queue suites across both serialisation modes.
| config: { | ||
| type: "worker", | ||
| name: "", | ||
| compatibilityDate: "2025-05-01", |
There was a problem hiding this comment.
This changes the suite's effective date from the old 2000-01-01 fallback to 2025. That enables r2_list_honor_include, so the list() metadata tests now exercise different behaviour and legacy include semantics lose coverage. Please retain the old date for the mechanical migration or add explicit coverage for both modes.
| @@ -143,56 +200,96 @@ test("Miniflare: validates options", async ({ expect, onTestFinished }) => { | |||
|
|
|||
| test("Miniflare: accepts mixed r2Buckets record", () => { | |||
There was a problem hiding this comment.
This test no longer exercises a mixed r2Buckets record; it constructs two already-normalised v5 bindings. The KV, D1 and pipeline tests immediately below have the same problem. Please move the original mixed shorthand/object cases to converter tests and assert the converted output, otherwise these names claim compatibility coverage that no longer exists.
| name: "", | ||
| compatibilityDate: "2025-05-01", | ||
| }, | ||
| dev: { rootPath: path.join(tmp, "a") }, |
There was a problem hiding this comment.
This is now absolute, so the test no longer exercises the relative-root double-resolution regression described directly above it; changing CWD is also no longer material. Please pass the relative v4 input through the converter and reload that result, or replace this with a focused converter integration test that can still fail on double resolution.
| serviceWorkerScriptPath: serviceWorkerPath, | ||
| }, | ||
| }, | ||
| // Module workers with co-located source maps on disk. |
There was a problem hiding this comment.
The source-map matrix has narrowed substantially here. b and c now have effectively identical configurations, while the former path-only, inline-content, explicit-modulesRoot, and auto-collected variants (d–g) and several inspector assertions disappeared. Some v4 forms are intentionally removed, but the supported path/manifest conversions still need end-to-end source-map coverage rather than duplicate cases.
| name: "other", | ||
| modules: true, | ||
| scriptPath: "./src/other-worker.mjs", | ||
| durableObjects: { |
There was a problem hiding this comment.
The Wrangler fixture declares OtherObject in new_sqlite_classes, but this separate owning worker omits useSQLite: true. The converter creates the export from the owner, where omission maps to legacy-kv, so this fixture says SQLite while actually running legacy KV. Please propagate the storage choice to the owner and assert SQL availability so the mismatch is detectable.
| contents: readFileSync(path.resolve(rootPath, modulePath), "utf8"), | ||
| }; | ||
| } | ||
| return { mainModule: modulePaths[0], modules }; |
There was a problem hiding this comment.
The old setup supplied modulesRoot: helper.tmpPath; this manifest omits it, so the schema defaults to repository CWD even though contents are read from rootPath. Imports happen to resolve by manifest key, but source URLs, stack traces and source-map lookup are now anchored differently. Please include modulesRoot: path.resolve(rootPath) and ideally assert an imported module's source path.
penalosa
left a comment
penalosa
left a comment
There was a problem hiding this comment.
A second pass over the non-test changes found these configuration and runtime gaps. I have omitted the expected module-discovery change and the issues already covered in my earlier review.
| { | ||
| name: "dataset", | ||
| json: JSON.stringify(config.dataset), | ||
| json: JSON.stringify(binding.name), |
There was a problem hiding this comment.
name is optional in the accepted Analytics Engine binding schema, so this can evaluate to undefined and produce an inner dataset binding with no value. I verified that { env: { AE: { type: "analytics-engine-dataset" } } } then fails workerd startup with binding "dataset" does not specify any binding value. Should this use JSON.stringify(binding.name ?? name)?
| } | ||
| // Otherwise, build modules from the manifest (contents are provided inline). | ||
| const manifest = config.manifest; | ||
| assert(manifest !== undefined, "Unreachable: Workers must have code"); |
There was a problem hiding this comment.
The new schema permits a worker with no manifest, and assets.hasUserWorker explicitly supports assets-only workers. A direct assets-only config therefore passes validation and reaches this assertion. Wrangler happens to inject a placeholder script, but direct Miniflare callers should either get the same placeholder behaviour or a schema validation error rather than an internal assertion.
| workerName: z.string(), | ||
| exportName: z.string(), | ||
| limits: z.strictObject({ steps: z.number().optional() }).optional(), | ||
| remote: z.boolean().optional(), |
There was a problem hiding this comment.
My understanding is that remote Workflow bindings are not supported. This schema nevertheless accepts remote: true, and the converter also produces it. For an external target the workflows service then references a dev-registry proxy service that is deliberately not registered for remote bindings, causing startup failure. If remote Workflows are unsupported, can we reject/remove this field at validation instead?
| remoteProxyConnectionString: RemoteProxyConnectionStringSchema.optional(), | ||
| }); | ||
|
|
||
| const V4WorkerOptionsShapeSchema = z.object({ |
There was a problem hiding this comment.
These compatibility schemas are non-strict, so unsupported v4 options can be silently stripped before the converter has a chance to throw. A concrete example is unsafeExcludeFromObservability: it was accepted and honoured by v4, is absent here, and conversion succeeds with observability behaviour changed. This also contradicts the converter changeset's statement that unsupported options throw. Can we make this strict or explicitly recognise and reject removed options?
| contents: z.union([z.string(), z.instanceof(Uint8Array)]), | ||
| }); | ||
|
|
||
| export const MiniflareManifestSchema = z.strictObject({ |
There was a problem hiding this comment.
Can this schema refine that mainModule exists in modules and is not a sourcemap entry? Both inputs currently pass public validation and later hit internal assertions in core/module conversion rather than producing ERR_VALIDATION.
| unsafePreventEviction: z.boolean().optional(), | ||
| container: z.custom<DOContainerOptions>().optional(), | ||
| }); | ||
| export const MiniflareDurableObjectExpectingTransferExportSchema = |
There was a problem hiding this comment.
Worth flagging that transfer metadata is accepted but not consumed: transferFrom is dropped when DO class information is assembled, while renamed/transferred tombstones are filtered below. That means local persistence uses the new ${worker}-${class} namespace rather than following the declared transfer. If local transfer semantics are intentionally out of scope, should these states be rejected or explicitly documented rather than accepted and ignored?
| if (assets.workerName !== undefined) { | ||
| throwUnsupportedOption("assets.workerName"); | ||
| } | ||
| config.assets = { |
There was a problem hiding this comment.
This conversion drops additional v4 asset router metadata that was previously forwarded, including account_id, script_id, and debug. If those fields are intentionally unsupported in v5, can we reject them explicitly rather than silently discard them?
| for (const [bindingName, workflow] of Object.entries( | ||
| worker.workflows ?? {} | ||
| )) { | ||
| if (workflow.external !== undefined) { |
There was a problem hiding this comment.
This rejects both external: true and external: false. In v4 only a truthy value changed routing, so false is behaviourally equivalent to omission and can be preserved. Can this reject only external === true?
|
|
||
| const V4ModuleDefinitionSchema = z.object({ | ||
| type: V4ModuleRuleTypeSchema, | ||
| /** Module file path; relative to `modulesRoot` if not absolute. */ |
There was a problem hiding this comment.
This documentation does not match conversion behaviour. Relative module paths are resolved against rootPath in createManifestFromModules(), not modulesRoot. Following this comment causes the converter to read the wrong file.
|
|
||
| For the most part, users should not expect to notice any changes. | ||
|
|
||
| However, while `miniflare.modulesRules` is preserved for common text and WASM fixture imports, it is not a full replacement for Miniflare's old `modules: true` module graph collection and you may notice some differences in behaviour. |
There was a problem hiding this comment.
This understates the impact for auxiliary miniflare.workers. They are passed through the converter with neither a complete discovered graph nor module fallback enabled, so an auxiliary worker with an ordinary relative local import fails to start rather than merely behaving slightly differently. Can this call out the breaking configuration explicitly, and ideally provide the required explicit-module migration?
5 participants
Config:
All plugins now get the full config, and are responsible for filtering down the relevant config themselves - previously miniflare plugins each declared a schema of what config it needed, and then each plugin would only receive that.
There is probably a lot of tidying up we can do in miniflare to strip out more unused bits, but trying to avoid too much in this PR.
A large chunk of the diff is in tests, where we call
new Miniflarea lot. Inside miniflare this is migrated to the new config directly, outside miniflare we use the conversion helper.As discussed the v4 to v5 conversion happens right before passing options to miniflare, meaning the new config format is not exposed in any public APIs (well other than miniflare's obviously).
TODO later:
there are a couple of exports from miniflare that are only used by vitest-pool-workers and not miniflare itself. for example, the config merging helper and compileModuleRules. In a follow up i will probably just move that directly into VPW.
A picture of a cute animal (not mandatory, but encouraged)