throwHttpErrors: false suppresses ky.TimeoutError

[StephenCarboni]

I could have sworn that this did not use to be the case, but it's not useful to not throw on non-200 codes and lose TimeoutErrors, which have nothing to do with HTTP errors.

<script type="module">
    import ky from 'https://cdn.jsdelivr.net/npm/ky@latest/index.js';

    (async () => {

        const r = await ky('https://example.com', {timeout: 1, throwHttpErrors: false});

        console.log(r)

    })();
</script>

The response above is undefined. Which leaves me unable to tell why it's undefined.

How else are we expected to determine if a response timed out but we want to handle HTTP errors manually?

[sholladay]

Indeed, you are correct that throwHttpErrors affects timeout errors in addition to HTTP errors.

const response = await ky('https://httpstat.us/200?sleep=1000', {
    timeout         : 1,
    throwHttpErrors : false
});
console.log(response);  // undefined

I don't like this behavior, either, at least with the current option name. An undefined response is not exactly intuitive or useful. Most likely, it would be better to have people use try / catch and filter out ky.TimeoutErrror manually, if they want to ignore such errors for some reason.

I could have sworn that this did not use to be the case ...

Based on my reading of the code, it looks like this behavior has always existed since the introduction of PR #14, where throwHttpErrors was implemented.

Some possible ways forward:

1. Keep existing behavior, but rename the option from throwHttpErrors to throwOperationalErrors (or something similar)
2. Remove timeout errors from the scope of throwHttpErrors, and either:
   A. Add a separate throwTimeoutErrors option
   B. Do not add a separate option for timeout errors (always throw them)

[StephenCarboni]

0.8.0 seems to be the earliest release that throws the timeout error with throwHttpErrors: false

But it doesn't throw http errors, either. It could be that what I remember was using a release between 0.9.0 and 0.3.0 🤷‍♂

<script type="module">

    import ky from 'https://cdn.jsdelivr.net/npm/ky@0.8.0/index.js';
    // import ky from 'https://cdn.jsdelivr.net/npm/ky@latest/index.js';

    (async () => {

        try {
            const r = await ky('https://httpstat.us/401?sleep=1000', {timeout: 1, throwHttpErrors: false});
        } catch (e) {
            console.log(e.name);
        }

        // nothing thrown for this
        await ky('https://httpstat.us/401', {throwHttpErrors: true});

    })();

</script>

[sindresorhus]

I would prefer 2B.

[sholladay]

A failing test for this would be much appreciated, as I am planning to work on it in the next few days.

[sholladay]

@StephenCarboni I have opened PR #334 to fix this bug. It would be great if you could try my branch and confirm that it works for you.

[StephenCarboni]

@StephenCarboni I have opened PR #334 to fix this bug. It would be great if you could try my branch and confirm that it works for you.

Looks okay. I'm getting a TimeoutError with example in OP.