Add `code` property to errors

[vweevers]

For example:

if (err.code === 'ERR_NOT_FOUND') // ..

This will allow us to:

• Change error messages without a major bump. Going forward, the semver contract will be on code.
• Change the test suites of levelup and abstract-leveldown to check expected errors by code, making it easier to align errors.
• Return specific errors from C++, because it aligns well with napi_create_error(env, code, ...). E.g. leveldown can return a ERR_NOT_FOUND natively without the need for further wrapping in JS (although initially we'll still need that for backwards compat).

TBD:

1. Does this remove the need for the current error types defined here?
2. If not, will we need a mapping of codes to types?
3. Should we restrict the codes to a known set, or allow modules (like leveldown) to return arbitrary codes?
4. Should the codes have a certain prefix, like ERR_LEVEL_?

Ref Level/community#58

[juliangruber]

• Change error messages without a major bump. Going forward, the semver contract will be on code.

This 💯

1. Does this remove the need for the current error types defined here?

Hard to tell. I think that checking string constants is the safest way, hence also what node does, since you don't get issues with multiple classes being in the dependency tree, breaking instanceof checks. However from a DX perspective it's nicer to import and check for instances. If we can't decide I think I would just mirror node core, and that's using string constants.

3. Should we restrict the codes to a known set, or allow modules (like leveldown) to return arbitrary codes?

Do you have an example of a module needing to create a new error type, or an error type that we have here that is only necessary with certain modules?

4. Should the codes have a certain prefix, like ERR_LEVEL_?

ERR_LEVEL_ seems to match https://nodejs.org/api/errors.html

[vweevers]

Do you have an example of a module needing to create a new error type, or an error type that we have here that is only necessary with certain modules?

leveldown could return more specific error codes like ERR_LEVEL_CORRUPTION, a leveldb error that we currently only expose by error message. I'd prefer that over a generic ReadError or WriteError that only indicate what type of operation caused the error.

[vweevers]

Another example: levelup directly exposes abstract-leveldown iterators via db.iterator(). Which has created the need in leveldown to add safety checks that levelup normally does, and throw errors that (I now realize) aren't wrapped in a level-errors error. For example napi_throw_error(env, NULL, "iterator has ended"). Same goes for abstract-leveldown errors that we've had for a longer time.

[juliangruber]

ERR_LEVEL_CORRUPTION could be one that's useful for more types of backend 🤔 To me either way could work, difficult to tell.

Errors from db.iterator() should be consistent indeed, good catch!

I think the simpler the better here, as long as errors have ideally a code, or a message, people can deal with them. And these can be easily thrown from c++ as well.

[vweevers]

ERR_LEVEL_CORRUPTION could be one that's useful for more types of backend

True. So we can document known error codes here; anyone that implements a new error should first consult that list. Much like node core.

I think the simpler the better here, as long as errors have ideally a code, or a message, people can deal with them. And these can be easily thrown from c++ as well.

My main motivation for opening this issue is merging abstract-leveldown and levelup, starting with API parity and being able to run their test suites on each other. Introducing error codes offers a non-breaking migration path - just for us (userland consumption of errors is a secondary concern):

• Add default code properties to level-errors, e.g. a generic ERR_LEVEL_READ for ReadError
• Add code argument to level-errors constructors, so that the default generic codes can be made more specific
• Add code to non-wrapped errors in (abstract-)leveldown, esp. those that the test suite(s) currently check by message
• Add code to wrapped errors from levelup, esp. those that that the test suite(s) currently check by message

At that point, errors from either have a common property, they're documented in a central place, and messages no longer matter. It may remove the need to add level-errors to abstract-leveldown (and the need for #37, though I'll move ahead on that just to give us a choice) except in rare cases like normalizing NotFoundErrors (TBD).

[juliangruber]

This sounds fantastic to me, @vweevers 👏

[vweevers]

May not need level-errors at all. Here's an example without: https://github.com/Level/transcoder/blob/b725dff60a75e037d3e626fe6d31b316726ffacc/lib/encoding.js#L92-L95

[mcollina]

We have developed https://github.com/fastify/fastify-error as part of Fastify. It's a re-implementation in userland of the same pattern we use in Node.js core.

[vweevers]

I'm happy with https://github.com/vweevers/module-error because it's not http-centric, aligns with ecmascript spec (first argument being a message, second argument an options object), simple, browser-compatible, supports cause, and a few other patterns that I'm using in airtap and elsewhere.

[vweevers]

Done in abstract-level (see errors). It's not using level-errors, as the only error that needed special handling (for backwards compat) is the not found error on db.get().