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

fix($httpBackend): use ActiveX XHR when making PATCH requests on IE8 #5390

Merged
merged 2 commits into from
Jan 3, 2014

Conversation

IgorMinar
Copy link
Contributor

IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

Closes #2518
Closes #5043

@caitp
Copy link
Contributor

caitp commented Dec 31, 2013

Some quick ways to get tests passing again without getting rid of the noxhr minerr:

@mgol
Copy link
Member

mgol commented Jan 2, 2014

@caitp jQuery 1.x uses only the simple form:
https://github.com/jquery/jquery/blob/3140d3bdd3a520e98c5178b04eba68ce16dacf46/src/ajax/xhr.js#L179-183
and it works fine all the way back to IE6 so I don't think such granularity is needed.

@IgorMinar
Copy link
Contributor Author

silly me! I didn't realize that this PR didn't pass our tests.

sorry @caitp for wasting your time.

the noxhr error is thrown because I removed the only instance of the error from the code, but not the doc for this error and the documentation generation code doesn't expect to come across an error doc for an error that is not being used.

as @mzgol it should be fine to use the simplified version of the xhr instantiation. I checked with jquery source as well as some other places and this short form is legit.

IE8's native XHR doesn't support PATCH requests, but the ActiveX one does.

I'm also removing the noxhr error doc because nobody will ever get that error.

Closes angular#2518
Closes angular#5043
@IgorMinar
Copy link
Contributor Author

ok, I pushed a new version now. it should be good to go.

@IgorMinar
Copy link
Contributor Author

travis/saucelabs/karma is not having a good day today, but our ci server says that this is good to go: http://ci.angularjs.org/job/angular.js-igor/1159/

@caitp
Copy link
Contributor

caitp commented Jan 3, 2014

I don't consider it a waste of time at all --- this appears, as you said, to be a widely adopted solution, but I would still prefer to see tests where the browser can make real requests, just so that we can verify that the actual transport stuff is working as expected across all supported browsers, and have some kind of alert when if/it breaks.

I don't know if there's a good infrastructure for that right now, but maybe it could be tacked on as an afterthought later, I'm not sure.

But if this is good to merge, then awesome

@IgorMinar
Copy link
Contributor Author

we don't have a good infrastructure for this kind of test right now. we could write a e2e test and add it to our build system, but it's not worth doing for this particular issue since it's an IE8 thing and you know how we feel about IE8.

instead I manually tested this and verified that it works as expected and in fact I found an unrelated issue that I also fixed in fd9a03e. IE8 is such pleasure to work with...

@IgorMinar IgorMinar merged commit fd9a03e into angular:master Jan 3, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Using the HTTP PATCH method with $http results in a "Invalid argument" exception on IE8
5 participants