Use original request for fetch and store cloned request for retries

[veltman]

Related to #231, it seems like ky clones the request before every fetch so that a retry with a POST body works. But this continues to prevent using ky in cases where a POST gets proxied by something like Puppeteer or Playwright because of the underlying bug in the DevTools Protocol (which might never get fixed).

If I'm understanding the status quo correctly, would there be any downside to only cloning the request on the retry attempts and not on the initial fetch? It seems like that could at least solve the problem for requests that succeed without a retry. If that seems reasonable to you I could work up a PR for it...

[sholladay]

It's been a while since I took a hard look at the request cloning mechanics, but I don't believe this will work.

Any time we pass a Request instance to fetch(), it consumes the request body, meaning that the request cannot be reused because the request body can never be read more than once. Thus, we have to preemptively clone the request and pass in the cloned copy to fetch() if we ever want to be able to reuse the original request instance in the future (as would happen in the case of a retry).

So, if the first fetch() gets a request that hasn't been cloned, it will consume the body and prevent any retries.

I'm open to suggestions, but don't currently see a way to make this workable. request.clone() is a real PITA.

[veltman]

But am I correct that you could preemptively clone the request for the future usage, and then consume the uncloned request? That way on the first fetch attempt, you'd be using the uncloned request but still be able to use the clone in the event of a retry.

i.e. in pseudocode:

async function fetchAttempt(request) {
  const clone = request.clone();
  const response = await fetch(request);
  if (shouldRetry(response)) {
    return fetchAttempt(clone);
  } else {
    return response;
  }
}

[sholladay]

Ah, yes, I quite like that and it should work. At the time of #231, Deno had an implementation bug where calling clone() would consume the original request's body, so we had to pass in the clone and not the original, plus it simplified the code a bit. But that bug has been fixed since then. I also just checked node-fetch and even its clone() implementation seems to behave properly in this respect.

If you'd like to open a PR, that would help a lot.

[ulken]

Given this, is this something you still want to pursue? I'm hit by the same problem and would be willing to have a stab at it. But I'd like you to sign off on it first. If not, should this be closed?

[sholladay]

This is still a valid improvement. Cloning requests is error prone, see e.g. #209. It would be nice to use the original request for the initial fetch attempt to avoid any cloning bugs where cloning isn't needed..