Skip to content

Add timeout for requests.#8

Open
avshenuk wants to merge 1 commit into
javorosas:masterfrom
avshenuk:add-timeout
Open

Add timeout for requests.#8
avshenuk wants to merge 1 commit into
javorosas:masterfrom
avshenuk:add-timeout

Conversation

@avshenuk

Copy link
Copy Markdown

Adds timeout support.

Important for production-ready apps.

@javorosas javorosas left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution! It's great to have people interested in this project.

I'm pushing this one back because it adds too many dependencies for something that can be accomplished without any. Check the Promise.race approach here: https://gist.github.com/davej/728b20518632d97eef1e5a13bf0d05c7
Simple enough. No need for more dependencies.

@avshenuk

avshenuk commented May 26, 2018

Copy link
Copy Markdown
Author

The solution by that link doesn't cancel the actual HTTP request sent. It just raises an error. That's where 'cancellable-fetch' lib comes into play.

And 'promise-timeout' lib I've added actually uses Promise.race approach you mentioned. The only diff is it clears timeout if the request is successful or any other error happens.
In React native a non-cleared timeout can trigger warnings which look not really elegant... But for sure this can be easily implemented manually but why?

'uuid' lib was needed for identifying the HTTP request to cancel although here I agree it can be just replaced by a simple counter.

@javorosas

Copy link
Copy Markdown
Owner

What you mention makes total sense. However, that would mean we should move away from the original implementation of the fetch API spec, included in ReactNative.

cancellable-fetch is a reimplementation. I'm not that convinced.

I'm personally not interested in turning this into a fully-featured http library, there are great solutions out there making a great job at this.

Let's keep the scope for now on being an abstraction of the fetch API. I'm reading that aborting http requests is already in the plans. Let's implement it when it's available for RN. Or is it already?

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants