fix(cli/dts): Type Deno.errors.* as subclasses of Error

[Liamolucko]

Fixes #10640

[bartlomieju]

We need to be very careful if we want to land this PR, it must be backwards compatible change

[kitsonk]

Thanks for the contribution. I think we need to go a slightly different route to ensure we don't have any unintentional breakages in existing code, so we need to effectively "narrow" the existing types instead of introducing new classes. I would prefer if we did something like this:

declare namespace Deno {
  interface DenoError<N extends string> extends Error {
    name: N;
  }

  interface DenoErrorConstructor<N extends string> extends ErrorConstructor {
    new (message?: string): DenoError<N>;
    (message?: string): DenoError<N>;
    readonly prototype: DenoError<N>;
  }

  /** A set of error constructors that are raised by Deno APIs. */
  export const errors: {
    AddrInUse: DenoErrorConstructor<"AddrInUse">;
    AddrNotAvailable: DenoErrorConstructor<"AddrNotAvailable">;
    AlreadyExists: DenoErrorConstructor<"AlreadyExists">;
    BadResource: DenoErrorConstructor<"BadResource">;
    Busy: DenoErrorConstructor<"Busy">;
    BrokenPipe: DenoErrorConstructor<"BrokenPipe">;
    ConnectionAborted: DenoErrorConstructor<"ConnectionAborted">;
    ConnectionRefused: DenoErrorConstructor<"ConnectionRefused">;
    ConnectionReset: DenoErrorConstructor<"ConnectionReset">;
    Http: DenoErrorConstructor<"Http">;
    Interrupted: DenoErrorConstructor<"Interrupted">;
    InvalidData: DenoErrorConstructor<"InvalidData">;
    NotConnected: DenoErrorConstructor<"NotConnected">;
    NotFound: DenoErrorConstructor<"NotFound">;
    NotSupported: DenoErrorConstructor<"NotSupported">;
    PermissionDenied: DenoErrorConstructor<"PermissionDenied">;
    TimedOut: DenoErrorConstructor<"TimedOut">;
    UnexpectedEof: DenoErrorConstructor<"UnexpectedEof">;
    WriteZero: DenoErrorConstructor<"WriteZero">;
  };
}

We should also provide a unit tests to validate the types are working as expected. The tests in cli/tests/unit/ will be type checked as part of the build, and so while the code can be non functional it should at least type check. Maybe something like:

unitTest(function errorTypeChecking() {
  const err: unknown = new Deno.errors.NotFound();
  if (err instanceof Deno.errors.NotFound) {
    assertEquals(err.name, "NotFound");
  }
}

If the type narrowing doesn't work, the property .name won't be on err.

[sant123]

@Liamolucko Do you think that is ok? 😅

[Liamolucko]

I would prefer if we did something like this:

declare namespace Deno {
  interface DenoError<N extends string> extends Error {
    name: N;
  }

  interface DenoErrorConstructor<N extends string> extends ErrorConstructor {
    new (message?: string): DenoError<N>;
    (message?: string): DenoError<N>;
    readonly prototype: DenoError<N>;
  }

  /** A set of error constructors that are raised by Deno APIs. */
  export const errors: {
    AddrInUse: DenoErrorConstructor<"AddrInUse">;
    AddrNotAvailable: DenoErrorConstructor<"AddrNotAvailable">;
    AlreadyExists: DenoErrorConstructor<"AlreadyExists">;
    BadResource: DenoErrorConstructor<"BadResource">;
    Busy: DenoErrorConstructor<"Busy">;
    BrokenPipe: DenoErrorConstructor<"BrokenPipe">;
    ConnectionAborted: DenoErrorConstructor<"ConnectionAborted">;
    ConnectionRefused: DenoErrorConstructor<"ConnectionRefused">;
    ConnectionReset: DenoErrorConstructor<"ConnectionReset">;
    Http: DenoErrorConstructor<"Http">;
    Interrupted: DenoErrorConstructor<"Interrupted">;
    InvalidData: DenoErrorConstructor<"InvalidData">;
    NotConnected: DenoErrorConstructor<"NotConnected">;
    NotFound: DenoErrorConstructor<"NotFound">;
    NotSupported: DenoErrorConstructor<"NotSupported">;
    PermissionDenied: DenoErrorConstructor<"PermissionDenied">;
    TimedOut: DenoErrorConstructor<"TimedOut">;
    UnexpectedEof: DenoErrorConstructor<"UnexpectedEof">;
    WriteZero: DenoErrorConstructor<"WriteZero">;
  };
}

That fixes the issue with Error, but Deno.errors.* are still instanceof each other:

let err = new Deno.errors.AddrInUse();
if (err instanceof Deno.errors.AddrNotAvailable) {
    // Property 'message' does not exist on type 'never'.
    //   The intersection 'DenoError<"AddrInUse"> & DenoError<"AddrNotAvailable">' was reduced to 'never' because property 'name' has conflicting types in some constituents.
    //
    // I think TypeScript initially thinks the `instanceof` is always true but then gets confused because the types are incompatible.
    err.message
} else {
    // Property 'message' does not exist on type 'never'.
    err.message
}

I think TypeScript considers generics to never exist at runtime, so all versions of DenoError are considered instances of the same class.

Also, Deno.errors.* aren't currently callable as functions so that would have to be added for ErrorConstructor to be correct.

[sant123]

What do you think @kitsonk? 😁

[bartlomieju]

If this change is backwards compatible I would love to get this into 1.12

[bartlomieju]

I verified that this PR causes no problems when running tests for deno_std (0.103.0)

[sant123]

So do you think this can be merged?

[bartlomieju]

LGTM, thank you Liam and sorry it took so long to merge it.

[lucacasonato]

These changes broke old std versions: https://github.com/lucacasonato/esbuild_deno_loader/runs/3201002174

[sant123]

It breaks until [email protected] according with denoland/deno_std#831

[sant123]

name: "UnexpectedEof" | "PartialReadError";

This fixes the error but may introduce future issues with name typing. Since is expected to have just one value.

[Liamolucko]

It's not really that important for name to be statically typed, so the simplest way to deal with it would probably just be to change it back to string.

[sant123]

Or to not support versions below [email protected] 🤭 but got your point, thank you!!!