Permalink
Browse files

Ajax: Fix the XHR fallback logic for IE8

The logic for IE8 has been incorrectly reversed: every non-local request
outside of the whitelist was run via the native XHR. This commit reverses
this logic and adds back a fallback to the ActiveX XHR if the native one
fails even after the regex detection.

Refs 61f812b
  • Loading branch information...
mgol committed May 19, 2015
1 parent ef30bdf commit bd699cb17b6347a414c0c6d105ef80a4ea1ed678
Showing with 2 additions and 5 deletions.
  1. +2 −5 src/ajax/xhr.js
View
@@ -31,11 +31,8 @@ jQuery.ajaxSettings.xhr = window.ActiveXObject !== undefined ?
// and http://www.w3.org/Protocols/rfc2616/rfc2616-sec9.html#sec9
// Although this check for six methods instead of eight
// since IE also does not support "trace" and "connect"
- if ( /^(get|post|head|put|delete|options)$/i.test( this.type ) ) {
- return createActiveXHR();
- }
-
- return createStandardXHR();
+ return /^(get|post|head|put|delete|options)$/i.test( this.type ) &&
+ createStandardXHR() || createActiveXHR();
} :
// For all other browsers, use the standard XMLHttpRequest object
createStandardXHR;

8 comments on commit bd699cb

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 19, 2015

Member

I guess these things happens all because of these magic && and ||

Member

markelog replied May 19, 2015

I guess these things happens all because of these magic && and ||

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 19, 2015

Member

The previous one were more magic. :) Here the if form would be longer as I'd have to write:

if ( /^(get|post|head|put|delete|options)$/i.test( this.type ) ) {
    return createStandardXHR() || createActiveXHR();
}
return createActiveXHR();

Not sure how UglifyJS handles this.

Member

mgol replied May 19, 2015

The previous one were more magic. :) Here the if form would be longer as I'd have to write:

if ( /^(get|post|head|put|delete|options)$/i.test( this.type ) ) {
    return createStandardXHR() || createActiveXHR();
}
return createActiveXHR();

Not sure how UglifyJS handles this.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 19, 2015

Member

If we have precedent, i think it is appropriate to add test for it

Member

markelog replied May 19, 2015

If we have precedent, i think it is appropriate to add test for it

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 19, 2015

Member

A test for what? We already had tests failing because of this mistake of mine.

Member

mgol replied May 19, 2015

A test for what? We already had tests failing because of this mistake of mine.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 20, 2015

Member

The previous one were more magic. :) Here the if form would be longer as I'd have to write

What is the diff in byte-size?

A test for what?

For exactly this issue, tests are failed but without concrete problem, this was logic issue, but it could have been an env problem, if we would have a test which tests only first issue we immediately would know what has failed.

Member

markelog replied May 20, 2015

The previous one were more magic. :) Here the if form would be longer as I'd have to write

What is the diff in byte-size?

A test for what?

For exactly this issue, tests are failed but without concrete problem, this was logic issue, but it could have been an env problem, if we would have a test which tests only first issue we immediately would know what has failed.

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 20, 2015

Member

What is the diff in byte-size?

+1 byte. :-)

For exactly this issue, tests are failed but without concrete problem

We already have a test - we send the PATCH request in IE8 & check if it worked; it won't work with the native XHR. This test didn't run because the POST one hung the whole suite so it didn't have a chance to reach it. But the test is there.

Member

mgol replied May 20, 2015

What is the diff in byte-size?

+1 byte. :-)

For exactly this issue, tests are failed but without concrete problem

We already have a test - we send the PATCH request in IE8 & check if it worked; it won't work with the native XHR. This test didn't run because the POST one hung the whole suite so it didn't have a chance to reach it. But the test is there.

@markelog

This comment has been minimized.

Show comment
Hide comment
@markelog

markelog May 20, 2015

Member

+1 byte. :-)

It is longer then, could we sacrifice it for the readability?

We already have a test - we send the PATCH request in IE8 & check if it worked; it won't work with the native XHR. This test didn't run because the POST one hung the whole suite so it didn't have a chance to reach it. But the test is there.

Yes, i got the idea, this was logic issue not the env one, but we could have added test for the logic too, so if logic test fails then we know exactly what is wrong, whereas with the env test we need to investigate

Member

markelog replied May 20, 2015

+1 byte. :-)

It is longer then, could we sacrifice it for the readability?

We already have a test - we send the PATCH request in IE8 & check if it worked; it won't work with the native XHR. This test didn't run because the POST one hung the whole suite so it didn't have a chance to reach it. But the test is there.

Yes, i got the idea, this was logic issue not the env one, but we could have added test for the logic too, so if logic test fails then we know exactly what is wrong, whereas with the env test we need to investigate

@mgol

This comment has been minimized.

Show comment
Hide comment
@mgol

mgol May 20, 2015

Member

+1 byte. :-)

It is longer then, could we sacrifice it for the readability?

It's more readable now than it was the expression is contained in the last check but I don't have a definite opinion here.

we could have added test for the logic too

What exactly would you want to test? If a specific condition triggers the ActiveX XHR in IE8 and others the native one? This seems wrong as we'd be testing our implementation and not the end result. One day we may decide that we want to change the logic while preserving the tests and then we'd have to modify the logic test.

In any way, if I saw that PATCH is not working in IE8 then ajax/xhr.js would be the first place to look as the code for deciding which request type to use is there. So I don't feel that I'd gain more clarity if we had test asserting our logic for the XHR fallback chain.

Member

mgol replied May 20, 2015

+1 byte. :-)

It is longer then, could we sacrifice it for the readability?

It's more readable now than it was the expression is contained in the last check but I don't have a definite opinion here.

we could have added test for the logic too

What exactly would you want to test? If a specific condition triggers the ActiveX XHR in IE8 and others the native one? This seems wrong as we'd be testing our implementation and not the end result. One day we may decide that we want to change the logic while preserving the tests and then we'd have to modify the logic test.

In any way, if I saw that PATCH is not working in IE8 then ajax/xhr.js would be the first place to look as the code for deciding which request type to use is there. So I don't feel that I'd gain more clarity if we had test asserting our logic for the XHR fallback chain.

Please sign in to comment.