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.
There was a problem hiding this comment.
Pull Request Overview
This PR enables configurable timeout for the InfoReceiver to support users with poor internet connections, replacing the hardcoded 8000ms timeout with an optional user-provided value.
- Added timeout parameter to InfoReceiver constructor and doXhr method
- Modified SockJS main class to pass options.timeout to InfoReceiver
- Added unit tests to verify timeout functionality with both default and custom timeout values
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/info-receiver.js | Added timeout parameter support with fallback to default timeout |
| lib/main.js | Modified to pass options.timeout to InfoReceiver constructor |
| tests/lib/receivers.js | Added test cases for default and configured timeout scenarios |
| @@ -121,7 +121,7 @@ function SockJS(url, protocols, options) { | |||
| , sameScheme: urlUtils.isSchemeEqual(this.url, loc.href) | |||
| }; | |||
|
|
|||
| this._ir = new InfoReceiver(this.url, this._urlInfo); | |||
| this._ir = new InfoReceiver(this.url, this._urlInfo, options.timeout); | |||
There was a problem hiding this comment.
Potential runtime error if 'options' is undefined or null. The code should safely access options.timeout with optional chaining or a fallback check.
Copilot uses AI. Check for mistakes.
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.