Repository navigation
[miniflare] Preserve binding error codes in local development - #16499
Conversation
Errors from remote bindings and Node.js binding proxies lost custom properties such as Artifacts' `code` and `numericCode`, and had their name folded into the message. Enable `enhanced_error_serialization` on the internal workers that relay these errors, and keep primitive own properties and custom names in the devalue `Error` reducer.
🦋 Changeset detectedLatest commit: 74baf24 The changes in this PR will be included in the next version bump. This PR includes changesets to release 8 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 |
|
Hi @dinasaur404. Thanks for taking the time to contribute to Workers SDK. This PR has been automatically closed because you are not currently a registered (vouched) collaborator for this repository. AI tools now let our team generate proposed code changes quickly. Reviewing changes, checking that they solve the right problem, and maintaining code quality over time still take substantial care and effort. We therefore limit PRs to registered collaborators so we can focus that effort where it is most useful. For the majority of external contributors, the most effective way to help is through issues: describing a problem or use case, providing a minimal reproduction, investigating its cause, or helping us verify a fix. Those contributions help us understand what needs to change and are very welcome. We will take an initial look at this PR and decide whether it is something we want to invest time in reviewing and maintaining. We may reopen it if we decide to proceed, but we cannot guarantee that it will be reopened or receive a full review. |
|
Codeowners approval required for this PR:
Show detailed file reviewers |
|
✅ Approved — No issues found. The flag changes are consistent across the three workers, and the error serialization change is narrow and covered by tests. The decision is advisory: this comment never approves, blocks or requests changes on the pull request. ✅ All checks passed🚀 Release: ✅ LGTMRisk: Low. The changeset describes the combined fix and requests patch releases for the affected packages. 🔒 Security: ✅ LGTMNo security concerns. Revived error properties are limited to primitive values and set with 📖 AGENTS.md: ✅ LGTMThe change follows the repository conventions. 📋 Code Quality: ✅ LGTMThe compatibility-flag additions match across the three internal workers. The devalue ⚡ Performance: ✅ LGTMSerializing an error now loops over its few own property names, which costs very little. The flag changes add nothing to hot paths. 📝 Documentation: ✅ LGTMThe changeset and the code comments describe the change accurately. Reviewed commit: |
@cloudflare/autoconfig
@cloudflare/build-output-utils
@cloudflare/codemods
@cloudflare/config
@cloudflare/containers-shared
create-cloudflare
@cloudflare/deploy-helpers
@cloudflare/kv-asset-handler
miniflare
@cloudflare/pages-functions
@cloudflare/pages-shared
@cloudflare/runtime-types
@cloudflare/unenv-preset
@cloudflare/vite-plugin
@cloudflare/vitest-plugin
@cloudflare/workers-auth
@cloudflare/workers-editor-shared
@cloudflare/workers-utils
wrangler
commit: |
…zation Match workerd's enhanced_error_serialization, which includes all own properties, by using Object.getOwnPropertyNames instead of Object.keys.
workers-devprod
left a comment
There was a problem hiding this comment.
Codeowners reviews satisfied
Errors from Artifacts (and other bindings) lose
codeandnumericCodein local development. Developers can only branch on message text, so code that works in production can fail locally.What was broken
Comparison for the same calls (
get()on a missing repo, invalid name,list({ limit: 0 })):name: "ArtifactsError",code: "NOT_FOUND",numericCode: 10200wrangler devwithremote: true, inside the Workername: "Error",message: "ArtifactsError: Repository not found…", no codecodeandnumericCodekept, clean message,name: "Error"getPlatformProxy()withremote: truecodeandnumericCodekept, clean messageMiniflare#getBindings()with a local bindingname: "ArtifactsError"Why
Three internal workers pass errors between Workers. They use compatibility dates from before workerd's
enhanced_error_serializationbehavior, so they keep only the message text and fold the name into it:packages/remote-bindings/src/start-remote-proxy-session.ts)packages/miniflare/src/plugins/shared/constants.ts)ProxyServerfor Node callers (packages/miniflare/src/plugins/core/index.ts)Miniflare already enables this flag for local D1 for the same reason.
Separately, the devalue
Errorreducer used by the Node proxy (packages/miniflare/src/workers/core/devalue.ts) sent onlyname,message,stackandcause. It now also sends own properties with primitive values (string, number, boolean or null) and a custom error name. It skips non-primitive values so they can't make an error unserialisable. Errors without extra properties keep the old encoding.Known limitation
Inside a Worker, remote-binding errors are still named
Error. capnweb rebuilds unrecognized error types as plainErrorand doesn't restorename. Code should branch oncode. That's fixed separately in cloudflare/capnweb#289. With both changes,remote: trueerrors match production exactly, includingname: "ArtifactsError"; I verified this end to end.Testing
name,codeandnumericCodethrown by an RPC binding. All three behavior tests fail without the source changes.remote-bindings-connect.spec.ts.experimentalis still opt-in only.miniflare: full suite 1,664 passed (3 skipped) after the assertion update.remote-bindings: 31 passed.wranglerremote-bindings andstartDevWorkertests: 160 passed.tsc,oxlint --type-awareandoxfmtclean.wrangler devwithremote: true,getPlatformProxy()andwrangler dev --remote. Results are in the table.