Throw custom error types instead of generic errors inside the HubConnection class

[qnku]

Background and Motivation

This is about the JavaScript SignalR client ("dotnet/aspnetcore/src/SignalR/clients/ts/signalr").

The HubConnection class may throw errors (i.e. with the message "Error: Server returned handshake error: Handshake was canceled."). These errors propagate up to the application layer. Currently, it is challenging to reliably identify these errors as originating from SignalR because the HubConnection class creates plain Error types without any distinct characteristics. In our scenario, we have implemented a global error handler that notifies users visually when an unhandled error occurs.

However, since SignalR errors can occur sporadically and independently of user actions, it is desirable to filter out these errors. Presently, we achieve this by checking if the error's message property contains a specific string, like the one mentioned earlier. Unfortunately, this approach is not reliable, as the error message can change at any time, and there are multiple variations.

The proposal is to follow best practices and define a custom error type and/or set the cause field for library specific errors. This will allow library consumer to properly catch errors thrown by the library.

From MDN

You might want to define your own error types deriving from Error to be able to throw new MyError() and use instanceof MyError to check the kind of error in the exception handler. This results in cleaner and more consistent error handling code. https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error

Interestingly, the Error.ts file already contains some error types; however, they seem to be exclusively used by the HTTP wrapper, leaving the other SignalR errors unaccounted for.

Proposed API

1) Add a new error class (derived from Error) and set the name field 2) Change code that uses new Error to use new HubConnectionError instead

Unfortunately, I don't understand why the existing error types use a strange prototype workaround. I have read that it is supposed to make instanceof work correctly. However, in my tests, that didn't change anything, as instanceof checks still don't work. Therefore, it seems like it doesn't do anything? I'd be interested in the background on that.

Due to this reason, I have not followed this pattern in this proposal. Instead, the name field in the constructor is set. While not perfect (i.e. other error types might use the same name), setting the name field is a common practice and essential for making the error identifiable during a catch.

--- a/src/SignalR/clients/ts/signalr/src/Errors.ts
+++ b/src/SignalR/clients/ts/signalr/src/Errors.ts
+/** Error thrown by HubConnection class. */
+export class HubConnectionError extends Error {
+    /** Constructs a new instance of {@link @microsoft/signalr.HubConnectionError}.
+     *
+     * @param {string} errorMessage A descriptive error message.
+     */
+    constructor(errorMessage: string) {
+        super(errorMessage);
+
+        this.name = "HubConnectionError";
+    }
+}
+
--- a/src/SignalR/clients/ts/signalr/src/HubConnection.ts
+++ b/src/SignalR/clients/ts/signalr/src/HubConnection.ts
-import { AbortError } from "./Errors";
+import { AbortError, HubConnectionError } from "./Errors";
export class HubConnection {
             const message = "Server returned handshake error: " + responseMessage.error;
             this._logger.log(LogLevel.Error, message);

-            const error = new Error(message);
+            const error = new HubConnectionError(message);
             this._handshakeRejecter(error);
             throw error;
         } else {

Usage Examples

Now we can properly catch the error by checking the name property that was set in the error constructor. Using instanceof for error checks is known to be problematic so we avoid it.

try {
    throw new HubConnectionError("test");
}
catch (error: any) {
    if (error.name === "HubConnectionError") {
        console.log('SignalR Error', error.name);
    } else {
        console.log('Other Error', error.name);
    }
}

Alternative Designs

As an alternative to introducing new error types, we could populate the cause field mentioned earlier with a value or object structure that allows us to identify these error instances as SignalR errors.

const DEFAULT_ERROR_CAUSE = Object.freeze({
    source: 'signalr',
});

function createError(message: string) {
    throw new Error(message, { cause: DEFAULT_ERROR_CAUSE });
}

// ...
createError(message);

Risks

• This change might be considered breaking, depending on how library consumers handle error instances.
• Since the cause property is not available in "es2019" (introduced in es2022), it is likely unsuitable as it won't function on older browsers.

[flensrocker]

Maybe related to #39079 with the need to get the proper status codes from negotiation.

[luga97]

This looks like a potential good first issue for me. I would like to collaborate, If I can, there is some doc related about how to start?