catchException catches all exceptions rather than just Error exceptions

[eric-corumdigital]

It seems clear that this package is intended to catch Error exceptions. However, catchException does this:

try {
        return t();
      } catch (e) {
        if (e instanceof Error || Object.prototype.toString.call(e) === "[object Error]") {
          return c(e)();
        } else {
          return c(new Error(e.toString()))();
        }
}

That is, if the exception is determined to not be an Error exception, it attempts to convert the exception to an Error exception. This conversion itself throws for both null and undefined, which are possible values. Furthermore, this implementation prevents users from catching other types of exceptions in an obvious way (they would have to use a different catch before this one).

Suggestion: just rethrow the exception if it is not Error.

[JordanMartinez]

I think this makes sense?

At the very least, there could be a variant that will only catch Errors and just rethrow exceptions, but I'm not sure whether that's desirable or not.

[hdgarrood]

It's definitely a problem that attempting to catch a thrown null or undefined will throw an unrelated exception inside the implementation of catchException. I also think it's not ideal that we can't catch some other kind of thrown non-Error object without converting it into an Error, especially if it's a lossy conversion - .toString() usually loses information. Perhaps we could save the originally thrown object by writing it to a new property on the Error we are throwing.

However, I think catching a thrown object even if it's not Error does make sense. If I've written catchException, then I want exceptions to be caught. I would be open to adding an catchExceptionForeign which types the argument as Foreign, or something along those lines, for when you're expecting something which isn't necessarily an Error to be thrown. I would also be open to adding a variant which only catches Errors and rethrows everything else, but I don't think that should be the default.

[kl0tl]

There’s a stage 2 proposal to add a cause property to errors: https://github.com/tc39/proposal-error-cause. We could perhaps add the value originally thrown on the newly created error, under a cause property, when the proposal will reach stage 3 or 4?

[jmp-0x7C0]

There’s a stage 2 proposal to add a cause property to errors: https://github.com/tc39/proposal-error-cause. We could perhaps add the value originally thrown on the newly created error, under a cause property, when the proposal will reach stage 3 or 4?

It looks like the proposal has reached stage 4 now.

[JordanMartinez]

Looks like 88% of browsers support that? https://caniuse.com/mdn-javascript_builtins_error_cause

[garyb]

Even without native support for cause... this is JavaScript, so we could just attach the caught value as any arbitrary property to the Error we create; caught, inner, or something.