mootools / mootools-core

MooTools Core Repository
https://mootools.net
2.65k stars 510 forks source link

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

Open lorenzos opened 8 years ago

lorenzos commented 8 years ago

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!](https://www.bountysource.com/issues/31955539-shouldn-t-request-json-fire-complete-event-on-json-errors?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github)** We accept bounties via [Bountysource](https://www.bountysource.com/?utm_campaign=plugin&utm_content=tracker%2F22067&utm_medium=issues&utm_source=github).
SergioCrisostomo commented 8 years ago

Maybe onerror could call onFailure.

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

lorenzos commented 8 years ago

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 commented 8 years ago

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

lorenzos commented 8 years ago

(sorry, I accidentally clicked the wrong button)