-
-
Notifications
You must be signed in to change notification settings - Fork 15
feat: Add ensureError utility for converting unknown values to error #275
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds a new ensureError utility function to the @metamask/utils package that guarantees an Error instance from unknown values without wrapping existing errors. This addresses a common need across MetaMask codebases where caught errors need to be converted to Error instances while preserving original errors unchanged.
Changes:
- Adds
ensureError(error: unknown, context?: string): Errorfunction that returns existing Error instances unchanged or converts other values to Error instances - Provides comprehensive test coverage for all code paths including Error instances, fs.promises errors, primitives (strings, numbers), objects, and null/undefined
- Updates export snapshots in both index and node entry points
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| src/errors.ts | Implements the new ensureError function with proper type guards and message handling |
| src/errors.test.ts | Adds comprehensive test suite covering all scenarios including Error preservation, type conversions, and context handling |
| src/index.test.ts | Updates export snapshot to include ensureError in alphabetical order |
| src/node.test.ts | Updates export snapshot to include ensureError in alphabetical order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
I had a couple of questions, but good idea to make this a shared utility. I've wanted this for a long time. The type checking/coercion in every single |
- Use `{ cause: error }` to preserve original value instead of string conversion
- Standardize message to "Unknown error" for all non-Error values
- Remove unnecessary number-to-error test case
- Update tests to verify cause preservation
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Use ErrorWithCause polyfill when native Error cause is not supported - Follows same pattern as existing wrapError function - Add istanbul ignore for runtime-dependent branch Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requesting changes due to disallowed @ts-ignore directives
Requested changes (ts-ignore removal) have been made
Gudahtt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Summary
This utility already exists in
metamask-mobileand is duplicated across different teams/codebases. It makes sense to have it published in@metamask/utilsas a shared utility.Currently, when catching errors in JavaScript/TypeScript, the caught value can be anything (
unknown). The existing error utilities provide:isErrorWithCode,isErrorWithMessage,isErrorWithStackgetErrorMessage(error)→ returns a stringwrapError(error, message)→ creates new Error with cause linkHowever, there is no utility to guarantee an Error instance from an unknown caught value without wrapping it in a new Error.
This PR adds
ensureError(error: unknown): Errorwhich:Error("Unknown error")with the original value preserved incauseFor adding context, developers can compose with
wrapError:wrapError(ensureError(err), 'context')Why this is needed
In mobile, we have many locations doing
error instanceof Error ? error : new Error(error). This utility standardizes that pattern. External libraries and third-party SDKs sometimes throw non-Error values (undefined, strings, objects), and we need to convert them to proper Error instances for Sentry to process correctly.Real example: METAMASK-MOBILE-5DKA - 10,675 occurrences affecting 429 users where HyperLiquid SDK throws
undefined.Key differences from
wrapError:getErrorMessage()stringwrapError()Error(new, with cause)ensureError()Error(original or converted)ensureErrorreturns the original Error if already an Error (no wrapping)wrapErroralways creates a new Error with a cause chainImplementation notes
causeto preserve the original thrown value for debuggingTest plan
cause