Failures from cache PR

[sindresorhus]

Some issues after merging the cache PR:

• 2bc2b90
• https://travis-ci.org/sindresorhus/got/jobs/288149045

// @lukechilds

[lukechilds]

Oh no :/

Strange that the tests weren't failing on Travis in the PR builds 😕

I finally got myself some paid work 🎉 but it's on a really tight deadline and I've lost a few days from travel (I'm back in Thailand). I need to catch up on some work but I'll get this fixed ASAP. Probably either tonight or tomorrow night.

[lukechilds]

Regarding 2bc2b90, it seems to me this was originally a bug in Got. The fact that http.request could throw was never accounted for so in Promise mode the thrown http error caused the Promise to reject. Before the cache PR, in stream mode that error would never be caught, so definitely a bug IMO.

So this bug has now just moved to cacheable-request. It also existed in request: request/request#2120

We should definitely fix this so thrown requests (as opposed to errored) can be handled properly, however I don't think implementing the previous behaviour and returning the raw http error is a good idea. This isn't documented anywhere in Got and appears to just be side affect from previous code. I would suggest either throwing this as a got.RequestError or creating a new error class to differentiate between requests that error and requests that immediately throw.

Thoughts?

Edit: Potential solution in jaredwray/cacheable#14 that would align neatly with Got's functionality.

[sindresorhus]

The fact that http.request could throw was never accounted for so in Promise mode the thrown http error caused the Promise to reject.

How is the Promise rejecting incorrect? It is currently accounted for in Promise mode, as 92ed73a proves. No idea about Stream mode though.

[sindresorhus]

Potential solution in jaredwray/cacheable#14 that would align neatly with Got's functionality.

👍

[lukechilds]

How is the Promise rejecting incorrect? It is currently accounted for in Promise mode, as 92ed73a proves. No idea about Stream mode though.

Ahh, my bad. If it's intentional then IMO it would be nicer if it could be returned as one of the proper Got error types, like got.RequestError.

I'll make that proposed change to cacheable-request and then we can throw a got.RequestError if http.request() throws.

[lukechilds]

First bullet point in your original comment should be resolved in #412.

Actively looking into the failing cancel test. It's pretty difficult to pin down though, it never fails for me locally and only occasionally fails on Travis.