Throw custom error types instead of generic errors inside the HubConnection class
- Dominant language
- C#
- Stars
- 38.4k
- Forks
- 10.9k
- Avg merge
- 2d 5h
- Merged PRs (30d)
- 276
Description
## 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](https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/cause#providing_structured_data_as_the_error_cause) 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.
```diff
--- 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.
```ts
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.
Contributor guide
Assessment
This issue has not been assessed yet.