ECONNRESET err.code for abort()

[mattdesl]

Currently you can abort requests like so:

var req = xhr(url, (err, resp) => {
   // ...
})

// when user cancels request
cancelButton.on('click', () => req.abort())

However, it just passes err as "Internal XMLHttpRequest Error" which makes it hard to distinguish between user-initiated cancellation and actual XHR errors.

If possible, it would be nice to emulate Node and use ECONNRESET for err.code when the error is due to an abort() rather than, say, a connection or JSON parse error. This way abstractions like xhr-request that work in Node/Browser could both use the same logic for handling req.abort().

[codepunkt]

Yes, please!

I get quite a bit of 'Internal XMLHttpRequest Error' entries in our error-logging due to aborting requests on purpose :(

[TehShrike]

This seems very related to #104. These issues could probably be merged once we decide what xhr should actually be doing when a XMLHttpRequest is aborted.

[naugtur]

To emulate node (https://github.com/request/request to be precise) it would require to skip calling the callback at all, as in https://github.com/Raynos/xhr/tree/abort-timeout

Any feedback on that?

[naugtur]

Requesting feedback ;)

[TehShrike]

I haven't poked at request, but if it skips returning any output after cancel is called, I don't have any issue with copying that.

[naugtur]

#104 is fixed, this should not be an issue anymore.
Feel free to reopen if anything's wrong.

[stuartsan]

I am running into an issue here where if I call abort() immediately, the callback is still called, with an error. For a minimal example, this test fails in Chrome 55 (using npm run browser):

test("aborting XHR prevents callback from being called", { timeout: 500 }, function(assert) {
    var req = xhr({ uri: "/mock/200ok" }, function(err, response) {
      assert.fail('this callback should not be called');
    });
    req.abort();
    assert.end()
})

AFAICT, loadFunc and specifically this code path is executing before the xhr.onabort handler sets aborted to true here.

Is there anything I'm missing here? I'd be happy to take a crack at a PR if I can figure out a good approach to fixing this.

[stuartsan]

I can make the test above pass with the following code modification, pushing the onreadystatechange call to loadFunc into the next callstack using setTimeout:

function readystatechange() {
    if (xhr.readyState === 4) {
        setTimeout(loadFunc, 0)
    }
}

Any thoughts on this approach?

[stuartsan]

(I don't think I have GH permissions or I would re-open this issue 😄 )

[naugtur]

The issue was about something else and I'd rather you created a new one, but there's valuable content here now and I came too late, so there you go.

Please create the PR and I'll get to it on Monday to check if it's the way to go.

I'm not yet sure if the new behavior is better than the current one for everybody.

[stuartsan]

Ok cool, I will PR this, thanks @naugtur. Also I'm happy to open a separate issue if that's more useful to you...LMK.