How will this conflict with existing solutions like VError?

[voxpelli]

I opened an issue in VError (TritonDataCenter/node-verror#81) about this spec as, while I very much look forward to this spec, it seems to be a possible issue that the VError extension of Error currently implements a different .cause than what this proposal proposes, and same goes for @netflix/nerror.

In VError:

err.cause().message

In this proposal:

err.cause.message

1. Any current code that assumes that when .cause exists it will be of the style of VError#cause, will have an issue, as it will try calling something that isn't a function. This probably shouldn't be a problem as one always needs to ensure that one have complete checks, like the one in Bunyan.

2. Any code that assumes that .cause will be as defined in this proposal won't have an issue with a VError#cause value, as it will then just be treated as if the method itself is the cause rather than the returned value of it – which is okay. (This can change if What type of value is "cause" meant to contain? #21 arrives at something else)

3. Type wise there will be an issue as any class that extends Error needs to have compatible such extensions, and the @types/verror definition of .cause is not compatible with the one in this proposal, so it will fail, as can be exemplified using .stack

I just want to raise this, partly as a way to connect the discussion in eg. VError to a discussion in this proposal.

[voxpelli]

Also: As a function is valid value for cause according to this proposal, then there is no way for a consumer of knowing if the function is provided through this mechanism or if the function is a VError#cause function, so hard to be compatible with both?

[ljharb]

The user can typeof error.cause === ‘function’ to determine that?

[voxpelli]

The user can typeof error.cause === ‘function’ to determine that?

To determine what? 🙂 Currently, as discussed in #21, the following is valid and fulfills that check as well:

const err = new Error('Upload job result failed', { cause: () => 'yay' });

[ljharb]

True. In that case, would instanceof VError work?

[voxpelli]

Not really, there will likely exist plenty of different VError modules in the dependency tree.

What one can do is to use VError.cause(), but that is only doable if you have VError as a dependency yourself and that is often not the case, see eg trentm/node-bunyan#665 (and my failed mochajs/mocha#2381):

if (ex.cause) {
    var cex = typeof (ex.cause) === 'function' ? ex.cause() : ex.cause;
    // ...
}

I guess what one could do is to mimic VError.cause() oneself and go directly for the supposed to be private .jse_cause, but that would be somewhat of a hack.

[bakkot]

the following is valid and fulfills that check as well

While true, it's also extremely un-idiomatic. I wouldn't bother trying to be defensive against such code.

By analogy, it's also perfectly possible to write throw { cause: () => 0 }. If you weren't previously worried about that, I wouldn't worry about someone doing new Error('Upload job result failed', { cause: () => 'yay' }).

[voxpelli]

(typeof error.cause === ‘function’ would actually also be true for new Error('Upload job result failed', { cause: Error });)

My intent with this issue was not to try to change the proposal, but to come up with advice for those who wants to transition from VError to this or have a split ecosystem between the two, so broader than typeof error.cause

[bakkot]

new Error('Upload job result failed', { cause: Error })

That's also extremely un-idiomatic, in just the same way that throw Error would be.

to come up with advice for those who wants to transition from VError to this or have a split ecosystem between the two, so broader than typeof error.cause

Right; my point is that I think typeof error.cause is perfectly sufficient. The language allows you to throw arbitrary values as errors (and, therefore, to set arbitrary values as the cause), but you shouldn't do that, and downstream code shouldn't generally be worrying about how to deal with it if you do.

[voxpelli]

@bakkot I updated my initial post to make my points clearer and to incorporate the feedback so far

[legendecas]

Any current code that assumes that when .cause exists it will be of the style of VError#cause, will have an issue.

I'm afraid I didn't see the issue around the assumption. People using verror-like modules or modules would like to printing causes with verror-like (i.e. errors with a callable cause) would still have to safeguard the type of cause whether or not the cause is part of the language.

For point 2, things would not be alleviated whether or not the cause property is an error object or not. Like said, the modules willing to call on the error.cause() need to check that error.cause is callable with or without the changes.

As a function is valid value for cause according to this proposal, then there is no way for a consumer of knowing if the function is provided through this mechanism or if the function is a VError#cause function, so hard to be compatible with both?

This issue is already a problem if people are caring about it. People may set an arbitrary value on the error.cause already with const error = new Error(); error.cause = () => "yay". The proposal's stance is to make every legit JavaScript code works reasonable, like throw () => "yay" and throw 1, so it currently doesn't swallow a specific type of value.

To my view, in pure JavaScript code, there isn't any transition required for existing code to continue to work with verror-like modules. But additionally, people can now assume the error.cause is the thrown error if it is not a function and they are using verror-like modules.

[ghermeto]

As long as it doesn't break the current implementation of VError you can just make a simple typeof check as it was suggested here.

My only concern, is that right now the polyfill doesn't allow us to test with @netflix/nerror. The following test code:

'use strict';
require('./cause-polyfill');
const { VError } = require('@netflix/nerror');

let e = new VError({
    cause: new Error('test'),
    name: 'TestCause'
});

console.info(typeof e.cause);

will throw (on this line):

/Users/ghermeto/Workspace/runtime/restify-test/node_modules/@netflix/nerror/lib/verror.js:276
    Error.call(this, message);
          ^

TypeError: Class constructor klass cannot be invoked without 'new'

And current error libraries should continue to work moving forward when this proposal is implemented on the engines.

[bakkot]

That's just a bug in the polyfill, which isn't in any sense official (there are no official polyfills). I'm sure there will be more production-ready polyfills at some point that correctly implement the spec and therefore do not throw.

[rauschma]

Is the cause-polyfill (or any other polyfill) available anywhere?

[ljharb]

I haven’t published the es-shims one yet, but will be doing so soonish.

[fabis94]

@ljharb hi, any estimate for when a polyfill would be available?

[voxpelli]

I've now published a ponyfill here: https://github.com/voxpelli/pony-cause

It also includes helpers that makes it easier to make good use of Error Causes + supports both these Error Causes and those from VError/NError.

[ljharb]

@voxpelli nice work! to clarify tho, it's only a "ponyfill" (not that any of us should be using that term) if it's spec-compliant, and making a non-spec ErrorWithCause subclass, as well as calling Error.captureStackTrace and a number of other things, disqualify it from that. It seems like your use case tho is to match VError, so maybe that's OK :-)

I've just published https://www.npmjs.com/package/error-cause, which is spec-compliant, and follows the es-shims interface.

Given both your VError solution and the existence of spec-compliant polyfills, perhaps this is ready to be closed?

[voxpelli]

Having a ponyfill specific name like ErrorWithCause is to avoid it clashing with the built in Error, as is common with ponyfills: https://github.com/sindresorhus/ponyfill#ponyfill

Error.captureStackTrace is taken from here and from VError:

proposal-error-cause/polyfill.js

Lines 8 to 10 in a002df0

	if (Error.captureStackTrace) {
	Error.captureStackTrace(this, klass);
	}

If there’s anything substantial that isn’t spec-compliant in it, feel free to file issues.

And yes, let’s close this 👍

[legendecas]

@voxpelli @ljharb thanks for your work on bringing the implementation to life! I'm going to update the README to guide people to use https://www.npmjs.com/package/error-cause and https://github.com/voxpelli/pony-cause.