ITwinError changes

[karolis-zukauskas]

Is your feature request related to a problem? Please describe.
ITwinError has been recently added (under beta tag), see #6909.

While this new interface supports passing "information rich" errors from Backend to Frontend, there is now a problem of having two different interfaces for errors. One of the driving examples for improved errors was the "lock in use" error that originates in imodels-client package - detailed error information in the case of "lock in use" is lost when internal package error is converted to "platform flavour" IModelError. Now with the new ITwinError, imodels-client could start throwing it, but that cannot be easily done because it is a breaking change - major version update needed ... Concern here is that ITwinError will not be adopted and platform will have inconsistent errors.

Describe the solution you'd like
A backwards compatible solution. Perhaps a better approach would be to retain the one and only BentleyError class, don’t subclass it and implement ITwinError interface on it. Make the problematic errorNumber deprecated and have namespace + errorKey as the new way of checking error types (we can keep the assert function approach introduced with ITwinError). Advantage is that we stay with one error interface and updates to it are backwards compatible – imodels-client can continue to throw IModelError (derived from BentleyError) just pass in the additional properties for the object. Over time, platform could deprecate existing BentleyError derived classes (like IModelError).

Simplified version of BentleyError expansion:

interface ITwinError {
    namespace: string;
    errorKey: string;
}

class BentleyError extends Error implements ITwinError {
    // @deprecated ... use namespace + errorKey
    public errorNumber: number;
    public namespace: string;
    public errorKey: string;

    // New constructor
    public constructor(namespace: string, errorKey: string, message?: string);

    // Existing constructor (deprecated)
    public constructor(errorNumber: number, message?: string);

    public constructor(namespaceOrErrNumber?: number | string, errorKeyOrMessage?: string, message?: string) {
        super("TODO");
        this.namespace = "TODO";
        this.errorKey = "TODO";
        this.errorNumber = 0;
    }
}

// "derived" error example:

interface FooError extends ITwinError {
    namespace: "myPackage";
    errorKey: "foo";
    extraDetail: number;
}

function throwFooError(message: string, extraDetail: number): never {
    const error = new BentleyError("myPackage", "foo", message);
    Object.assign(error, { extraDetail });

    throw error;
}

function isFooError(error: unknown): error is FooError {
    return error instanceof BentleyError && error.namespace === "myPackage" && error.errorKey === "foo";
}

[nick4598]

Thanks for creating this issue. The current plan is to deprecate all subclasses of Error and replace them with ITwinError for 5.0. I would hope that would be sufficient for eventually ending up with consistency among Errors within the platform. Ideally the imodels-clients repo would release a new major version which depends on itwinjs 5.0 or above and also relies on ITwinErrors. Unfortunately I think no matter what we do, itwinjs 5.0 will find us in a weird in-between state where some errors are ITwinErrors and some are Bentley/IModelErrors. The original motivation for ITwinError came from this issue.

Notable quote from this private issue:

In general, I think our subclasses of Error are problematic and we should work towards eliminating them rather than propagating them. For one thing, the "hierarchy" of them is confusing and typeof tests are the root of lots of problems. Also, they can't be thrown from native code, which is why I purposely left these of type Error rather than IModelError with the hope of eventually eliminating IModelError entirely.

I worry about keeping BentleyError around due to the concerns mentioned in the above quote. I'm also not sure that having BentleyError hold these properties makes the transition much different/easier for a repo like imodels-clients. Any error object that benefits from the 'information rich' approach still requires a new interface to be defined which extends ITwinError. Otherwise it would be about mapping their API error codes to an errorKey as opposed to an errorNumber.
Do you think the plan to deprecate BentleyError and IModelError resolves your concern about platform having inconsistent errors?

Other thoughts

I do think your approach might let imodels-clients keep their peerDependency to say they support 4.0 and 5.0 of itwinjs.. but I'm not 100% sure. How would the imodels-clients repo know if they were given an itwinjs version of 5.0 or 4.0? Implementing your approach would probably mean we'd add the optional errorKey and namespace properties to the currently available constructor. I don't see that its possible to have two constructors in typescript.

[karolis-zukauskas]

@nick4598,
I'm down to get rid of BentleyError entirely, but I would like to avoid having to live through 5.x with two error types. What I mean is that I don't want to check for both of them in catch blocks. And we can avoid this awkwardness - implement ITwinError for BentleyError - map existing errorNumber (i.e. IModelStatus and friends) to an errorKey and done.

I want this code to work in iTwin.js 5.x:

try {
  throw new IModelError(IModelStatus.BadArg, "random example");
} catch (e: unknown) {
  if (ITwinError.isITwinError(e) && e.namespace === "itwinjs-core" && e.errorKey === "badArg") {
    // ... can catch IModelError here
  }

  throw e;
}

Do you think the above code is not reasonable to support in 5.x?

Other thoughts

I don't see that its possible to have two constructors in typescript.

You can overload signatures and then deprecate one of the signatures, see https://www.typescriptlang.org/docs/handbook/2/functions.html#function-overloads

[nick4598]

I see now, I think its a good idea. I will work on it for 5.0, but may be sometime before I get to it.