Feat/standardize errors

[RangerMauve]

Closes #84

[achou11]

Haven't looked through the actual usage of the custom errors but providing some initial remarks to address issues related to CI + builds.

[RangerMauve]

CI is freezing somewhere, been debugging it, should be ready for final review after

[achou11]

Code changes overall lgtm - just need to fix another small problem with the build process before merging.

[achou11]

just checking: the name of this export is desired over something like ./error-codes.js? I think it's fine to keep what you have - just want to make sure that's an intentional choice.

[RangerMauve]

I was thinking that errors would be less effort to type, but I guess it could be good to have the name be the same. Is there an option that'd be easier to consume downstream?

[gmaclennan]

I appreciate how much work it is to go through all of this! A few overall comments:

• Why drop assertions and switch to conditionals? Is this a syntax preference? I guess I'm ok, we've just tended to use assertions throughout our modules so far.
• I don't think we should have custom errors for absolutely everything, and instead focus on the custom errors for application runtime errors, and just plain errors for developer mistakes that mean that the code just won't run.
• Why define both the name and the code of all the custom errors? It seems like a lot to maintain, and can lead to confusion for future devs about which they should be using to identify errors. I think we should choose either name or code as our error identifier and stick to that.
• We should ensure we're correctly capturing the error stack, which will help with Sentry errors. Do we need something like this.
• A nitpick on naming, but since this PR is renaming everything, I've stuck with e or reason for exceptionVar from a catch, since it's not guaranteed to be an error (although it should be in a sane world). Not using err or error reminds the dev it's not necessarily an error, and allows you to do const err = ensureError(e), although you can always overwrite the exception var with err = ensureError(err) but eslint gets upset about that (although I don't agree with eslint on that one).
• I think if we're going to wrap pTimeout, we should keep the API, rather than wrapping with a slightly different API that removes some of the options (e.g. opts.signal)
• Why do we need the generated errorCodes.js file? What purpose do you imagine this being used for? I'm wondering if there is a different approach that could work for what you envisage.
• I think our Error API should be consistent. Currently some of the custom errors take a string message as the first argument, some of them take the options arg from the Error() constructor as their first, second or third argument (to support opts.cause), and some take one or more arguments to construct an error message. Having those message variables be a list of error parameters makes maintenance hard: e.g. if we have new BlobSourceNotFound(path, opts) and we want to add another message variable e.g. blob Id, then we have to change every reference to move opts to the third parameter. One approach: every custom error takes an optional message or custom message variables as the first parameter, and the Error options as the second param e.g.

  class BlobSourceNotFound {
    constructor(varsOrMessage?: string | { path: string }, opts: ErrorOptions) {
      const message = typeof varsOrMessage === 'string' ? varsOrMessage : typeof varsOrMessage = 'object' ? `blob not found at ${path}` : 'blob not found'
      super(message, opts)

  (feel free to not use tertiaries)
• Naming: I think all custom error names should end in the work Error.

[gmaclennan]

Rather than make this a custom error, I think we would be better wrapping anywhere we use this in a try...catch and throwing a more descriptive custom error, with the cause being the plain error here. KeyNotFoundError() doesn't really help us in application code.

[RangerMauve]

Mind elaborating a bit more? This only gets used as part of getOrThrow inside src/import-categories.js.

[RangerMauve]

Why drop assertions and switch to conditionals?

In order to consistently have error classes with name/code/status everywhere we throw errors.

just plain errors for developer mistakes that mean that the code just won't run.

Is it just the ones you pointed out or is there a specific heuristic I should follow?

Why define both the name and the code of all the custom errors?

The name is there for the pretty printing of error messages, the code is the only thing devs should be checking against. The status is there in case an error hits an HTTP boundry somewhere. We could also use the trick of pulling the name from the constructor name.

We should ensure we're correctly capturing the error stack

That is only necessary if you aren't extending from Error the way we are.

, I've stuck with e or reason for exceptionVar from a catch

I'll rename it 👍

if we're going to wrap pTimeout, we should keep the API

👍

Why do we need the generated errorCodes.js file?

The purpose is to make it easier to check for error types in a type-safe way from consumers. Consumers import errorCodes from "@comapeo/core/errors" and check for e.code === errorCodes.SomeErrorType. This avoids needing to have strings everywhere and accounts for changes in codes over time + is type safe.

An alternative could be to structure the error classes such that they have a code exposed in the constructor, but that requires some fancy wrappers or more boilerplate per error definition.

I think our Error API should be consistent

That makes sense, I'll see what I can do.

I think all custom error names should end in the word Error.

I think I had left some without that convention since they were existing error types, but I agree and will change them.

[RangerMauve]

@gmaclennan @achou11 Regarding the message param, I'm actually leaning more towards removing it from any error types that don't explicitly need it.