Fix redirect timeout

[gdoron]

Current implementation resets the timeout on every redirect.
It can cause severe issues when the user is willing to wait only for x amount of seconds.
But because the remote server redirects him, he can wait a lot more (maybe even indefinitely)

@bitinn makes sense?

[codecov-io]

Codecov Report

Merging #685 into master will decrease coverage by 0.68%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master     #685      +/-   ##
==========================================
- Coverage     100%   99.31%   -0.69%     
==========================================
  Files           7        7              
  Lines         579      586       +7     
  Branches      185      187       +2     
==========================================
+ Hits          579      582       +3     
- Misses          0        3       +3     
- Partials        0        1       +1

Impacted Files	Coverage Δ
src/index.js	96.66% <50%> (-3.34%)	⬇️

---

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 1e99050...40a280a. Read the comment docs.

[Richienb]

Please unit test this.

[gdoron]

@Richienb I'm quite new to codecov so sorry for the noob question.
Is there a way of knowing which lines are not covered because of this new pull request?

[Richienb]

@gdoron https://codecov.io/gh/bitinn/node-fetch/pull/685/src/src/index.js

[gdoron]

@Richienb I can't seem to find a way to make the test spend on the first request in the chain exactly the time given in the timeout property.
I can remove lines 162-166 and then there won't be any test-coverage issue, but that's wrong.
Any suggestions?

[Richienb]

@gdoron Since we can't match the exact time specified as the timeout, we should not be exact.

[gdoron]

@Richienb I can remove these lines, and all will keep working.
These lines just save sending another request if the timeout just expired.
Since we already know the next request won't make in 0 ms.

Do you want me to remove this edge-case?

[Richienb]

I'll let another reviewer chime in here.

[gdoron]

Actually removing handling this edge case will cause removing the timeout completely from the next request in case the remaining time is 0, since 0 is a "falsy value".

[bitinn]

Thx for the PR, but as explain in #615, timeout was an extension to Fetch spec, we are not planning to change this behavior as many are depending on this behavior. And we plan to drop timeout eventually. Use AbortSignal instead.

#615 (comment)

[xxczaki]

@bitinn Are we going to drop timeout support in 3.x or not yet? We can always add a warning message when user is using timeout to let them know, that it will be deprecated soon.

[Richienb]

@xxczaki Timeout already has a deprecation path in the 3.x roadmap and is outlined in #523.

[xxczaki]

@Richienb Ok, as it was striked out in the roadmap, I'm going to add it again 😄

[gdoron]

Thanks for the answer.
I just find it very hard to believe anyone using the library understood the timeout behaves as it does now.
Actually it took me significant amount of time understanding what's happening in my application, how the average time I see in DataDog exceeds significantly the timeout value I used.

And anyway, if someone wants the current implementation, Abort Signal won't allow him maintain his current flow once you drop support of timeout.

I would reconsider, but it's up to you.
Thanks for your time!
@bitinn

[gdoron]

@bitinn did you see my last comment?
Do you want me to close the PR?

[Richienb]

@bitinn @xxczaki Since we're going to deprecate timeout in favour of AbortController in v3, we can probably include these changes in the final update for v2.

[bitinn]

@gdoron Sorry, I will have to close this PR, the timeout + follow redirect scenario is setup for web crawler type use cases: You want to follow redirect, but just can't be sure how long each redirect would take. If you want to limit the timeout in v2, use timeout + don't follow redirect or timeout + limit total redirect to N (so maximum timeout is timeout x N), or use AbortController which is the spec compliant way.