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

Catching an exception on Send. This happens when request for HEAD ret… #534

Merged
merged 2 commits into from
May 14, 2019
Merged

Conversation

rybama
Copy link

@rybama rybama commented Mar 19, 2019

…urns 405 Method Not Allowed. Instead of error http status, this may throw an exception. Return false and fallback to link download.

Marcin Rybacki added 2 commits March 19, 2019 10:44
…urns 405 Method Not Allowed. Instead of error http status, this may throw an exception. Return false and fallback to link download.
@jimmywarting
Copy link
Collaborator

jimmywarting commented Mar 19, 2019

In which browser dose it throw an error?
i only get a error log.

Skärmavbild 2019-03-19 kl  11 20 21

the code still runs after it (also tried it without the onerror - same result)

seems like this PR in unnecessary

@rybama
Copy link
Author

rybama commented Mar 19, 2019

I'm gettin exception (NetworkError) on Chrome and Firefox for some reason. I'm not sure here, but could it be the case that HEAD method is not implemented on the API and thus causing the exception?

@jimmywarting
Copy link
Collaborator

Mind sharing the url you are trying so that i can test it?
also dose it throw with the httpbin url i used?

@rybama
Copy link
Author

rybama commented Mar 19, 2019

httpbin url you send doesn't throw an exception. This api endpoint for example will throw an exception https://portal.radar.f-secure.com/api/latest/productinfo (works for GET, not HEAD). I'm now more convinced that send throws an exception when certain API doesn't implement HEAD verb.

@jimmywarting
Copy link
Collaborator

hmm, when making a sync request to any url that don't implement CORS support seems to throw an error

@rybama
Copy link
Author

rybama commented Mar 21, 2019

So, is it good to merge it?

@rybama
Copy link
Author

rybama commented Mar 26, 2019

@jimmywarting kindly reminder :) ping :)

@rybama
Copy link
Author

rybama commented May 14, 2019

@jimmywarting I would like to ask to for clear comment if you intend to accept this change or not. We rely on the forked project in the npm dependencies in our project right now which I would like to remove.

@jimmywarting
Copy link
Collaborator

Sure. Will try to do it later todayy

@jimmywarting jimmywarting merged commit 71f266f into eligrey:master May 14, 2019
@jimmywarting
Copy link
Collaborator

released this with v2.0.2 on npm now

@rybama
Copy link
Author

rybama commented May 14, 2019

thanks!

@xiaoluntian
Copy link

hi why use a HEAD request,not the GET request replace.If crosed enabled,continue to use blob to download, if not, go to the other side.

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.

3 participants