Use request to normalize options

[sholladay]

Closes #166
Closes #139

This PR:

• Aims to simplify Ky and solve some edge case issues and confusing behavior related to how our options are normalized and merged.
• Makes it so that internally, and within hooks, a Request instance is now used as the source of truth instead of input. This replaces some of the bespoke logic that we've had for processing options. It also makes life easier for people using the hooks option, because they will receive a normalized request, whose behavior is more obvious and is well-specified by web standards. We can discuss whether the raw input should be available as well.
• Cleans up the separation betwen Ky options and fetch options internally, so there's a clear distinction and it's easy to pass around one or the other, or both.
• Adds the ability for beforeRequest hooks to return a Request instance to replace the existing request.

[sholladay]

Maybe I just need some sleep and am missing something obvious, but it's not clear to me why that timeout test is failing. I did not intentionally change any timeout or retry behavior.

Any thoughts @szmarczak?

[sholladay]

I tested Chrome, Firefox, Safari, and Edge 18, and all of them default to same-origin now. Not sure about mobile browsers, though. Should we remove this?

[sindresorhus]

Nah, not worth the risk. It's only a few bytes anyway.

[sholladay]

I kinda want to change the signature of hooks.

Prior to this PR, we had:

• beforeRequest(input, options)
• beforeRetry(input, options, error, retryCount)
• afterResponse(input, options, response)

This PR changes input to request in these signatures, anyway. So it is worth considering any other breaking changes we want to do. For example...

• We could attach the response to the request passed to afterResponse, rather than having it as a separate argument. I find this more elegant since the response belongs to a particular request. Plus it improves the symmetry of the call signature between beforeRequest and afterResponse.
• We could attach retryCount to the request or error passed to beforeRetry, rather than passing it as a separate argument. I don't have strong feelings about this one, but I do feel that any function with four arguments is a bit much.

[sholladay]

Ready for review. Could use help on that one failing test.

// @sindresorhus @szmarczak

[sholladay]

For now, I've worked around the timeout test issue by changing it to validate synchronously.

I would argue that:

1. This is how input validation should work, anyway.
2. Even if you disagree with 1, it still makes our validation more consistent.

[sindresorhus]

Promise-returning methods should always reject: https://twitter.com/jaffathecake/status/638659818996269056?s=21

[sholladay]

Alright, I made it a rejection. It's still validated in the constructor to work around the broken test that failed for reasons I don't understand.

[sholladay]

It was a real pain to make it reject asynchronously before the request, because the rejected promise didn't have the Body methods, so .json() didn't exist and would immediately throw an error, for example. I had to place the rejection in a very specific, non-obvious place.

I wholeheartedly disagree with this design pattern of rejecting user mistakes asynchronously. But I aim to please, so I made it work. The tests are passing. Please review.

At this point, the main thing I'm wondering is what options the hooks should receive. I'm thinking either:
A. Only fetch options (current behavior of this PR)
B. All Ky and fetch options mixed together

The benefit of A is that we can more easily support users directly mutating the options, without forcing them to return a Request. It also avoids the illusion that you can override Ky's settings within hooks and prevents the temptation to try to do so.

As for B, I'm not sure if there is a concrete use case for reading Ky's options within hooks, but I can sort of imagine it being useful to do something special when throwHttpErrors is false or when timeout has a large value, etc. The only downside is that to support hooks mutating the object, we'd have to go back to a single combined options object (i.e. not separate fetch options vs Ky options), which I'd be fine with, albeit it feels a little less clean.

[sholladay]

Anything else needed here?

In addition to the issues that this PR already addresses, I also have fixes for #167, #182, and #187, which I want to implement on top of these changes. I could easily include those in this PR (most of them actually remove lines of code), however I don't want to do too many unrelated things and make it harder to review. If we can merge this soon, I will get to those quickly.

It would be preferable to hold off on a release (even though everything here is working as-is). I realized that hooks really should be receiving all options for both Ky and fetch mixed together, so that the hooks themselves can call Ky and "inherit" the Ky options. I'd like to address that at the same time as #167, which as mentioned previously, should probably be done in a separate PR.

[sindresorhus]

This is great. Nice work 👌 And sorry for the delay. I reviewed this a week ago and was going to merge and then got distracted and forgot about it...

[sholladay]

@sindresorhus is codecov broken? I just noticed that the status check for this PR says "No report found to compare against" and clicking on the details also doesn't bring up anything useful. Maybe it needs to be re-authorized?

[sindresorhus]

Ah yeah. It was not enabled on the GitHub side. Done now.