fix: error handling with Node VM#7280
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
WalkthroughUnknownCauseError now takes a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/server/src/unstable-core-do-not-import/error/TRPCError.ts`:
- Around line 7-10: The constructor currently uses Object.assign(this, cause)
which copies only enumerable properties and loses non-enumerable fields like
Error.prototype.stack; replace that assignment by copying all property
descriptors from the cause so non-enumerable properties are preserved (e.g., use
Object.getOwnPropertyDescriptors(cause) and Object.defineProperties(this, ...)
and guard for null/primitive causes) — update the constructor(cause: object) in
TRPCError.ts to use full descriptor copying instead of Object.assign(this,
cause) while still calling getMessage(cause) as before.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9682d153-f908-4b2b-89ea-bf8a1b46b9da
📒 Files selected for processing (1)
packages/server/src/unstable-core-do-not-import/error/TRPCError.ts
| constructor(cause: object) { | ||
| super(getMessage(cause)); | ||
| Object.assign(this, cause); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In JavaScript/Node.js, does Object.assign copy non-enumerable properties like Error.stack from an Error instance?
💡 Result:
No, Object.assign does not copy non-enumerable properties like Error.stack from an Error instance. Object.assign only copies enumerable own properties from source objects to the target. Error properties such as stack, message, and name are own properties but non-enumerable, so they are skipped. This behavior is consistent in both browser JavaScript and Node.js, as Object.assign follows the ECMAScript specification. To copy non-enumerable properties from an Error, use Object.getOwnPropertyDescriptors followed by Object.defineProperties, or manually assign specific properties like {stack: error.stack}.
Citations:
- 1: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/assign
- 2: https://www.bennadel.com/blog/2874-error-object-properties-are-not-iterable-enumrable-in-node-js.htm
- 3: https://stackoverflow.com/questions/52845316/unexpected-output-from-object-assign
- 4: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Guide/Enumerability_and_ownership_of_properties
Stack trace lost when wrapping non-enumerable error properties
Line 9: Object.assign only copies enumerable properties. Error objects have non-enumerable properties like stack, so wrapping cross-realm or error-like objects causes stack traces to be lost.
Proposed patch
class UnknownCauseError extends Error {
[key: string]: unknown;
constructor(cause: object) {
super(getMessage(cause));
+ if ('stack' in cause && typeof (cause as { stack?: unknown }).stack === 'string') {
+ this.stack = (cause as { stack: string }).stack;
+ }
Object.assign(this, cause);
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| constructor(cause: object) { | |
| super(getMessage(cause)); | |
| Object.assign(this, cause); | |
| } | |
| constructor(cause: object) { | |
| super(getMessage(cause)); | |
| if ('stack' in cause && typeof (cause as { stack?: unknown }).stack === 'string') { | |
| this.stack = (cause as { stack: string }).stack; | |
| } | |
| Object.assign(this, cause); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/server/src/unstable-core-do-not-import/error/TRPCError.ts` around
lines 7 - 10, The constructor currently uses Object.assign(this, cause) which
copies only enumerable properties and loses non-enumerable fields like
Error.prototype.stack; replace that assignment by copying all property
descriptors from the cause so non-enumerable properties are preserved (e.g., use
Object.getOwnPropertyDescriptors(cause) and Object.defineProperties(this, ...)
and guard for null/primitive causes) — update the constructor(cause: object) in
TRPCError.ts to use full descriptor copying instead of Object.assign(this,
cause) while still calling getMessage(cause) as before.
@trpc/client
@trpc/next
@trpc/openapi
@trpc/react-query
@trpc/server
@trpc/tanstack-react-query
@trpc/upgrade
commit: |
There was a problem hiding this comment.
Pull request overview
This PR adjusts @trpc/server’s internal error-cause normalization to better preserve error messages when the thrown value comes from a different JS realm (e.g. Node’s vm), where instanceof Error can fail.
Changes:
- Add a custom
UnknownCauseErrorconstructor that initializes the error message from the unknown “error-like” object. - Replace
Object.assign(new UnknownCauseError(), cause)withnew UnknownCauseError(cause)somessageis captured even when non-enumerable.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| function getMessage(cause: object) { | ||
| return (cause as Error).message; |
There was a problem hiding this comment.
getMessage(cause) reads (cause as Error).message and passes it directly to super(...). Because cause is unknown, accessing .message can throw (e.g. Proxy/getter), and non-string values like symbol can cause the Error constructor to throw during string coercion. Since this runs in the error-handling path, it should be best-effort and never throw—wrap the read/coercion in a try/catch and fall back to '' (or a safe default) if anything goes wrong.
| return (cause as Error).message; | |
| try { | |
| const message = (cause as Error).message as unknown; | |
| if (typeof message === 'string') { | |
| return message; | |
| } | |
| if ( | |
| typeof message === 'number' || | |
| typeof message === 'boolean' || | |
| typeof message === 'bigint' | |
| ) { | |
| return String(message); | |
| } | |
| if (typeof message === 'symbol') { | |
| return message.description ?? ''; | |
| } | |
| return ''; | |
| } catch { | |
| return ''; | |
| } |
I updated the function getMessage(cause: object) {
if ('message' in cause) return String(cause.message);
return undefined;
}If I were to Do you still think I should do it? |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tests/server/regression/issue-7272-node-vm-error-message.test.ts (1)
64-66: Use.toContain()for resilience against potential future error message format changes.The test currently asserts the exact error message format with
.toBe(). While the message format is consistent across Node 18.x and 20.x (the tested versions), using substring matching with.toContain()would make the test more resilient to potential V8/Node.js changes in future versions while still validating that the message is preserved.Suggested assertion adjustment
- expect(trpcError.message).toBe( - "Cannot read properties of null (reading 'property')", - ); + expect(trpcError.message).toContain('Cannot read properties of null'); + expect(trpcError.message).toContain("'property'");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tests/server/regression/issue-7272-node-vm-error-message.test.ts` around lines 64 - 66, Replace the strict equality assertion on trpcError.message with a substring check to make the test resilient to V8/Node message format changes: locate the assertion using trpcError.message in the test (in issue-7272-node-vm-error-message.test.ts) and change the expect(...).toBe("Cannot read properties of null (reading 'property')") call to use expect(...).toContain("Cannot read properties of null") so the test still verifies the preserved message content without depending on exact formatting.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tests/server/regression/issue-7272-node-vm-error-message.test.ts`:
- Around line 64-66: Replace the strict equality assertion on trpcError.message
with a substring check to make the test resilient to V8/Node message format
changes: locate the assertion using trpcError.message in the test (in
issue-7272-node-vm-error-message.test.ts) and change the
expect(...).toBe("Cannot read properties of null (reading 'property')") call to
use expect(...).toContain("Cannot read properties of null") so the test still
verifies the preserved message content without depending on exact formatting.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7f6461a9-6757-4285-b9b8-df63bba378c9
📒 Files selected for processing (2)
packages/server/src/unstable-core-do-not-import/error/TRPCError.tspackages/tests/server/regression/issue-7272-node-vm-error-message.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/server/src/unstable-core-do-not-import/error/TRPCError.ts
|
Hi @Nick-Lucas , is there anything else I need to do in this PR? |
|
Hey @znikola - busy week, sorry for the delay. Thanks for adding the test! I can see it fails without the fix, and the fix seems straight-forward, so let's get this in |
Closes #7272
🎯 Changes
Fixed error handling when tRPC is used with Node VM by attempting to show the error message for implicitly thrown errors.
✅ Checklist
Summary by CodeRabbit