Skip to content
This repository was archived by the owner on Apr 4, 2019. It is now read-only.

Change XMLHttpRequest error handling #5

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Change XMLHttpRequest error handling #5

wants to merge 3 commits into from

Conversation

gnandretta
Copy link

  • Listen for 'load' event instead of 'readystatechange' because the
    later can be triggered along with the 'error' event.
  • Check the response status code, and invoke the callback with an
    error argument when it is not in the 20x range.

- Listen for 'load' event instead of 'readystatechange' because the
  later can be triggered along with the 'error' event.
- Check the response status code, and invoke the callback with an
  error argument when it is not in the 20x range.
@cristiandouce
Copy link

So, this avoids the fn to be called twice when there is an error, right? @gnandretta

@gnandretta
Copy link
Author

@cristiandouce yes, and it will also invoke the callback with an error argument when the response doesn't succeed, for instance when a 404 is returned.

if (4 == req.readyState) return fn(null, req);
// sometimes IE returns 1223 when it should be 204
// see http://stackoverflow.com/questions/10046972/msie-returns-status-code-of-1223-for-ajax-request
/^(20\d|1223)$/.test(req.status) ? fn(null, req) : fn(req);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't the falsey case here create an error to bubble up?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gjohnson Sure! I've also created the error for the call in the onerror handler as well. See commit 13ccd0d

- Rename error.evt to error.event for the network error case.
- Pass the request as the second argument instead of as an event
  property for the unsuccessful request case.
@gnandretta
Copy link
Author

@gjohnson any chance on get this merged?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants