Use the provided timeout option, if set, for the InfoReceiver. ⏰ #594
Conversation
| var self = this | ||
| , url = urlUtils.addPath(baseUrl, '/info') | ||
| ; | ||
| debug('doXhr', url); | ||
|
|
||
| this.xo = InfoReceiver._getReceiver(baseUrl, url, urlInfo); | ||
|
|
||
| if (timeout === undefined) { | ||
| timeout = InfoReceiver.timeout | ||
| } |
There was a problem hiding this comment.
I think I saw in the documentation that SockJS will use the bigger timeout of either the "internally calculated" timeout, or the timeout option. Do you want this to apply here, too? Something like
if (timeout < InfoReceiver.timeout) {
timeout = InfoReceiver.timeout
}
There was a problem hiding this comment.
It's not really "calculated," though. 😉
There was a problem hiding this comment.
timeout (number)
Specify a minimum timeout in milliseconds to use for the transport connections. By default this is dynamically calculated based on the measured RTT and the number of expected round trips. This setting will establish a minimum, but if the calculated timeout is higher, that will be used.
|
@brycekahle - You look like the person running the repo, if you would be so kind as to review this. |
It is only a bit awkward. 😳
…on in the default case. 🥸
| describe('info', function () { | ||
| it('will timeout - default timeout', function (done) { | ||
| this.timeout(10000); | ||
| InfoReceiver.prototype._getReceiver = function(baseUrl, url) { | ||
| return new InfoAjax(url, XhrFake); | ||
| }; | ||
|
|
||
| var expectedWasClean = true; | ||
| InfoReceiver.prototype._cleanup = function(wasClean) { | ||
| expect(wasClean).to.equal(expectedWasClean); | ||
| if (expectedWasClean === false) { | ||
| // Cleanup was called because of a timeout | ||
| done(); | ||
| } | ||
| expectedWasClean = false; | ||
| }; | ||
|
|
||
| new InfoReceiver('test', {}); | ||
| }); | ||
|
|
||
| it('will timeout - configured timeout', function (done) { | ||
| this.timeout(2000); | ||
| InfoReceiver.prototype._getReceiver = function(baseUrl, url) { | ||
| return new InfoAjax(url, XhrFake); | ||
| }; | ||
|
|
||
| var expectedWasClean = true; | ||
| InfoReceiver.prototype._cleanup = function(wasClean) { | ||
| expect(wasClean).to.equal(expectedWasClean); | ||
| if (expectedWasClean === false) { | ||
| // Cleanup was called because of a timeout | ||
| done(); | ||
| } | ||
| expectedWasClean = false; | ||
| }; | ||
|
|
||
| new InfoReceiver('test', {}, 1000); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
I apologize for these awkward tests - I do not fully understand all of the inheritance going on, so I overrode what I needed to test the override. The result is that the finish event is still emitted, and that is why I have the wasClean expectation change.
We have some users with truly terrible internet connections. From time to time, the InfoReceiver times out for them. We'd like to be able to configure a longer timeout for the InfoReceiver, instead of the hardcoded 8000ms.
This PR accomplishes this. However, I haven't used javascript for years, and am a little uncertain if there is a simple and reliable way to unit test the addition of this configurable timeout. I'd be happy to figure it out if given some guidance.
I'm also unsure if I should build dist/sockjs for the PR and if so, how to do it.