fix(client): preserve subclass prototype chain in cloneORPCError - #1801
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
More templates
@orpc/ai-sdk
@orpc/arktype
@orpc/bun
@orpc/client
@orpc/cloudflare
@orpc/contract
@orpc/experimental-effect
@orpc/evlog
@orpc/hibernation
@orpc/json-schema
@orpc/nest
@orpc/next
@orpc/openapi
@orpc/opentelemetry
@orpc/pinia-colada
@orpc/pino
@orpc/publisher
@orpc/ratelimit
@orpc/server
@orpc/shared
@orpc/swr
@orpc/tanstack-query
@orpc/trpc
@orpc/valibot
@orpc/zod
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
ℹ️ Minor suggestions only — the fix is correct and well-tested; one foot-gun worth noting inline.
Reviewed changes
- Prototype-chain preservation in
cloneORPCError— the clone is now built as a realORPCErrorand its prototype is swapped to the original's (Object.setPrototypeOf+Object.definePropertiescopying own descriptors), so subclass instances stayinstanceoftheir class while keeping nativeErrornon-enumerablemessage/stack/causesemantics. - Signature widened from
ORPCError<T, TData>to<T extends AnyORPCError>(error: T): T, with the old explicitdefined/inferablecopy superseded by the descriptor copy (they are own instance properties, so it's covered). - Two new tests: subclass prototype-chain preservation (would fail under the previous implementation) and native-error shape semantics for the base
ORPCError. Both tests are meaningful — the subclass test assertsinstanceof, which the old clone could not satisfy, and the shape test would catch any ofmessage/stack/causeleaking into a spread.
I verified the implementation in V8: public subclass fields are carried over via the descriptor copy, and cloning never throws on private fields (they aren't returned by Object.getOwnPropertyDescriptors). The change is backward compatible and the instanceof behavior composes correctly with ORPCError's cross-graph Symbol.hasInstance workaround.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…tests Co-authored-by: Wadoud <wadiouyt@gmail.com>
There was a problem hiding this comment.
✅ No new issues found. The prior foot-gun note is now addressed — the subclass private-field limitation is pinned by an explicit test rather than a JSDoc comment.
Reviewed changes
Since the prior review (54b11bd), the cloneORPCError logic is unchanged; the two new commits trim comments and tighten coverage:
- Added
does not carry over subclass private fields— asserts that reading a#private-backed accessor on the clone throwsTypeError, making the constructor-skip limitation executable and regression-proof (the test fails if the clone were built as a plainORPCError). - Removed the
cloneORPCErrorJSDoc and the shape-test comment that are now redundant with the pinned tests.
I re-ran packages/client/src/error-utils.test.ts (25 tests) — all pass. cloneORPCError remains a single shared implementation imported by contract, server, and json-schema, so the fix covers all reconciliation paths.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
…oc instead of a test
There was a problem hiding this comment.
ℹ️ This round swaps the private-field regression test for a JSDoc note — a reasonable doc-for-coverage trade; one observation inline.
Reviewed changes
Since the prior review (4e35e9c), the cloneORPCError implementation is unchanged. The only delta is on documentation/coverage:
- Removed the
does not carry over subclass private fieldstest added in4e35e9c. - Added a JSDoc block on
cloneORPCErrordocumenting that subclass constructors aren't re-run, so#privatefields are absent and subclass members reading them throw on the clone.
The JSDoc matches the verified runtime behavior (Object.defineProperties from getOwnPropertyDescriptors never surfaces private fields, so the clone carries no private slots while inheriting the subclass getter, which then throws on read). This reverses the earlier approved state (limitation pinned by a test) back to documentation-only, per the author's rationale that a limitation — not a contract — shouldn't be pinned by a test.
DeepSeek Flash (free via Pullfrog for OSS) | 𝕏
| * Limitation: subclass constructors are not re-run, so private fields | ||
| * (`#field`) are not carried over and subclass members that read them | ||
| * will throw on the clone. |
There was a problem hiding this comment.
Recording a deliberate trade in this round: the real regression signal for this limitation is now gone. If a future change ever re-runs subclass constructors (or the clone strategy changes), this JSDoc would drift silently stale instead of a test failing — the earlier does not carry over subclass private fields test from 4e35e9c was the only executable guard for it. The rationale (a known limitation, not a contract to pin) is reasonable, so this is fine as-is — just confirming we're consciously accepting documentation-only coverage here.

cloneORPCErrorrebuilt errors withnew ORPCError(...), which stripped custom subclass prototypes, soerror instanceof CustomErrorfailed after error reconciliation. The clone is now constructed as a realORPCErrorwhose prototype is swapped to the original's, so subclass instances stayinstanceoftheir class while the clone remains a nativeError.Supersedes #1799, thanks @Wadiou for the report and the initial approach.
Fixes
instanceoftheir subclass, and custom properties are copied with their exact descriptors.Object.createapproach in fix(client): preserve subclass prototype chain in cloneORPCError #1799, the clone keeps nativeErrorsemantics:message/stack/causestay non-enumerable, so{ ...error }and key iteration do not expose stack traces, andstructuredClone/postMessagestill round-trip.<T extends AnyORPCError>(error: T): T, backward compatible.Testing
tscclean.