Design AggregateError

[bakkot]

See this thread.

Personally I would expect the design to be basically

class AggregateError extends Error {
  errors;
  constructor(errors, message) {
    super(message);
    this.errors = Array.from(errors);
  }
}

though it isn't totally obvious to me what order the parameters should go in.

We might also want to make it iterable. I lean towards not - if you want to iterate the errors, iterate the .errors - but other people might disagree.

[bakkot]

@ljharb @chicoxyzzy:

I think "optional arguments follow mandatory arguments" is a stronger principle than "subclasses should have the same signature as their superclass", especially given the existence of additional optional arguments on all Error subclasses in many implementations, so I am inclined to put the errors parameter before message.

[ljharb]

I think errors is optional, as is message.

[ljharb]

I might want an AggregateError with no errors simply to communicate that something nonspecific has happened, say.

[bakkot]

I think AggregateError ought to be reserved for cases where you have a finite (if possibly empty) collection of errors to report. That seems to me to be what the name implies.

[domenic]

Strongly agree. Is there anything we can do, either in the naming or design, to prevent people from abusing AggregateError as an UnknownError? Perhaps we could even throw if there are 0 errors supplied?

[chicoxyzzy]

Speaking about iterability, between

Promise.any(failingAsyncTasks).catch(aggregateError => [...aggregateError]).map(...);

and

Promise.any(failingAsyncTasks).catch(({ errors }) => errors).map(...);

I'd prefer latter.

Though property named errors could be misleading because as we discussed, it can hold elements of any type, not just Errors

[ljharb]

Looking at https://tc39.github.io/ecma262/#sec-error-message, message is not in fact optional - so there aren’t any ordering constraints except that if error is optional, it must come last.

Given that, i think that it should come last regardless of what requirements it has.

[bakkot]

Sorry, I was using "optional" in the colloquial sense, i.e., the constructor still works if you don't pass that parameter.

I think that AggregateError should work if and only if the errors parameter is supplied and is an iterable, and so it should be the first parameter.

[ljharb]

i don’t think the ordering mandate applies unless its optional in the “it’s signature lists it in brackets” sense.

I see how it might be awkward to do new AggregateError(undefined, []), but what default message do you imagine engines might provide for that case? It seems like making message more required (in the colloquial sense; throwing if it’s not a sting) is a better fit here anyways.

[bakkot]

i don’t think the ordering mandate applies unless its optional in the “it’s signature lists it in brackets” sense.

/shrug I disagree. I don't think the spec formalism should guide the API design very much, if there's other arguments one way or another.

Edit: or rather, I agree that there is not a mandate, in the strict specification sense, but I continue to think "optional arguments follow required ones" is a good rule to follow when designing APIs.

what default message do you imagine engines might provide for that case?

I would expect them to omit it, as they are required to do per spec for new TypeError(undefined) and friends (and hence inherit that property from Error.prototype, such that (new AggregateError(undefined, [])).message === ''). I don't see why this would be any different for AggregateError.

[ljharb]

Are there HTML error types (that inherit from Error) that take "not a message" as the first argument? It'd be interesting to compare.

[ljharb]

(In general, regarding API design, it seems very strange to me when a subclass's constructor signature is not a superset of its parent class's constructor signature, but to be fair i don't design inheritance-based APIs very often)

[bakkot]

Are there HTML error types (that inherit from Error) that take "not a message" as the first argument?

OverconstrainedError in the media capture API does.

I'm not sure what other error types there are - MediaError exists but is not constructible and DOMError has been removed (but previously took not-a-message as its first argument). There's also DOMException, which takes an additional (optional) argument after the message argument, and a proposed RTCError which takes a non-optional argument after the message argument (though I've just opened an issue over there suggesting they switch the order).

EDIT: RTCError now takes its not-a-message argument first.

[Ginden]

Personally I would expect indexed properties (err[0]), length and @@iterator.

[bakkot]

@Ginden I'd be pretty reluctant to introduce a new array-like, vs just having a property which is actually an array.

[ljharb]

I agree with that; a new iterable is nbd, but a new arraylike isn’t quite as palatable :-)

[chicoxyzzy]

So as I understand it, the API should look like this:

class AggregateError extends Error {
  message: string;
  name: string;

  constructor(errors: any[], message: string = '') {
    super(message);
    if (!Array.isArray(errors) || errors.length === 0) throw new TypeError('AggregateError should provide an array of errors');
    this.name = 'AggregateError';
    this.errors = errors;
  }
}

[zloirock]

I'm for usage iterator protocol for errors and making AggregateError instance iterable.

[bakkot]

@chicoxyzzy I'd disagree on a couple of points:

• errors should only need to be an iterable, not an array.
• errors should be copied into a new array.
• I don't think errors.length === 0 is something this constructor should enforce.
• I don't think message should default to ''. There is observably different behavior between the two (in particular, whether message is own or inherited).

I think it should be basically the thing I put in the OP. Or to be more precise, in spec text, I would say

AggregateError ( errors, message )

When the *AggregateError* function is called with arguments _errors_ and _message_, the following steps are taken:

1. If NewTarget is *undefined*, let _newTarget_ be the active function object, else let _newTarget_ be NewTarget.
2. Let O be ? OrdinaryCreateFromConstructor(_newTarget_, `"%AggregateErrorPrototype%"`, « [[ErrorData]] »).
3. Let _errorsIterator_ be ? GetMethod(_errors_, @@iterator).
4. Let _errorsList_ ? IterableToList(_errors_, _errorsIterator_).
5. Let _errorsArray_ be CreateArrayFromList(_errorsList_).
5. Perform ! CreateDataProperty(_O_, `"errors"`, _errorsArray_).
6. If _message_ is not _undefined_, then
  a. Let msg be ? ToString(_message_).
  b. Let msgDesc be the PropertyDescriptor { [[Value]]: _msg_, [[Writable]]: *true*, [[Enumerable]]: *false*, [[Configurable]]: *true* }.
  c. Perform ! DefinePropertyOrThrow(_O_, `"message"`, _msgDesc_).
7. Return _O_.

If we want to make AggregateError iterable - which I am opposed to - the cleanest thing would be to make a @@iterator getter on AggregateError.prototype which returned this.errors[@@iterator].

(Edit: switched the creation of the errors property to use CreateDataProperty, which has the practical effect of making it enumerable, as I think it probably should be.)

[mathiasbynens]

Per #14 (comment) I consider this issue to be resolved. @bakkot's suggested spec text made it into the proposal. Closing this issue.

[rbuckton]

Out of curiosity, why should errors be enumerable when message is not?

[rbuckton]

Also, considering the fact that https://github.com/tc39/proposal-explicit-resource-management also needs AggregateError, should we consider splitting it out into a separate proposal?

[bakkot]

@rbuckton I don't have a strong reason. I was mostly thinking that it would be more discoverable that way. But if there's any particular reason for it not to be, I'd be fine with that changing.

(Are there other error types which have enumerable properties?)

[ljharb]

Object.keys(new Error()) prints out ['line', 'column'] in my Safari console, but an empty array in Chrome; if i require a nonexistent file in node, the error thrown has [ 'code', 'requireStack' ].

[rbuckton]

Object.keys(new Error()) prints out ['line', 'column'] in my Safari console, but an empty array in Chrome; if i require a nonexistent file in node, the error thrown has [ 'code', 'requireStack' ].

This is likely because Safari and Node add additional members via Set. Most built-in data properties are non-enumerable.

[ljharb]

It feels like message should have always been enumerable, but since it's not, we probably should keep errors non-enumerable too?

[mathiasbynens]

@szuend, any opinions on whether the errors property on AggregateError instances should be enumerable?

[szuend]

No strong opinion whether errors should be enumerable. My intuition is to keep it consistent with message.

Another question I have though is, since AggregateError is an Error, what would be the semantic of the stack property?

[ljharb]

It should get a stack trace from the place it’s created, identically as if any other error type was constructed there.

[rbuckton]

I'm primarily in favor of errors having the same enumerability as message (i.e. non-enumerable).

It's unfortunate that stack isn't standardized, but I concur with @ljharb. For reference, the AggregateException in .NET includes not only the stack trace of the AggregateException itself, but the stacks for each inner exception as well:

System.AggregateException: One or more errors occurred.
   <AggregateException stack trace>
---> (Inner Exception #0) System.Exception: foo
   <First exception stack trace>
---> (Inner Exception #1) System.Exception: bar
   <Second exception stack trace>

.NET's AggregateException also has a Flatten() method that unnests nested AggregateExceptions.

[ljharb]

Since non-errors can also be thrown, not everything in the errors array will have a stack trace.

[rbuckton]

Since non-errors can also be thrown, not everything in the errors array will have a stack trace.

In the AggregateException case, its actually doing ex.ToString() (which happens to include the stack trace). In our case it would likely be ? ToString(error) (which also happens to include the stack trace).

Either way, stack isn't currently standardized. Until it is, we can just hope to provide a recommendation to the various engines for its representation.

[ljharb]

Any recommendation to display a different kind of stack trace than all the current ones makes standardizing stack traces that much harder; I’d prefer to recommend that the stack trace ignore the errors array entirely.

[domenic]

It's worth noting that there's a difference between the JS-exposed stack property, for which I second @ljharb's call for simplicity, vs. what devtools prints when you log an AggregateError. The latter is not exposed and thus can be arbitrarily complicated (and arbitrarily helpful).

[Ginden]

Almost all of ES-defined properties on builtin objects are non-enumerable, so I think that errors should be non-enumerable too.

I can think of another concern:

const foo = new AggregateError([new TypeError, new RangeError], 'foo');
const bar = new AggregateError([foo], 'bar');
const baz = [...bar].map(e=>e.constructor);

What should baz contain? I would expect one of following:

• [AggregateError]
• [AggregateError, TypeError, RangeError]
• [TypeError, RangeError]

No. 2 seems most useful.

[bakkot]

I'm fine with switching errors to be non-enumerable, since it seems most people's intuition runs that way.

@Ginden, that code should throw a TypeError, because instances of AggregateError (e.g. bar) should not be iterable. But bar.errors should definitely a single AggregateError (i.e. foo) whose .errors holds the TypeError and RangeError. I can't really see how it would be otherwise - are you expecting the language to do some sort of auto-flattening behind the scenes? I think that sort of magic is very bad.

[mathiasbynens]

Per today’s TC39 meeting, we have consensus on errors being non-enumerable (to match message)

[chicoxyzzy]

I think that @rbuckton's idea about splitting AggregateError to a separate proposal is a good idea. We have two copies of spec text for AggregateError in two repos now with some minor differences.

[ljharb]

both however are at different stages. I think it’s fine to have the first one to land be the one to add AggregateError.

[chicoxyzzy]

This issue seems to be resolved now. Closing it.

[felixfbecker]

I'd like to say that it would make serializing errors over JSON (HTTP, RPC, IPC, etc) would be easier if errors was enumerable. Currently you usually do something like this:

send({ message: error.message, ...error })

as special-handling for error.message, because it is the one property on every error that is non-enumerable. But if errors is non-enumberable too, special handling needs to be added specifically for AggregateError (because that's the only Error that has errors).

Is there any practical reason (other than consistency) to make it non-enumberable?

[bakkot]

But if errors is non-enumberable too, special handling needs to be added specifically for AggregateError (because that's the only Error that has errors).

You'd need special handling anyway if you want to serialize the message properties of the errors in the errors array (which you evidently do).

[felixfbecker]

Good point, but it is still simpler:

JSON.stringify(error, value => value instanceof Error ? { message: error.message, ...error } : error)