Skip to content
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

Add error utils #146

Merged
merged 13 commits into from
Oct 17, 2023
Merged

Add error utils #146

merged 13 commits into from
Oct 17, 2023

Conversation

mcmire
Copy link
Contributor

@mcmire mcmire commented Oct 13, 2023

This commit satisfies a few needs that we have in various projects:

  • When catching a throwable from some kind of operation, we want to be able to test whether the throwable is an error.
  • Furthermore, since the Error interface in TypeScript is pretty simple, we want to be able to test for different properties on an error (code, stack, etc.).
  • We want to wrap an error produced by a lower level part of the system with a different message, but preserve the original error using the cause property (note: this property was added in Node 18, so for older Nodes, we use the pony-cause library to set this).
  • Finally, we want to be able to take a throwable and produce an error that has a stacktrace. This is particularly useful for working with the fs.promises module, which (as of Node 20) does not produce proper stacktraces.

Places where we've used these kinds of utility functions:


Relates to MetaMask/module-lint#5.

This commit satisfies a few needs that we have in various projects:

- When catching a throwable from some kind of operation, we want to
  be able to test whether the throwable is an error.
- Furthermore, since the Error interface in TypeScript is pretty simple,
  we want to be able to test for different properties on an error
  (`code`, `stack`, etc.).
- We want to wrap an error produced by a lower level part of the system
  with a different message, but preserve the original error using the
  `cause` property (note: this property was added in Node 18, so for
  older Nodes, we use the `pony-cause` library to set this).
- Finally, we want to be able to take a throwable and produce an error
  that has a stacktrace. This is particularly useful for working with
  the `fs.promises` module, which (as of Node 22) [does not produce
  proper stacktraces][1].

[1]: nodejs/node#30944
@mcmire mcmire requested a review from a team as a code owner October 13, 2023 23:26
@socket-security
Copy link

New dependencies detected. Learn more about Socket for GitHub ↗︎

Packages Version New capabilities Transitives Size Publisher
pony-cause 2.1.10 None +0 23.9 kB voxpelli

@MajorLift
Copy link
Contributor

MajorLift commented Oct 14, 2023

I wrote some test cases for error objects with multiple properties: 41aba60.

Copy link
Member

@Mrtenz Mrtenz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A function to get an error message from an unknown error would be nice too. We use this in various places in Snaps at least.

@mcmire
Copy link
Contributor Author

mcmire commented Oct 16, 2023

@Mrtenz I've added getErrorMessage. I took it from errors.ts in Snaps, but modified it to return an empty string for null and undefined. What do you think?

@mcmire
Copy link
Contributor Author

mcmire commented Oct 16, 2023

@MajorLift I added your tests. There were a couple of duplicate tests that I removed as we discussed. I also added a test for isErrorWithStack that mimicked what you did for isErrorWithCode and isErrorWithMessage. Does that work?

MajorLift
MajorLift previously approved these changes Oct 16, 2023
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the updates look good!

@mcmire
Copy link
Contributor Author

mcmire commented Oct 16, 2023

@MajorLift ^ I pushed one more change, sorry!

MajorLift
MajorLift previously approved these changes Oct 16, 2023
Copy link
Contributor

@MajorLift MajorLift left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

legobeat
legobeat previously approved these changes Oct 17, 2023
src/assert.ts Outdated Show resolved Hide resolved
@mcmire mcmire dismissed stale reviews from legobeat and MajorLift via 55bc59e October 17, 2023 03:31
mcmire and others added 2 commits October 16, 2023 21:31
Co-authored-by: legobeat <109787230+legobeat@users.noreply.github.com>
@mcmire mcmire merged commit 2263f9b into main Oct 17, 2023
16 checks passed
@mcmire mcmire deleted the add-error-utils branch October 17, 2023 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants