-
-
Notifications
You must be signed in to change notification settings - Fork 41
Allow for variable timeout #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Looks good, thank you! I'll add it to the docs as well. |
|
The optional Also changed the Android implementation a little, because the original PR interfered with SSL pinning. |
|
Hi Eddy, thought about that implementation but then figured out that the Client variable will always be returned if already set. However, going by the new changes you have now made, the getClient() in the request function https.android.ts: 121 does not pass a timeout and as such the 10 seconds will always apply regardless of whatever timeout anyone has set, or have I missed something? |
|
@bobbyngwu The timeout is applied at https://github.com/EddyVerbruggen/nativescript-https/blob/ea44ec0b07bb45c1434dd4159cec72a35cc4a6ea/src/https.android.ts#L144-L149 and the client instance is returned afterward. Or do I miss something? |
|
Yes, except for the fact that line 158 where the getClient() function is called does not pass the httpsRequestOption timeout value which invariably means that the default 10 seconds will always be set? |
|
I don't really know how this slipped through my fingers, but in 1.2.1 that's now corrected. Do you have a good test environment for this? I looked at httpbin.org for instance, but didn't immediately see a "response delay" feature. |
|
Thanks Eddy, will have a look, thanks for all your hard work on this. I do have a local test environment and I am also throttling as well with an emulator, that was how I spotted it in the first place :) |
This simply allows for a configurable timeout to be applied instead of using the default 10 seconds