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

timeout arg for file actions #49

Closed
wants to merge 1 commit into from

Conversation

realhidden
Copy link
Collaborator

In some cases the b2 servers just wait forever to respond, or the upload is stuck for some reason.

By passing a timeout value you can at least get a reject, and retry the upload. The timeout directly goes into Axios, and is in ms.

@yakovkhalinsky yakovkhalinsky added this to the 1.0.6 release milestone Jan 15, 2019
Copy link
Owner

@yakovkhalinsky yakovkhalinsky left a comment

Choose a reason for hiding this comment

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

Looks great and has tests!

@cbess
Copy link
Collaborator

cbess commented Jan 15, 2019

Looks good to me as well.

@yakovkhalinsky yakovkhalinsky requested a review from odensc January 16, 2019 22:18
Copy link
Collaborator

@odensc odensc left a comment

Choose a reason for hiding this comment

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

Code looks good to me, but I don't see a test for the actual timeout logic. (just updates for the existing tests) Could you possibly add a test and also something to the readme about the timeout option?

@odensc odensc removed this from the 1.0.6 release milestone Jan 20, 2019
@Amit-A
Copy link
Collaborator

Amit-A commented Feb 17, 2019

Code looks good to me, but I don't see a test for the actual timeout logic. (just updates for the existing tests)

@odensc it looks like the timeout parameter is just being passed on to axios - does it really need testing?

@odensc
Copy link
Collaborator

odensc commented Feb 17, 2019

@Amit-A True, missed that on first pass. Still would like a mention in the readme though.

@Amit-A
Copy link
Collaborator

Amit-A commented Feb 17, 2019

Why not just take an axios object parameter that we Object.assign({}, axios, options)? This way we can add any axios option (not just timeout).

@Amit-A
Copy link
Collaborator

Amit-A commented Feb 20, 2019

Why not just take an axios object parameter that we Object.assign({}, axios, options)? This way we can add any axios option (not just timeout).

Just implemented this in #62

@Amit-A
Copy link
Collaborator

Amit-A commented Feb 26, 2019

#62 merged: enables timeouts (and any other axios feature) at the request level.

@Amit-A Amit-A closed this Feb 26, 2019
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.

None yet

5 participants