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

Handle error events correctly #158

Closed
wants to merge 2 commits into from
Closed

Handle error events correctly #158

wants to merge 2 commits into from

Conversation

finnp
Copy link
Collaborator

@finnp finnp commented Mar 23, 2017

The onerror and ontimeout handler are not calling back with errors but with error events (without any messages). Therefore the errorFunc is wrapping them as Error: [ProgressEvent object].

Here's my quick proposal on how to fix this. Just pass generic error messages to the errorFunc, then there will be no case where the error needs to be constructed there.

The spec can be quite helpful here: https://xhr.spec.whatwg.org/

The `onerror` and `ontimeout` handler are not calling back with errors but with error events (without any messages). Therefore the `errorFunc` is wrapping them as `Error: [ProgressEvent object]`. 

Here's my quick proposal on how to fix this. Just pass generic error messages to the `errorFunc`, then there will be no case where the error needs to be constructed there.
@naugtur
Copy link
Owner

naugtur commented Mar 23, 2017

It's not a surprise. The check for error type was there for the case when the function is called with an error. The split between error and timeout could be useful though.

It's a breaking change, so I'll schedule merging this for 3.0

@naugtur naugtur added this to the v3.0 milestone Mar 23, 2017
@krazik
Copy link

krazik commented Apr 26, 2017

+1 for a fix for this.

@naugtur
Copy link
Owner

naugtur commented Apr 28, 2017

Closing this one in favor of #164 - the same thing but rebased on top of current development.

@naugtur naugtur closed this Apr 28, 2017
@finnp finnp deleted the patch-1 branch April 28, 2017 12:56
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

3 participants