dsherret / dax

Cross-platform shell tools for Deno and Node.js inspired by zx.
MIT License
997 stars 34 forks source link

Add a default timeout to `$.request` #90

Closed andrewbrey closed 1 year ago

andrewbrey commented 1 year ago

I think that a case can be made in either direction for whether or not $.request should have a default timeout (as of now, it does not have one). Since the principle use case of dax is as a scripting tool, presumably it will frequently have uses in:

and other places where it can be reasonably expected that a human is not sitting there watching a script proceed (and able to notice that a request is hung due to any number of potential issues).

Due to the fact that the current implementation makes no distinction between the various parts of making a request (and I don't know if it even reasonably can while remaining a simple fetch under the hood) such as the time it takes to connect, to lookup DNS, to actually download the data, etc, I think it makes sense to have this default timeout be "generous" within the context of calling an API or downloading a webpage. I would suggest something like 60_000 (one minute) as the default.

I looked around at default http clients from different ecosystems (ruby, php, go, etc) and found that it's pretty common not to have a default timeout, so the precedent is definitely there to keep it as is, but in my opinion it makes sense to have a default for the role that dax fills.

Thanks for considering!

dsherret commented 1 year ago

I don't think this should be done because the default timeout value to pick depends on the scenario. Generally on CIs people will have an overall CI timeout and if they would like to use one on requests then they can build their own $ with a default $.request timeout that works for their situation.

For example, say we add the default timeout to be 1 minute. Occasionally on GitHub CIs something might just hang for 2 minutes or so before completing successfully. Us not having a timeout means that request completes after 2 minutes successfully and the CI completes successfully. Us having a timeout means we fail that request and the CI run fails. So it might end up annoying people quite a bit if we introduce a default timeout (even if it's from 1 hour... somewhat related, I once had a 1 hour 30 minute bulk update program timeout on me while it was progressing and that was not very pleasant).

By the way, looking at the API I realized that there was no way to specify a delay string like "1s" to RequestBuilder#timeout. I fixed that in #91. I'll do a release a bit later today.

andrewbrey commented 1 year ago

Ya, that's for sure possible, and your stance makes sense!

Thanks for taking a look, feel free to close if you think it's not the right default to provide. Also, thanks for #91, cheers!