attempt 2 at new miniflare config - #14954
Conversation
|
✅ All changesets look good |
There was a problem hiding this comment.
I'm Bonk, and I've done a quick review of your PR.
Large refactor of miniflare's config format, moving from plugin-namespaced options to a config/dev/legacy structure, plus exporting binding/export schemas from @cloudflare/config. The core index.ts and plugin refactors look semantically faithful. However, there is a build-breaking inconsistency in the @cloudflare/config workflow re-enablement (the head commit is named temp, so this may be a WIP/rebase artifact).
Ranked issues:
- (blocker)
packages/config/src/types.ts:55importsWorkflowExportfrom./exports, butexports.tsdoes not define or exportWorkflowExport(there are zeroworkflowreferences in that file). This is an unresolved type import and will fail the type-check/build. - (correctness) The workflow binding is re-enabled in the TS type layer (
bindings.ts,types.ts) butpackages/config/src/schema.tsstill has theworkflowentry commented out in bothKnownBindingSchemaandExportSchema. Result:bindings.workflow(...)type-checks, but a{ type: "workflow" }binding/export is rejected at runtime by the zod schema (falls throughKnownBindingSchema-> validation error). The type and runtime schema must agree. - (process) None of the changesets mention re-enabling workflow bindings/exports; the two config changesets only cover schema exports. If the workflow re-enablement is intentional it needs a changeset; if it's an accidental rebase artifact it should be reverted.
| DurableObjectRenamedExport, | ||
| DurableObjectTransferredExport, | ||
| WorkerEntrypointExport, | ||
| WorkflowExport, |
There was a problem hiding this comment.
WorkflowExport is imported here but is never defined or exported from ./exports (that file has no workflow references at all). This is an unresolved import and will break the type-check/build. Either add and export WorkflowExport in exports.ts, or drop this import until workflow exports are actually implemented:
| WorkflowExport, | |
| WorkerEntrypointExport, |
(and remove the corresponding | WorkflowExport on line 128).
| | DurableObjectExpectingTransferExport | ||
| | WorkerEntrypointExport; | ||
| | WorkerEntrypointExport | ||
| | WorkflowExport; |
There was a problem hiding this comment.
Depends on WorkflowExport (line 55), which is not defined/exported anywhere. Until WorkflowExport exists in exports.ts this member of the Export union won't compile:
| | WorkflowExport; | |
| | WorkerEntrypointExport; |
| // * `exportName` must be a valid `WorkflowEntrypoint` export for the given Worker. | ||
| // */ | ||
| // workflow(options: WorkflowBindingOptions): WorkflowBinding; | ||
| workflow(options: WorkflowBindingOptions): WorkflowBinding; |
There was a problem hiding this comment.
This re-enables the workflow binding in the type layer, but KnownBindingSchema in packages/config/src/schema.ts still has the workflow variant commented out. So bindings.workflow(...) type-checks, yet a { type: "workflow" } binding is rejected at runtime by zod validation. Please make the schema and types consistent — either re-enable workflow in schema.ts (KnownBindingSchema and ExportSchema) as well, or keep this commented out. If this re-enablement is intentional, it also needs a changeset.
|
Review posted successfully to PR #14954. Summary of what I found and posted:
The core miniflare |
48a3e7b to
d0f1fde
Compare
🦋 Changeset detectedLatest commit: 943ccc4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 9 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 |
Fixes #[insert GH or internal issue link(s)].
Describe your change...
A picture of a cute animal (not mandatory, but encouraged)