Skip to content

Conversation

@bobbyngwu
Copy link

This simply allows for a configurable timeout to be applied instead of using the default 10 seconds

@EddyVerbruggen
Copy link
Member

Looks good, thank you!

I'll add it to the docs as well.

@EddyVerbruggen EddyVerbruggen added this to the 1.2.0 milestone Oct 6, 2019
@EddyVerbruggen EddyVerbruggen merged commit 2cddd67 into nativescript-community:master Oct 6, 2019
EddyVerbruggen added a commit that referenced this pull request Oct 6, 2019
@EddyVerbruggen
Copy link
Member

The optional timeout property now expects a number of seconds.

Also changed the Android implementation a little, because the original PR interfered with SSL pinning.

@bobbyngwu
Copy link
Author

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?

@EddyVerbruggen
Copy link
Member

@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?

@bobbyngwu
Copy link
Author

bobbyngwu commented Oct 6, 2019

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?

@EddyVerbruggen EddyVerbruggen modified the milestones: 1.2.0, 1.2.1 Oct 7, 2019
EddyVerbruggen added a commit that referenced this pull request Oct 7, 2019
@EddyVerbruggen
Copy link
Member

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.

@bobbyngwu
Copy link
Author

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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants