Pass TimeoutErrors to beforeError hooks

[sean-lagger]

As of now, the beforeError hook only handles HTTPErrors. A hook for handling errors like TimeoutError would be beneficial to avoid having to wrap every request call in try { ... } catch { ... } blocks

[cerinoligutom]

Is there a particular reason why TimeoutErrors do not go to the beforeError hook?

Our use case is to centralize all errors coming from this library and we're expecting that TimeoutError would go through beforeError as well, however it does not so we end up having to do try...catch statements downstream everywhere just so we could catch the TimeoutError thrown by this library. Is there a more elegant way to achieve this with the current API as of v0.33.3?

[sindresorhus]

The hook was originally designed to let users enrich HTTP response errors. It also does not receive network errors.

Just out of curiosity, if it would receive TimeoutError, what would you do with it in the hook?

[sean-lagger]

Basically the intent is to have a centralized way to handle errors like TimeoutError so that we don't have to run a try...catch for every call while still leveraging the Ky instance.

"Just out of curiosity, if it would receive TimeoutError, what would you do with it in the hook?"
Primarily communicating that a timeout error has occurred to the user in the UI, amongst other things

[sindresorhus]

Primarily communicating that a timeout error has occurred to the user in the UI, amongst other things

The hook expects you to return an error though, so even if it supported receiving TimeoutError, you would not be able to silence the error.

The hook function receives a HTTPError as an argument and should return an instance of HTTPError. - https://github.com/sindresorhus/ky#hooksbeforeerror

[sindresorhus]

Basically the intent is to have a centralized way to handle errors like TimeoutError so that we don't have to run a try...catch for every call while still leveraging the Ky instance.

Lets imagine that Ky supported this. What would happen on error? You handle the error in beforeError, but the await ky() call needs to resolve somehow. You don't want it to throw, but it cannot resolve to a successful response either.

[cerinoligutom]

I suppose with the current design, either way, we'll have to try...catch by the end of the chain since we still need to return an HTTPError. For our purposes, we just simply need a middleware-like hook that we can listen to for all incoming errors from this lib including TimeoutErrors but I suppose that goes against the current design of beforeError 🤔

[ChoJongHoon]

How about adding a separate beforeTimeoutError from beforeError?

Usage

import ky from 'ky';

await ky('https://example.com', {
	hooks: {
		beforeTimeoutError: [
			error => {
				// Send logs, show toast, or... etc.

				return error;
			}
		]
	}
});

My patch

diff --git a/distribution/core/Ky.js b/distribution/core/Ky.js
index f827f13cc3a38a75356b2ad15b505441a1d379ad..6827ab8abee88fb95d58207597e53fab6b803023 100644
--- a/distribution/core/Ky.js
+++ b/distribution/core/Ky.js
@@ -15,7 +15,18 @@ export class Ky {
             }
             // Delay the fetch so that body method shortcuts can set the Accept header
             await Promise.resolve();
-            let response = await ky._fetch();
+            let response
+            try {
+                response = await ky._fetch();
+            } catch (error) {
+                if (error instanceof TimeoutError) {
+                    for (const hook of ky._options.hooks.beforeTimeoutError) {
+                        // eslint-disable-next-line no-await-in-loop
+                        error = await hook(error);
+                    }
+                }
+                throw error;
+            }
             for (const hook of ky._options.hooks.afterResponse) {
                 // eslint-disable-next-line no-await-in-loop
                 const modifiedResponse = await hook(ky.request, ky._options, ky._decorateResponse(response.clone()));
@@ -114,6 +125,7 @@ export class Ky {
                 beforeRetry: [],
                 beforeError: [],
                 afterResponse: [],
+                beforeTimeoutError: [],
             }, options.hooks),
             method: normalizeRequestMethod(options.method ?? this._input.method),
             // eslint-disable-next-line @typescript-eslint/prefer-nullish-coalescing
diff --git a/distribution/types/hooks.d.ts b/distribution/types/hooks.d.ts
index 158198a6515e33f0af159557f38faa5d02d7b783..9ef26124aa717ae8d7136c78a6af5029b0e10c90 100644
--- a/distribution/types/hooks.d.ts
+++ b/distribution/types/hooks.d.ts
@@ -1,5 +1,5 @@
 import { stop } from '../core/constants.js';
-import { HTTPError } from '../index.js';
+import { HTTPError, TimeoutError } from '../index.js';
 import type { NormalizedOptions } from './options.js';
 export type BeforeRequestHook = (request: Request, options: NormalizedOptions) => Request | Response | void | Promise<Request | Response | void>;
 export type BeforeRetryState = {
@@ -11,6 +11,7 @@ export type BeforeRetryState = {
 export type BeforeRetryHook = (options: BeforeRetryState) => typeof stop | void | Promise<typeof stop | void>;
 export type AfterResponseHook = (request: Request, options: NormalizedOptions, response: Response) => Response | void | Promise<Response | void>;
 export type BeforeErrorHook = (error: HTTPError) => HTTPError | Promise<HTTPError>;
+export type BeforeTimeoutErrorHook = (error: TimeoutError) => TimeoutError | Promise<TimeoutError>;
 export interface Hooks {
     /**
     This hook enables you to modify the request right before it is sent. Ky will make no further changes to the request after this. The hook function receives normalized input and options as arguments. You could, forf example, modiy `options.headers` here.
@@ -111,4 +112,6 @@ export interface Hooks {
     ```
     */
     beforeError?: BeforeErrorHook[];
+
+    beforeTimeoutError?: BeforeTimeoutErrorHook[];
 }

[sindresorhus]

How about adding a separate beforeTimeoutError from beforeError?

What's your argument for making another hook?

[sindresorhus]

If we do decide to do this, it should be named beforeTimeout.

[sholladay]

As I see it, there are really two issues at play here.

1. Handling errors is genuinely a bit confusing since beforeError doesn't listen for all error types.
2. Your code is designed in such a way where you must try / catch each Ky call individually.

I think the main problem is 2. I would recommend that you call Ky without a local try / catch. Let the errors bubble up. You can handle them in a centralized manner by using something like a React Error Boundary. On the server, hapi.js for example will automatically catch any errors thrown by route handlers, so if Ky throws in a route, it will automatically return a 500 error and there are hooks in hapi to modify how it handles those errors. If you're using another framework, worst case scenario you could use process.on('uncaughtException', ...). The point is, find a good boundary where you can handle errors.

I think of beforeError as just a place to make quick little modifications to HTTP error messages or whatever, in an app where more comprehensive error handling is not necessary. That said, I would support giving beforeError more errors.

[zaubara]

I just came across this; i wanted to have a "busy" indicator that works across all requests, regardless of what it's doing, it's working when the request succeeds or gets denied, but on a timeout, the indicator would get "stuck". I don't want to edit the error, I just wanted to be informed of it, basically. That's only possible outside of ky, currently.

[pat-richter]

Basically the intent is to have a centralized way to handle errors like TimeoutError so that we don't have to run a try...catch for every call while still leveraging the Ky instance.

Lets imagine that Ky supported this. What would happen on error? You handle the error in beforeError, but the await ky() call needs to resolve somehow. You don't want it to throw, but it cannot resolve to a successful response either.

I would say it should be possible to simply and automatically start the retry logic. In my tests, retry already triggered when my PC was without networkt before I start the requests; however, if requests already running, ky waits for the response and gets a timeout. While I can restart the request manually (requires external logic, mimic delay, ...) or set timeout to 9999 minutes (can't give an easy early feedback like "response failed, retrying"), I would like to streamline the logic.

[sholladay]

I think whenever Ky encounters an error that we can easily pass to beforeError, we should do so. Then, hooks can override what will be thrown by mutating the error or throwing or returning an Error themselves (only throw stops running hooks).

Make a new issue if you want an option for Ky to swallow the error and resolve with undefined. The caller would be responsible for conditionally handling the undefined success case.

[borodinalive]

Just out of curiosity, if it would receive TimeoutError, what would you do with it in the hook?

We now have a handy entry point to handle any errors in our app. When an error occurs, we'll show a friendly notification letting you know what went wrong. So, if I add a new API endpoint, I won't need to add this part of the functionality explicitly. How I'll manage the error in the try-catch block is the second question.

At the moment I have to add this duplicating logic to ALL(!) API calls in every try-catch.

[HappyHidra]

If we do decide to do this, it should be named beforeTimeout.

That would be great if you will add such hook. Could be very useful in some cases like the one above.