Skip to content
This repository has been archived by the owner on Jan 26, 2022. It is now read-only.

What type of value is "cause" meant to contain? #21

Closed
domenic opened this issue Nov 30, 2020 · 18 comments
Closed

What type of value is "cause" meant to contain? #21

domenic opened this issue Nov 30, 2020 · 18 comments

Comments

@domenic
Copy link
Member

domenic commented Nov 30, 2020

The explainer talks about chaining errors, or catching and re-throwing with more contextual information. That implies cause should usually be an Error instance of some sort.

However, one of the proposal champions suggests using a string:

throw new Error('Request Error', 'Expected JSON got a string!');

Is the intention of cause to contain arbitrary "detail" data of this sort? The example goes a bit further, even, making the message property into more of a "title", where the cause property contains the actual error message.

I ask because web specifications have sometimes wondered about how to include more detailed error codes or similar (e.g. for distinguishing different types of network errors), and so if cause is meant to contain general, potentially-string data about what happened, we might start using it as such in web specifications.

@concavelenz
Copy link

Before I read this, I add assumed that cause was an Error. It would be good to get this clarified so everyone is talking about the same thing.

@hemanth
Copy link
Member

hemanth commented Dec 3, 2020

Yes, that was intent in which we presented the proposal. Also here is a fiddle with that behavior.

@domenic
Copy link
Member Author

domenic commented Dec 3, 2020

What, exactly, was the intent? The fiddle shows a string for the cause, instead of wrapping an error; are you saying the intent was to use strings?

@hemanth
Copy link
Member

hemanth commented Dec 12, 2020

Well, should allow any value and wrap it internally to an error?

Maybe we should avoid the the cause on the prototype altogether?

@ljharb
Copy link
Member

ljharb commented Dec 12, 2020

It seems like the best path forward is to:

  • signal the intent, by making all the docs and examples use an Error instance for the cause
  • remove cause from the prototype, to follow the precedent established by AggregateError with "errors"
  • decide if cause should:
    1. allow any value (current spec text, i believe)
    2. throw on a non-Error instance
    3. somehow coerce non-Error instances to an Error instance (like message, which stringifies)

@yordis
Copy link

yordis commented Dec 12, 2020

From what I can gather if the intention is to allow some sort of rethrowing with additional context,

I would like the cause to be an Error.

Otherwise, such a proposal could become an anti-pattern and people would overused simple to ignore creating new errors in the system. I would like to see a more concrete example of the problem statement such as https://github.com/tc39/proposal-error-cause#why-not-custom-subclasses-of-error it is not clear to me what the issue is.

@bakkot
Copy link
Contributor

bakkot commented Dec 30, 2020

Because I understand the intended usage to be

try {
  something();
} catch (e) {
  throw new Error('something failed!', e);
}

and there are no restrictions on the type of e (e.g. something() could have thrown 42), I think you have to technically allow any type of value (or force all users to type-check in this pattern every time, which seems bad).

However, I think putting anything other than an Error instance in this field should be considered to be extremely un-idiomatic, just as throwing any value other than an Error is considered un-idiomatic. I am strongly against using this field to represent anything other than "the Error (as would be seen by catch) which caused this Error".

For the case of web specs, it seems reasonable to synthesize a new Error object which contains other data and to put that object in the cause field if the root error is logically "caused" by some other error. If it is not, I wouldn't think cause would be the appropriate field.

But perhaps I have a different understanding of the intent of this proposal than the champions.

@ljharb
Copy link
Member

ljharb commented Dec 30, 2020

@bakkot makes sense; that would seem to support option 1 - ie, the current spec text modulo the second bullet point.

@yordis
Copy link

yordis commented Dec 30, 2020

if it helps somehow (at least to me it does),

This proposal reminds me Go and wrapping Errors, which, if I am not mistaken, is the intention of such a proposal, or at least is what I am hoping it is (the difference is that Go errors are not like JS, but shouldn't matter).

Right?

In that case, definitely see the value of the proposal, be able to keep the original context of the error, and keep adding more contexts to the chain of errors.

@legendecas
Copy link
Member

legendecas commented Jan 6, 2021

It is true the value of cause can be any JavaScript value, for any JavaScript value being able to be thrown. However, I believe the value in the field of cause should be considered suitable and reasonable to be thrown. In that sense, we may avoid throwing plain JavaScript values like numbers or strings, and avoid forging a number or string for the field of cause.

@bakkot
Copy link
Contributor

bakkot commented Jan 6, 2021

@legendecas

I believe the value in the field of cause should be considered suitable and reasonable to be thrown

I can't tell what actual behavior you're supporting. Do you think that the Error constructor should validate the type of the cause parameter, and throw if it is "bad"? Or are you just saying that in our examples we should use actual Error instances and not numbers or strings?

@legendecas
Copy link
Member

legendecas commented Jan 6, 2021

@bakkot I'm supporting throwing with Error instances and placing Error instances in the field of cause.

However, I'm not comfortable with validating the value of cause in the Error constructor and somehow throw a TypeError if the cause is not an Error object. So for the options @ljharb has listed:

  • allow any value
  • somehow coerce non-Error instances to an Error instance (like the message, which stringifies)

Both above options sound good to me, but not throw on a non-Error instance. And the coercing solution would allow the cause to have a consistent interface, i.e. it is either undefined or an Error object. I'm happy with that.

@hax
Copy link
Member

hax commented Jan 21, 2021

The problem is "coercing" may also throw error :)

So I feel allowing any value may be the simplest choice.

@voxpelli
Copy link

I had assumed that .cause would be an Error as well (when writing this and comparing to VError: #31)

I'm pro on somehow coerce non-Error instances to an Error instance (like the message, which stringifies) and when it isn't possible to coerce – either add an Error with some message saying so or just leave out the cause property all together to fail silently?

A basic coercion could be: If value is not an instanceof Error, but is a string, then do new Error(value), else don't do anything and just ignore the value?

@voxpelli
Copy link

To make compatibility with existing solutions like VError easier (#31) – could at least it be ensured that a function value won't be exposed as .cause? As all of those solutions use a function to fetch the cause rather than exposing it directly.

@papb
Copy link

papb commented Mar 26, 2021

JavaScript already allows (unfortunately?) anything to be thrown. I agree with @bakkot that the following should be a valid way to code:

try {
  something();
} catch (e) {
  throw new Error('something failed!', e);
}

Therefore the cause should be allowed to be anything as well.

@papb
Copy link

papb commented Mar 26, 2021

Whether or not this cause will be coerced to a real error before being stored is another question. I think it should only be coerced if the coercion method is guaranteed to succeed perfectly at all times. Things like coercing if possible and failing silently, or coercing if possible and keeping the original value if not possible, or anything else, would become just another point for confusion and complexity.

For a coercion method that is guaranteed to succeed, see the ensure-error module.

@legendecas
Copy link
Member

Closing this as there isn't any open question to be answered. Feel free to re-open if needed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants