Hook beforeError does not respect the throwHttpErrors setting

[Supinic]

Describe the bug

• Node.js version: 18.0.0
• OS & version: Windows 10, Raspbian Buster, Ubuntu 20.04
• Got version: 11.6.0 and higher

Actual behavior

The beforeError hook fires regardless of whether or not the throwHttpErrors option is set to false and the request fails. This has been (erroneously, it seems?) changed in #1384 - more specifically in as-promise/index.ts. This change wasn't mentioned anywhere in the 11.6.0 release, or in other releases since.

The behaviour is correct in all versions up until 11.6.0, where this PR was merged.

Expected behavior

The beforeError hook should not fire if the throwHttpErrors option is set to false, and the request fails.

Code to reproduce

const got = require("got");
const opts = {
    throwHttpErrors: false,
    hooks: {
        beforeError: [
            (e) => {
                console.log("beforeError hook", e.name);
            }
        ]
    }
};

(async () => {
    const res = await got.get("https://httpstat.us/400", opts);
    console.log("Response", res.body);
})();

In versions prior to 11.6.0, the request response will be logged into console. In versions since, the beforeError hook will take precedence, log the error from it, and the response will not be logged.

As an alternative to a code-based fix, the documentation could be adjusted instead, as it currently contains no reference to such behaviour.

Checklist

• I have read the documentation.
• I have tried my code with the latest version of Node.js and Got.

[szmarczak]

This unfortunately won't be easy to fix. In order to make a retry, an error is required. Then the beforeRetry hooks are called. If the error is still there (e.g. still getting 500) then beforeError is called. The error is then passed to the Promise implementation, it checks if options.throwHttpErrors is false. If it is, then returns error.response instead.

It wasn't possible to retry streams until #1384 - to make the same logic there we would need to replace

got/source/core/index.ts

Lines 796 to 799 in 0947389

	if (options.isStream && options.throwHttpErrors && !isResponseOk(typedResponse)) {
	this._beforeError(new HTTPError(typedResponse));
	return;
	}

with

		if (options.isStream && !isResponseOk(typedResponse)) {
			if (options.throwHttpErrors || (this.listenerCount('retry') > 0 && thisIsNotTheLastRetry)) {
				this._beforeError(new HTTPError(typedResponse));
				return;
			}
		}

but since calculateDelay hasn't been called yet, then thisIsNotTheLastRetry is unknown. And calculateDelay needs an error.

[szmarczak]

Also HTTPErrors always have error.response.body defined. So it's not possible to retry HTTPErrors for Stream API. On the last retry we'd need to check if options.throwHttpErrors is false. If false, then don't throw. But the body has been already read... so we have a few solutions:

1. get rid of error.response.body (severely degrades DX so it's rather a no), or
2. accept this behavior, or
3. do not retry HTTPErrors if options.throwHttpErrors is false (this would be best design-wise, this is the behavior <=v8.3.2), or
4. apply a workaround: don't call beforeError hooks (for HTTPErrors) if options.throwHttpErrors is false (compatible with v9.6.0 behavior).

[szmarczak]

We need to document this retry difference.

[szmarczak]

#2104 is a potential fix.

[szmarczak]

/cc @sindresorhus