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

Shouldn't Request.JSON fire 'complete' event on JSON errors? #2764

Open
lorenzos opened this issue Mar 16, 2016 · 4 comments
Open

Shouldn't Request.JSON fire 'complete' event on JSON errors? #2764

lorenzos opened this issue Mar 16, 2016 · 4 comments

Comments

@lorenzos
Copy link
Contributor

lorenzos commented Mar 16, 2016

When Request.JSON fails because response cannot be correctly parsed, the 'error' event is fired. 'error' is not an event inherit from Request, and is fired here:

try {
    json = this.response.json = JSON.decode(text, this.options.secure);
} catch (error){
    this.fireEvent('error', [text, error]);
    return;
}

Shouldn't it fire also the 'complete' event? I use 'complete', for example, to hide progress indicators (like spinners), and for me the request is completed in case of errors too. Maybe my assumption is wrong, but then why the 'failure' event does it my way instead?

onFailure: function(){
    this.fireEvent('complete').fireEvent('failure', this.xhr);
}


Want to back this issue? Post a bounty on it! We accept bounties via Bountysource.

@SergioCrisostomo
Copy link
Member

Maybe onerror could call onFailure.

If we keep it like it is it should at least be documented.

@lorenzos
Copy link
Contributor Author

Not sure about firing 'failure', docs says it is for when the request failed (error status code), here we aren't dealing with a failed request, but more likely with an unexpected response (i.e. not JSON).

IMHO the three possible options are, in order of my preference:

  1. Modify Request.JSON line 39 with this.fireEvent('complete').fireEvent('error', [text, error]);.
  2. Fix documentation, specifying 'complete' is not fired in case of successful request but parsing error.
  3. Do nothing.

@SergioCrisostomo
Copy link
Member

I'm ok with #1 and #2:) lets wait for what others say.

@lorenzos lorenzos reopened this Aug 29, 2016
@lorenzos
Copy link
Contributor Author

(sorry, I accidentally clicked the wrong button)

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

No branches or pull requests

2 participants