Abort request on timeout

[roblav96]

Currently, if timeout is defined in request options, it's ignored and the request does not abort.

Shamefully adopted from:
feross/simple-get/index.js#L65
@feross 🤓

[codecov-io]

Codecov Report

Merging #17 into master will decrease coverage by 4.34%.
The diff coverage is 33.33%.

@@            Coverage Diff             @@
##           master      #17      +/-   ##
==========================================
- Coverage     100%   95.65%   -4.35%     
==========================================
  Files           1        1              
  Lines          43       46       +3     
==========================================
+ Hits           43       44       +1     
- Misses          0        2       +2

Impacted Files	Coverage Δ
src/index.js	95.65% <33.33%> (-4.35%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8d8f45e...fc34d3f. Read the comment docs.

[lukeed]

Can you look at / try this? a2158e6

It's been hanging out in a branch because I haven't had time to sufficiently test it yet and figure out my thoughts. The way it's set up is that it will honor the original timeout on redirects. Otherwise, a redirect from HTTP to HTTPS on a broken link won't abort after 3s, for example.

I wanted to do it this way because this is how the browser version operates. It's (still) coming so they should be consistent IMO

[roblav96]

I haven't used this lib on the browser so I'm not sure of its behavior in that environment. From the looks of a request timings lib like szmarczak/http-timer it seems node.js has a particular way of timing things. I've been working on a project all day with this PR's features and there certainly seems to be quite a large delay from the initial request to raising the timeout event to abort. I'm not sure at which stage of the request node.js decides to start the timeout countdown. 😅

To be honest though, we might be OT'ing (over thinking) this one. lol Your repos are the first place I look for simple micro-libs that don't over complicate it's basic functionality. I'd say we veto this quirk to keep the lib easy as pie.

🥧

[paulocoghi]

Your repos are the first place I look for simple micro-libs that don't over complicate it's basic functionality.

+1 million! 😄

I'm so inspired on Lukeed's way of work that I released my first micro lib Reflect, that replaces rsync-wrapper and doesn't depend on rsync, syncing directories in pure js with no dependencies.
(marketing sound ends now xD)

@lukeed, I really appreciate if you can take a look and give your feedback and opinion. :)

My next task is to release a micro-library to replace the ultra-heavy browser-sync.

(sorry to pollute the PR and sorry to make the readme completely inspired on yours 😄)

[lukeed]

@roblav96 Perhaps 😅 I was just worried about raising issue/concern with the browser version behaving differently than the server implementation. If you (both) don't think it's a big issue, then maybe I'll skip the extra legwork, albeit not much.

@paulocoghi Looks cool! I don't have any uses for it, but seems like a clever concept. My only advice/concern is that you/your users may(?) have problems with the native Reflect global. I don't mind at all that you the readme is so similar. Obviously I like the format 😉

Thank you both for the kind words 🙇

[paulocoghi]

If you (both) don't think it's a big issue, then maybe I'll skip the extra legwork, albeit not much.

If the necessary extra work isn't a deterrent, I vote for the consistency.