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

Conversation

Projects
None yet
2 participants
@rybama
Copy link

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 some commits Mar 19, 2019

Marcin Rybacki
Catching an exception on Send. This happens when request for HEAD ret…
…urns 405 Method Not Allowed. Instead of error http status, this may throw an exception. Return false and fallback to link download.
Marcin Rybacki
@jimmywarting

This comment has been minimized.

Copy link
Collaborator

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

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

@rybama

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Collaborator

commented Mar 19, 2019

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

@rybama

This comment has been minimized.

Copy link
Author

commented Mar 21, 2019

So, is it good to merge it?

@rybama

This comment has been minimized.

Copy link
Author

commented Mar 26, 2019

@jimmywarting kindly reminder :) ping :)

@rybama

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

Sure. Will try to do it later todayy

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

1 check passed

security/snyk - package.json (eligrey) No manifest changes detected
@jimmywarting

This comment has been minimized.

Copy link
Collaborator

commented May 14, 2019

released this with v2.0.2 on npm now

@rybama

This comment has been minimized.

Copy link
Author

commented May 14, 2019

thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.