Issue with Retries after v0.16.0

[zjr]

This could very well be something I'm doing wrong or misunderstanding and if so I apologize.

Starting at v0.16.0 and continuing to v.0.16.1, there seems to be an issue with retries reusing a Request object (at least that's what the error tells me). Ky's fetch promise rejects with this:

TypeError: Failed to execute 'fetch' on 'Window': Cannot construct a Request with a Request object that has already been used.
    at Ky._fetch (index.js:395)
    at fn (index.js:257)
    at async Ky._retry (index.js:346)
    at async Promise.Ky.result.<computed> [as json] (index.js:299)

In v0.15.0 & below I would just get the error response from my API.

[sholladay]

Our retry tests are passing, although unfortunately I just noticed that many of the assertions are not checking error messages, which is really bad because it may be giving us false confidence.

Could you please provide a reproducible example (or even better, a failing test) so we can verify any potential fix?

I think the solution will probably be to replace this.request with this.request.clone(), when we pass it to fetch.

[zjr]

Might be a couple weeks but if I can manage to find the time sooner I will.

[sholladay]

Hi @zjr do you think you'll be able to look at this soon? I'd like to fix this in the next patch release along with another v0.16 regression.

[elijahsmith]

we are hitting this same issue.

I've just spent several hours trying to reproduce the issue in a test, but had no success. I did open a small PR to improve the retry tests, by verifying the status text associated with the rejected promises: #229

What I am seeing when hitting this bug in our application is, the first retry returns the correct error -- HTTPError: Bad Request in the beforeRetry hook's errors param -- but then subsequent retries return TypeError: Failed to execute 'fetch' on 'Window': Cannot construct a Request with a Request object that has already been used., just as @zjr describes above.

But for the life of me, I can't reproduce it in a test. I've tried various permutations of some hooks we have in place; I'm replicating our use case exactly (POST, with an endpoint generating an [intentional] 400 status code); I've tried accessing the error in the .catch, etc etc. I'd be happy to do more work on reproducing this in tests but I'm at a bit of a loss... any ideas?

[sholladay]

@elijahsmith thanks for digging into this. I imagine you have been working with the tests in test/retry.js, where everything is run in Node. My recommendation at this point is to write a Puppeteer test instead, such as the tests in test/browser.js, where Ky is run in an actual browser (headless Chromium). That may expose the problem to the tests, as the lack of an error may very well have something to do with the Request implementation from node-fetch, which our Node tests use (imported in test/_require.js).

In fact, I think a lot of our tests should be updated to use Puppeteer, in order to catch potential issues that are only reproducible in a browser. That would slow down and complicate the tests a bit, but I think the tradeoff is worth it, given that we target browsers and this wouldn't be the first time the Node environment behaved differently.

To compensate somewhat for the slower tests, we could consider switching to the puppeteer-core package, which avoids downloading Chromium at npm install time in favor of using an existing Chromium / Chrome installation on the machine. I suppose that is feasible given how ubiquitous Chrome is, but still, some developers may not have it. It is also subject to certain limitations (e.g. no support for config via env vars).