Ensure all errors thrown by TdpClient are instances of Error class

[gzdunek]

The error listener in TdpClient specifies its error type as Error, but this assumption may not always be true at runtime. In JS, it's possible to throw any value: instances of Error, but also strings, numbers, etc.

To improve this, I changed the following:

1. Added a ensureError that can convert any value to Error instance.
   • getErrorMessage uses this function under the hood now.
2. Added types to EventEmitter in TdpClient to make it more difficult to emit wrong value for a given even type.

[ravicious]

Do you maybe have any articles on consequences of using this approach? By "this approach" I really mean trying to paper over the fact that error handling in JS is kinda bad.

I think it's fine if this function is meant to fix an issue in TdpClient, but if we were to use it in every catch statement, I'd definitely want to maybe see if there's someone else who did this and if they ran into any unforeseen problems.

But it also makes me think, is ensureError necessary in TdpClient in the first place? Does TdpClient or its users need to do anything specific with the error? If so, would it be enough if it used unknown instead of Error and the callsites first checked instanceof instead? I wonder why it's important for values in TdpClient to always be of Error class.

Often when an API is bad on a language level, I feel like it's better to pass that awfulness further rather than trying to paper over it. Let the callsites deal with something other than an Error being thrown. Trying to paper over it leads to some tricky edge cases like returning new Error() in response to null or undefined being thrown.

[gzdunek]

I just found an article with the same approach: https://medium.com/with-orus/the-5-commandments-of-clean-error-handling-in-typescript-93a9cbdf1af5

MDN mentions error normalization https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Error/isError#normalizing_caught_errors

But it also makes me think, is ensureError necessary in TdpClient in the first place? Does TdpClient or its users need to do anything specific with the error? If so, would it be enough if it used unknown instead of Error and the callsites first checked instanceof instead? I wonder why it's important for values in TdpClient to always be of Error class.

Yeah, ensureError it's not strictly needed, I even initially planned to use unknown.
But then I thought that maybe it's better to return something more meaningful than unknown, so callsites don’t have to guess how to handle it.

I took a look at how we handle errors across different parts of the codebase, and it seems we often assume that any thrown value is an instance of Error (I know that useUnknownInCatchVariables would help with it). But still normalizing the error could help reduce edge cases. As mentioned in the first article, handling the basic case (err instanceof Error) is straightforward but it's often not clear how to deal with the rest.

Some examples where normalizing the error first would make things easier:

• Reading from error.message without checking the Error type:

  teleport/web/packages/teleport/src/AppLauncher/AppLauncher.tsx

  Line 150 in 2d29769

  statusText = `Error while authenticating a required app: ${err.message}`;

• Reading from error.message without checking the Error type:

  teleport/web/packages/shared/hooks/useInfiniteScroll/useKeyBasedPagination.ts

  Line 148 in d6fa030

  attempt: { status: 'failed', statusText: err.message, statusCode },

• handleRequestError takes (e: Error) and a string to concatenate to it:

  teleport/web/packages/teleport/src/Discover/Database/CreateDatabase/useCreateDatabase.ts

  Line 295 in 5b33d41

  handleRequestError(e, 'failed to register database:');

Anyway, I'm fine with removing ensureError but maybe there's still some value in it :)

[ravicious]

But then I thought that maybe it's better to return something more meaningful than unknown, so callsites don’t have to guess how to handle it.

But that's the thing, most of the time they don't have to guess because they just want to be able to serialize the error somehow, unless they do something special when a specific error is thrown.

I checked how TdpClientEvent.ERROR is used. There are three callsites, one of them ignores the error altogether and the other two do error.message || error.toString().

Callsites

teleport/web/packages/teleterm/src/ui/DocumentDesktopSession/DocumentDesktopSession.tsx

Lines 107 to 113 in fabcd1a

	useListener(
	client.onError,
	useCallback(
	() => documentsService.update(uri, { status: 'error' }),
	[documentsService, uri]
	)
	);

teleport/web/packages/teleport/src/Player/DesktopPlayer.tsx

Lines 145 to 148 in fabcd1a

	const clientOnError = useCallback((error: Error) => {
	setPlayerStatus(StatusEnum.ERROR);
	setStatusText(error.message || error.toString());
	}, []);

teleport/web/packages/shared/components/DesktopSession/DesktopSession.tsx

Lines 140 to 152 in fabcd1a

	const handleFatalError = useCallback(
	(error: Error) => {
	setDirectorySharingState(defaultDirectorySharingState);
	setClipboardSharingState(defaultClipboardSharingState);
	setTdpConnectionStatus({
	status: 'disconnected',
	fromTdpError: error instanceof TdpError,
	message: error.message || error.toString(),
	});
	initialTdpConnectionSucceeded.current = false;
	},
	[setClipboardSharingState, setDirectorySharingState]
	);

Of course, without typing error as Error in an unsound way, the error handling code wouldn't be as straightforward.

I'm just not fully sold on using ensureError for absolutely everything, but I have nothing against giving it a try.

[gzdunek]

I'm just not fully sold on using ensureError for absolutely everything, but I have nothing against giving it a try.

I agree, we don't have to use it everywhere. We can give it a try in a few places, just to see how it feels in practice.

I fixed the callsites, they no longer need || error.toString().

[kimlisa]

just curious, i've been using input == null to test for both null and undefined, should i be doing it more explicitly like this?

[gzdunek]

I use the explicit version because I can never remember the coercion rules for == 😅

It’s probably just personal preference, but I find === a lot easier to reason about.

[backport-bot-workflows]

@gzdunek See the table below for backport results.

Branch	Result
branch/v16	Failed
branch/v17	Failed