Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

Update httpBackend, check window.XMLHttpRequest #5679

Closed
wants to merge 2 commits into from
Closed
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Update httpBackend, check window.XMLHttpRequest
As per this issue: #5677

window.XMLHttpRequest is not always available in IE8 despite it not running in quirks mode, in which case Angular should be using the ActiveXObject instead. Just checking the browser version is taking too many shortcuts.
jorgt committed Jan 8, 2014

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
commit ac4e9a76fa2cdad8839db6eca9bdffa97192410d
2 changes: 1 addition & 1 deletion src/ng/httpBackend.js
Original file line number Diff line number Diff line change
@@ -3,7 +3,7 @@
function createXhr(method) {
// IE8 doesn't support PATCH method, but the ActiveX object does
/* global ActiveXObject */
return (msie <= 8 && lowercase(method) === 'patch')
return ((msie <= 8 && lowercase(method) === 'patch') || isUndefined(window.XMLHttpRequest))
Copy link
Contributor

Choose a reason for hiding this comment

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

this change means that we should use activex even for non-ie browsers if window.xhr is not defined. that's not right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

According to https://developer.mozilla.org/en-US/docs/Web/API/XMLHttpRequest it's only IE that needs a backfill (see Browser Compatibility). My test case is admittedly limited. Besides, right now on browsers that do not define window.xhr, the entire $http module fails which is worse.

Choose a reason for hiding this comment

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

Why not use a try...catch?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Assuming you mean this method (simplified):

var xhr;
try {
  xhr = new ActiveXObject('Microsoft.XMLHTTP');
} catch(e) {
  xhr = new window.XMLHttpRequest();
}
return xhr;

This always uses ActiveX in IE instead of XMLHttpRequest which is not really in the spirit of the original code of using ActiveX only when necessary. This was the simplest change I could make without rewriting the method. There are try / catch blocks higher up in the tree and I'm not sure what removing an exception here will do.

Choose a reason for hiding this comment

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

Not quite.

var xhr;

if( msie <= 8 && lowercase(method) === 'patch' )
{
   xhr = new ActiveXObject('Microsoft.XMLHTTP');
} else {
  try {
    xhr = new window.XMLHttpRequest();
  } catch( e ) {
    xhr = new ActiveXObject('Microsoft.XMLHTTP');
  }
}
return xhr;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will that not only throw an error when XMLHttpRequest is undefined, which is what was checked already?

I don't mind changing that though, if it's considered more sturdy.

? new ActiveXObject('Microsoft.XMLHTTP')
: new window.XMLHttpRequest();
}