Support timeout option in http.request

jhiesey / stream-http

Streaming node http in the browser

MIT License

354 stars 62 forks source link

Support timeout option in http.request #55

Closed connor4312 closed 7 years ago

connor4312 commented 8 years ago

As seen in Node 6.8.0, http.request now supports a timeout option. This PR adds support for that in stream-http! Unfortunately we have to force XHR since the Fetch API does not support timeouts (yet).

connor4312 commented 8 years ago

Noticed you don't have saucelabs running on PR, so I ran them manually. They came up green except for Edge 14 which got stuck on the text streaming tests. https://peet.io/i/stream-http-test-output.txt

connor4312 commented 8 years ago

Just fyi I'm using this updated fork in the influx browser tests and they pass. It works it real life 😄

jhiesey commented 8 years ago

In addition to supporting this, we should fix the setTimeout method as well.

connor4312 commented 8 years ago

The issue there is that setTimeout is called after the request method is fired. At that point we may have already called fetch, which we can't timeout. The only ways to support that would be to either:

defer the request initialization by a tick to see if setTimeout is called before running the request, and throw an error if it's called asynchronously for a fetch request. (Failing silently could lead to some nasty bugs.)
only use XMLHttpRequest and not fetch. Which, personally, would make sense to me; fetch is newer and shinier, but XMLHttpRequest is better supported, has more features, and isn't going anywhere anytime soon. Removing fetch would also make the module simpler and smaller, always a win.

While I'd be happy to help with implementation, I think both options for implementation are outside the scope of this PR.

connor4312 commented 7 years ago

Hey @jhiesey I wanted to follow up to see if you had any more thoughts on this or if it's good to merge! 😄

jhiesey commented 7 years ago

What's wrong with calling request.setTimeout after the fetch starts? It should be fine to just start the timer a little late, and then cancel the timer when the fetch completes. If the timer doesn't get cancelled, it will fire and call the callback.

I'll merge this PR for now and then figure out what to do. I'll make sure a new version gets released in the next few days whether or not I add setTimeout as well.