search

jhiesey / stream-http

Streaming node http in the browser
MIT License
354 stars 62 forks source link

'timeout' option differs from Node.js #66

Closed feross closed 6 years ago

feross commented 7 years ago

@connor4312's PR https://github.com/jhiesey/stream-http/pull/55 added a 'timeout' option, but it's behavior differs from the Node.js behavior.

In Node.js the connection is not closed when the timeout occurs, only a 'timeout' event is fired. However, setting the xhr.timeout property actually does kill the connection.

xhr.timeout: https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest/timeout request.setTimeout: https://nodejs.org/api/http.html#http_request_settimeout_timeout_callback

connor4312 commented 7 years ago

It's fair to adjust stream-http to match Node's behaviour, but I'm curious whether as to a use case where a client would set the timeout but not want the connection to killed after the timeout occurs. In my own code, when I've used raw http, I ended up having to attach extra logic to make sure callbacks are only fired one time, etc.

feross commented 7 years ago

Most users probably want the connection killed after a timeout occurs. No idea why the Node.js API doesn't do that. Still, the point of this package is to match the Node.js API and any divergence will just introduce confusion.

jhiesey commented 7 years ago

The timeout browser test also fails on some browsers. The timeout implementation needs quite a bit of work IMHO.

jhiesey commented 6 years ago

Version 2.8.0 has clarified documentation on this.

The node behavior of timeouts deals with time between bytes on the socket, which the browser doesn't have access to, so it can't be implemented here in any form resembling the original.

The timeout option added in #55 is indeed doing something rather different, but it shouldn't conflict since its interface (passing in an option) is different. I think the real bug here is that there is an implication that these two features are related, which they aren't.

Feel free to reopen if you disagree.

jhiesey commented 6 years ago

Actually, it looks like I misread the node docs. Working on a solution.

jhiesey commented 6 years ago

Since it isn't really possible to match node's behavior properly in the browser, I'm just going to rename the broken timeout option to requestTimeout.