lib,crypto: add optional cause to DOMException

[panva]

Adds an optional cause to DOMException, put to use for operation specific WebCryptoAPI errors.

[nodejs-github-bot]

Review requested:

• @nodejs/crypto

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/46017/

[legendecas]

I believe API changes to web interfaces should be made to their standards first: https://webidl.spec.whatwg.org/#idl-DOMException. From what I can tell, there are discussions on overloading the second parameter of DOMException to accept an option bag instead of introducing a third positional parameter for cause: whatwg/webidl#969

[legendecas]

This can be a good addition! I'm going to submit a PR on WebIDL to define the API changes on DOMException: whatwg/webidl#1179.

[panva]

Given DOMExceptions are instances of Error I assumed this would be a fair game.

Probably makes sense to resolve idl if this changes the exposed constructor tho.

Question: If I change the PR to not modify the constructor but still have the cause property, would that be ok now?

[aduh95]

Wouldn't that be more correct? (at least that's the 'cause' in new Error === false, and 'cause' in new Error('', { cause: undefined})) === true)

Suggested change

	cause
	});
	});
	if (arguments.length > 2) {
	ObjectDefineProperty(this, 'cause', {
	__proto__: null,
	configurable: true,
	enumerable: true,
	value: cause,
	writable: true,
	});
	}

[LiviaMedeiros]

Is this signature preferable over any of these?

Suggested change

	constructor(message = '', name = 'Error', cause = undefined) {
	constructor(message = '', name = 'Error', options = { cause }) { // pseudocode

or

Suggested change

	constructor(message = '', name = 'Error', cause = undefined) {
	constructor(message = '', nameOrOptions = 'Error') {

[legendecas]

Question: If I change the PR to not modify the constructor but still have the cause property, would that be ok now?

I'm unsure how the property cause would be defined on the DOMException. In ECMAScript, both name and message are own properties on the Error instances or the Error prototype. But they are attributes (with getters) on DOMException. cause may not present on an Error instance as an own property if the cause is not given. But if as an attribute with getters, it must be present and may not be distinguished from the case of DOMException(message, { cause: undefined }).

The use case with undefined cause for DOMException might be negligible, as the DOMException is mostly constructed by the host environment. It's probably fine for DOMException to always define the cause getter.

[jasnell]

Per the debate (whatwg/webidl#1179) that is happening in the PR that is updating DOMException to have a cause there is a disagreement on whether cause should be an own property or an accessor. For all intrinsic Errors, TC-39 decided that cause should be an own data property in order to distinguish between the case where cause is explicitly not given vs. cause is explicitly undefined

const err1 = new Error('no cause');
console.log('cause' in err1); // false
console.log(err1.cause); // undefined

const err2 = new Error('undefined cause', { cause: undefined });
console.log('cause' in err2); // true
console.log(err2.cause); // undefined

From all appearances, it seems a decision is going to be made that DOMException should be different that intrinsic errors and will use an accessor instead, making it impossible to distinguish between these cases.

const err1 = new DOMException('no cause', 'ABORT_ERR');
console.log('cause' in err1); // true
console.log(err1.cause); // undefined

const err2 = new DOMException('undefined cause', { name: 'ABORT_ERR', cause: undefined });
console.log('cause' in err2); // true
console.log(err2.cause); // undefined

The reasons for diverging do not seem that well defined beyond consistency with the way name and message are currently handled for DOMException but it's not really a compelling reason for me.

For our implementation, I'd rather we choose to align with TC-39's definition and intentionally diverge from the official definition that is likely to land in the WebIDL spec.

tl;dr ... let's not use an accessor for cause. Let's make it an own data property on the DOMException instance.

[panva]

#44224 (comment)

[panva]

@jasnell I don't want to close this PR just yet given there are ongoing discussions, but when the dust settles please open a new PR with whatwg/webidl#1179 that will supersede this one.

[jasnell]

@panva ... absolutely. I've asked my son @flakey5 if he'd like to take an a variation of this that follows the approach in @legendecas' proposal but using the own property instead of the getter. He's working on the new PR now.

[jasnell]

New alternative PR: #44703