Allow for logging network errors and JSON parse errors

[danbahrami]

Hi! I'd like to contribute to ky for selfish reasons. I want to use it in my org's production app however, after trying to hook up the necessary logging we need I've come to a dead end.

The logging that I need is:

• at the start of every request (the beforeRequest hook works for this)
• at the end of every request (afterResponse works for success and HTTPError responses but not Network Errors)
• when a network error is received (there are no hooks that currently cover this case)
• when Response.json() fails

So that leaves me with two gaps:

1. Hooking into Response.json() errors
2. Hooking into network errors

I'd like to get a rough steer on an approach for these both before creating a PR.

Hooking into Response.json() errors

My first attempt at this was to try to do it in my own parseJson method:

const api = ky.create({
    ...
    parseJson: (text) => {
        try {
            JSON.parse(text);
        } catch (e) {
            log(e); // log errors here
            throw e
        }
    }
})

The problem with this approach is that, within the context of parseJson you don't have access to the request context so you can't send any information about the specific request along with the error.

Possible solutions

I can think of two sensible options:

1. Pass the request information to the parseJson option
This would look something like:

const api = ky.create({
    ...
    parseJson: (text, { request, options }) => {
        try {
            JSON.parse(text);
        } catch (e) {
            log(e, { url: request.url, headers: options.headers }); // log errors here
            throw e
        }
    }
})

not sure exactly what you'd want to pass in that second argument, could potentially contain the response as well if that's feasible.

2. A new hook - beforeJsonParseError
This would be more fitting with how we can hook into HTTP Errors.

type BeforeJsonParseErrorState = {
    request: Request;
    options: NormalizedOptions;
    response: Response
    error: Error;
};

type BeforeJsonParseErrorHook = (options: BeforeJsonParseErrorState) => Error | void;

Let me know if either of those sound reasonable or if you have any other ideas.

Hooking into network errors

This is already covered by another issue: #296

With this one I'm not sure if it's worth creating a new hook or expanding the beforeError hook to also run on network errors. Modifying beforeError behaviour would probably require a major version bump as it could be a breaking change but it does feel like the more sensible API.

otherwise I could add a new beforeNetworkError hook

type BeforeNetworkErrorState = {
    request: Request;
    options: NormalizedOptions;
    response: Response
    error: unknown; // probably want to force users to perform their own type narrowing
};

type BeforeNetworkErrorHook = (options: BeforeNetworkErrorState) => Error | Promise<Error> | void ;

Again, please let me know if either of those sound reasonable or if you have any other ideas.

Thanks!

[sholladay]

Let's start by adding { request, response, options } as the second argument to parseJson. My only concern with that is users may already be passing in a JSON.stringify-like function with multiple arguments that are incompatible with this new signature (i.e. the Array.map + parseInt footgun). It would be easy to update by wrapping the parser in an arrow function, though. We'll just need to mention this in the release notes.

Regarding network errors, indeed a lot of people have requested similar functionality. A major version bump is no problem, one is likely coming soon anyway because of PR #602.

However, we are not yet in a great place to provide this feature for a few reasons:

1. fetch() doesn't throw discrete error types for network errors vs other errors. In fact, there's nothing at all to even indicate an error came from fetch, other than some hardcoded assumptions about error messages. The error messages and properties also vary between environments. We can and should use is-network-error to make a best-effort attempt at this, but just be aware I doubt every edge case is handled correctly or consistently. For example, is-network-error#4 (comment) indicates that, in Chrome but not Safari, an aborted request looks like a network error and therefor is treated as such by is-network-error. This is really a spec-level problem and there hasn't been much movement from WHATWG to solve it.
2. The way our error handling is structured right now isn't very conducive to this. It's probably doable anyway, but getting #149 (comment) done first would make it easier (and is itself a breaking change).
3. Presumably, if you handle network errors via a hook, then you don't want ky() to throw them. That's all fine and good except then there is no Response or anything for us to return and you run into the same problem Sindre mentioned in #508 (comment). Your code will end up erroring on its own because it expects success if we don't throw. The only sensible solution I can think of is #508 (comment), but that's a big change, and I suspect it would be painful for TypeScript users.

All that being said, I will happily review any PR you send. Just don't be surprised if we decide not to move forward with it. 🙂

[danbahrami]

@sholladay thanks for the super fast response 🙇 Couple of thoughts on your points:

1. fetch() doesn't throw discrete error types for network errors vs other errors. In fact, there's nothing at all to even indicate an error came from fetch, other than some hardcoded assumptions about error messages. The error messages and properties also vary between environments. We can and should use is-network-error to make a best-effort attempt at this, but just be aware I doubt every edge case is handled correctly or consistently. For example, TypeError cancelled is-network-error#4 (comment) indicates that, in Chrome but not Safari, an aborted request looks like a network error and therefor is treated as such by is-network-error. This is really a More informative error types whatwg/fetch#526 and there hasn't been much movement from WHATWG to solve it.

Based on this it feels like trying to read into what went wrong when fetch() throws (e.g. is this a network error?) is outside the responsibility of ky. What makes this a great library, as opposed to something like axios, is that it's a relatively thin wrapper around fetch(). If you were handling this error with fetch() it might look something like...

try {
    const response = await fetch(url);
} catch (e) { // e is typed to any or unknown based on TS config
    if (e instanceof TypeError) {
        // probably a network error
    } else if (e instanceof Error && e.name === "AbortError") {
        // client aborted
    } else if (e instanceof Error) {
        // some random unknown error
    } else {
        // ok something really strange has happened
    }
}

I think it's fair to expect consumers to perform similar error handling with ky. A more honest name might be onFetchError rather than onNetworkError.

type BeforeNetworkErrorState = {
    request: Request;
    options: NormalizedOptions;
    error: unknown; // type as unknown so consumers can do their own type narrowing
};

const onFetchError: OnFetchErrorHook = ({ error, request, response, options }) => {
    if (error instanceof TypeError) {
        // probably a network error
    } else if (error instanceof Error && error.name === "AbortError") {
        // client aborted
    } else if (error instanceof Error) {
        // some random unknown error
    } else {
        // ok something really strange has happened
    }
}

Consumers are able to use is-network-error if they choose. Or at a later point we could do it internally and expose a NetworkError type like we do for HTTPError which consumers could use for type-narrowing.

3. Presumably, if you handle network errors via a hook, then you don't want ky() to throw them. That's all fine and good except then there is no Response or anything for us to return and you run into the same problem Sindre mentioned in Pass TimeoutErrors to beforeError hooks #508 (comment). Your code will end up erroring on its own because it expects success if we don't throw. The only sensible solution I can think of is Pass TimeoutErrors to beforeError hooks #508 (comment), but that's a big change, and I suspect it would be painful for TypeScript users.

I agree that it's not a good idea to allow handling of fetch errors because it would cause problems upstream. Perhaps having a naming convention of the hooks could set expectations about that:

• beforeFoo runs before an event and allows the input to be modified
• afterFoo runs after an event and allows the output to be modified
• onFoo runs after an event but is only to observe, doesn't modify anything

So in this case onFetchError would not allow you to handle errors, only observe.

[sholladay]

You're welcome!

Based on this it feels like trying to read into what went wrong when fetch() throws (e.g. is this a network error?) is outside the responsibility of ky

There's definitely a line to be drawn somewhere there. I think its perfectly reasonable to expect Ky to wallpaper over some of fetch's deficiencies and make the general task of error handling easier. But expecting Ky to solve the inconsistent handling of AbortErrors might be a bit too far.

we could do it internally and expose a NetworkError type like we do for HTTPError

That's what I want to do. We'll expose the fetch error as the NetworkError's cause, which will enable users who need something narrower than our definition of a network error to at least make an attempt at distinguishing them.

So in this case onFetchError would not allow you to handle errors, only observe.

That sounds pretty reasonable. We could even allow users to return a modified error if they want to, which will then be thrown after the hook. But then, does this behavior actually solve the "centralized logging" use case cleanly? I mean, surely any decent logging system is going to be monitoring errors thrown by Ky. If it's also logging inside of a hook, then you will likely end up with duplicate error logs. It seems kind of self-defeating to me. But I still think it could be useful for enriching errors with metadata.

[danbahrami]

surely any decent logging system is going to be monitoring errors thrown by Ky. If it's also logging inside of a hook, then you will likely end up with duplicate error logs.

In my case I'd like to use a shared api client across multiple different apps. Sometimes directly making fetch requests, sometimes inside React Query where errors are caught automatically. I'd like to have one consistent way of logging API errors everywhere. To me it makes sense to do it centrally otherwise it'll be hard to maintain consistent logging everywhere.

This discussion is making me wonder if perhaps the problem of logging is different to what ky's hooks are for. Do you think there would be appetite to add something completely new to the config like a logger function?

It could take a stream of events that are typed as a discriminated union so you could add different events over time without breaking changes.

type LogEvent = 
    { type: "RequestStart"; request: Request; options: Options }
    | { type: "ResponseSuccess"; request: Request; response: Response; options: Options }
    | { type: "ResponseError"; request: Request; error: Error; options: Options }
    | { type: "JsonParseError"; request: Request; error: Error; options: Options };

type Logger = (event: LogEvent) => void;

[sholladay]

I like that API. It might have too much overlap with hooks for both APIs to exist, but I'll give it some thought.

For now, let's create the NetworkError and ParseError classes and route them through the onError hook.

[kadhirr]

@sholladay are you folks looking for someone to pick up this enhancement? I'd like to help out here if it's possible!