Skip to content
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

Add timeout for requests. #8

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

avshenuk
Copy link

Adds timeout support.

Important for production-ready apps.

Copy link
Owner

@javorosas javorosas left a comment

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
Copy link
Author

avshenuk commented May 26, 2018

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