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

Don't wrap existing errors #37

Merged
merged 8 commits into from
Sep 24, 2021
Merged

Don't wrap existing errors #37

merged 8 commits into from
Sep 24, 2021

Conversation

vweevers
Copy link
Member

So that when abstract-leveldown starts using level-errors, and such a db is wrapped with levelup, levelup will not rewrap
errors and lose stack trace information in the process. I.e.:

// Error created by abstract-leveldown
const err = new ReadError('example')

// Error created by levelup
const wrapped = new ReadError(err)

assert(wrapped === err)

Initially I wanted to do this in errno, but I want more flexibility than what would be right for errno, specifically, comparing errors by their .type instead of with instanceof. In addition, I found some bugs in errno and prr, of which the latter is archived, so I figured inlining (a subset of errno) would be easier. Also results in less code overall. To see those bugs, review this PR commit by commit.

There's technically a breaking change here: errors are no longer an instanceof CustomError, but I consider that an internal detail.

Ref Level/community#58.

So that when `abstract-leveldown` starts using `level-errors`, and
such a db is wrapped with `levelup`, `levelup` will not rewrap
errors and lose stack trace information in the process. I.e.:

```
// Error created by abstract-leveldown
const err = new ReadError('example')

// Error created by levelup
const wrapped = new ReadError(err)

assert(wrapped === err)
```

Ref Level/community#58
@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Sep 18, 2021
Copy link
Member

@juliangruber juliangruber left a comment

Choose a reason for hiding this comment

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

Great idea! In general lgtm, just had some notes about the implementation being too forgiving / defensive

test.js Outdated Show resolved Hide resolved
test.js Outdated Show resolved Hide resolved
test.js Show resolved Hide resolved
errors.js Show resolved Hide resolved
errors.js Show resolved Hide resolved
errors.js Outdated Show resolved Hide resolved
@vweevers vweevers dismissed juliangruber’s stale review September 24, 2021 13:14

Comments were addressed.

@vweevers vweevers merged commit 189f2b1 into master Sep 24, 2021
@vweevers vweevers deleted the no-wrap branch September 24, 2021 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-patch Bug fixes that are backward compatible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants