TypeScript icon indicating copy to clipboard operation
TypeScript copied to clipboard

Make AggregateError generic to represent error types

Open ethanresnick opened this issue 1 year ago • 7 comments

lib Update Request

Make it possible for an AggregateError to carry a type parameter representing the shape of the items in its errors array, while preserving backwards compatibility using type parameter defaults.

Specifically, the current definition is:

// In lib.es2021.promise.d.ts
interface AggregateError extends Error {
    errors: any[]
}

interface AggregateErrorConstructor {
    new(errors: Iterable<any>, message?: string): AggregateError;
    (errors: Iterable<any>, message?: string): AggregateError;
    readonly prototype: AggregateError;
}

// In lib.es2022.error.d.ts; adds `options` argument.
interface AggregateErrorConstructor {
    new (
        errors: Iterable<any>,
        message?: string,
        options?: ErrorOptions
    ): AggregateError;
    (
        errors: Iterable<any>,
        message?: string,
        options?: ErrorOptions
    ): AggregateError;
}

I'm proposing instead:

// In lib.es2021.promise.d.ts
interface AggregateError<T = any> extends Error {
    errors: T[]
}

interface AggregateErrorConstructor {
    new<T = any>(errors: Iterable<T>, message?: string): AggregateError<T>;
    <T = any>(errors: Iterable<T>, message?: string): AggregateError<T>;
    readonly prototype: AggregateError;
}

// In lib.es2022.error.d.ts
interface AggregateErrorConstructor {
    new<T = any>(errors: Iterable<T>, message?: string,
        options?: ErrorOptions): AggregateError<T>;
    <T = any>(errors: Iterable<T>, message?: string,
        options?: ErrorOptions): AggregateError<T>;
    readonly prototype: AggregateError;
}

Again, the only difference is the addition of a new T parameter defaulting to any (the old value), with the errors key updated from any[] to T[].

Configuration Check

My compilation target is ES2022 and my lib is ["ES2022", "DOM"].

Missing / Incorrect Definition

NA, as the property isn't missing; it's just underspecified.

I think this would be a nice quality of life improvement, esp for code that returns AggregateErrors (sorta analogous to the functional style of returning a Result type) rather than throwing them.

Sample Code

// Current
const x = new AggregateError([new Error("Something something...")]).errors; // type is any[]

// With proposal
const x = new AggregateError([new Error("Something something...")]).errors // type is Error[]

Documentation Link

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/AggregateError/errors

ethanresnick avatar Apr 28 '23 19:04 ethanresnick

If this is accepted, I'm happy to put up a PR.

ethanresnick avatar Apr 28 '23 19:04 ethanresnick

This doesn’t help you much without typed catch:

try { /* do something that can throw an AggregateError */ }
catch (e) {
    if (e instanceof AggregateError) {
        // e is now AggregateError<any>
    }
}

Unless you’re doing something else with the error after you construct it other than throwing it, the extra type info doesn’t seem to buy you anything.

fatcerberus avatar Apr 29 '23 07:04 fatcerberus

I agree that the value is lost on throw, but there’s lots of code (including the code I’m dealing with) that returns AggregateError (or a value containing it), like JS versions of the Result type from many languages

ethanresnick avatar Apr 29 '23 17:04 ethanresnick

I'd be super curious to look at this code that returns exception values through normal control flow

RyanCavanaugh avatar May 02 '23 17:05 RyanCavanaugh

@RyanCavanaugh It's something like this (simplified):

function validate(
  schema: FieldDefinition[],
  item: { [field: string]: JSON }
): ValidatedItem | AggregateError {
  const errors = schema.flatMap((field) => {
    const value = item[field.name];
    // validate value against field definition in schema, producing 0-n errors
    return [/* errors for field */];
  });

  return errors.length > 0
    ? new AggregateError(errors)
    : instantiateOpaqueType<ValidatedItem>(item);
}

Again, the union return prevents chaining (relative to using some Result type), but it's enough for our needs

ethanresnick avatar May 02 '23 18:05 ethanresnick

Why use the built-in exception type though, that's the part I don't get

RyanCavanaugh avatar May 03 '23 02:05 RyanCavanaugh

Why use the built-in exception type though, that's the part I don't get

What's the alternative you're imagining? Just returning an Error[]? This code could've used Error[], I suppose, because an array is never returned in a success case. But, if an array were a legal return value, then AggregateError seems like a much clearer indication that the return value is from the failure case, and it seems much more convenient for caller to be able to discriminate the return value with res instanceof Error rather than, like Array.isArray(res) && res.some(it => instanceof Error).

ethanresnick avatar May 03 '23 05:05 ethanresnick

well I'd be happy to have a standardized Error in general .i can't believe the default after nearly 30 years of javascript is: whatever deal with it^^

Dreathorian avatar Jul 13 '24 16:07 Dreathorian

Why use the built-in exception type though, that's the part I don't get

Obviously I can't speak for everyone, but at work we use Errors like this as well to populate an errors array when returning HTTP responses. We do something similar to what @ethanresnick showed above for validating request bodies, but we have a few instances where, while validating, we also make calls to our DB to see if resources exist or not and compare the state of the data to determine if we have a conflict or not. Once we've finished it, we return a custom AggregateError with an overall status code that will be used to set our response bad. It's annoying to have to force TypeScript that our errors array is all of typed as our custom ServiceError when we could just do AggregateError<ServiceError>.

AllusiveBox avatar Jul 25 '24 17:07 AllusiveBox