Implement disabling timeouts #127
Conversation
|
|
||
| @default 10000 | ||
| */ | ||
| timeout?: number; | ||
| timeout?: number | false; |
There was a problem hiding this comment.
I tried this in a different TS project yesterday and it didn't seem to work. Are you sure this is correct?
There was a problem hiding this comment.
I'm pretty sure this works. Just tested quickly on a TS project and seemed like it did.
There was a problem hiding this comment.
This is a new feature, but not experimental. Available since TypeScript 2.0.
index.js
Outdated
| const delay = ms => new Promise(resolve => setTimeout(resolve, ms)); | ||
| const delay = ms => new Promise((resolve, reject) => { | ||
| if (ms > 2147483647) { // See #117, https://stackoverflow.com/questions/3468607/why-does-settimeout-break-for-large-millisecond-delay-values | ||
| reject(new TypeError('`timeout` can not be greater than 2147483647')); |
There was a problem hiding this comment.
Shouldn't be a TypeError, since it's a number, as expected. I would either use Error or RangeError.
Also, validation logic like this should be performed towards the top of the constructor.
There was a problem hiding this comment.
RangeError seems perfect
There was a problem hiding this comment.
True, not a type error from js perspective, fixed.
It's a side effect(we control) of setTimeout, IMO it's much clearer when it is throw from where the problem lies. Also, this way it handles all possible internal usages of delay in the future.
There was a problem hiding this comment.
To be honest, I expected this discussion. I even wrote the code to walk around this issue so there will be no need to overgeneralize constructor/docs and enforce stronger dependencies but then deleted it because it felt just too stupid to add 6 lines of code because of issue no one is expected to ever hit(now that false works). But at this point I can bring it back if you want.
There was a problem hiding this comment.
I little catch though: won't work for numbers higher than ~1e293, but no one will live long enough to know about that.
There was a problem hiding this comment.
@sindresorhus I think Got should also throw at this position.
test/main.js
Outdated
| response.end(fixture); | ||
| }); | ||
|
|
||
| await t.throwsAsync(ky(server.url, {timeout: 21474836470}).text(), TypeError); // Timeout greater than 2147483647 |
There was a problem hiding this comment.
It should throw synchronously, so that the user immediately knows something is wrong. Should also assert about the error message to be sure it's the error we are expecting.
There was a problem hiding this comment.
It should throw synchronously
Hmm... People expect a Promise so IMO it should throw asynchronously 🤔
There was a problem hiding this comment.
This is input validation, it represents a programmer error, and should cause a crash even if you don't await the promise. Keep in mind that some people use Ky in a fire-and-forget pattern where operational errors are ignored but programmer errors should still bubble up.
There was a problem hiding this comment.
Then unhandled rejection would pop up.
There was a problem hiding this comment.
The Promise spec says that Promise-returning functions should always reject, and never synchronously throw.
There was a problem hiding this comment.
@sholladay It is not input validation on library level tho, it's input validation on setTimeout level. If setTimeout was written correctly and throwing an error on timeouts bigger than const, would you add another check and throw there?
index.js
Outdated
| @@ -111,7 +111,13 @@ class TimeoutError extends Error { | |||
| } | |||
| } | |||
|
|
|||
| const delay = ms => new Promise(resolve => setTimeout(resolve, ms)); | |||
| const delay = ms => new Promise((resolve, reject) => { | |||
| if (ms > 2147483647) { // See #117, https://stackoverflow.com/questions/3468607/why-does-settimeout-break-for-large-millisecond-delay-values | |||
There was a problem hiding this comment.
I think that // See #117 is just enough :P
index.js
Outdated
| const delay = ms => new Promise(resolve => setTimeout(resolve, ms)); | ||
| const delay = ms => new Promise((resolve, reject) => { | ||
| if (ms > 2147483647) { // See #117, https://stackoverflow.com/questions/3468607/why-does-settimeout-break-for-large-millisecond-delay-values | ||
| reject(new RangeError('`timeout` can not be greater than 2147483647')); |
There was a problem hiding this comment.
| reject(new RangeError('`timeout` can not be greater than 2147483647')); | |
| reject(new RangeError('The `timeout` option cannot be greater than 2147483647')); |
szmarczak
changed the title
Limit maximum timeout, add timeout: false option, fixes #117