Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Simplify XHR detection #1967

Closed
FarSeeing opened this issue Dec 24, 2014 · 17 comments · Fixed by #4467
Closed

Simplify XHR detection #1967

FarSeeing opened this issue Dec 24, 2014 · 17 comments · Fixed by #4467
Assignees
Milestone

Comments

@FarSeeing
Copy link
Contributor

This is the XMLHttpRequest object detection code:

https://github.com/jquery/jquery/blob/master/src/ajax/xhr.js

jQuery.ajaxSettings.xhr = function() {
    try {
        return new XMLHttpRequest();
    } catch ( e ) {}
};
...
xhrSupported = jQuery.ajaxSettings.xhr();
...
support.cors = !!xhrSupported && ( "withCredentials" in xhrSupported );
support.ajax = xhrSupported = !!xhrSupported;

support.ajax is used nowhere in the code and is not documented, XMLHttpRequest object is supported in all modern browsers (including Android 2.3, iOS 6.1, Opera 12 and IE 9 — caniuse.com).
Why not remove xhrSupported variable together with support.ajax and try-catch block?

@gibson042
Copy link
Member

Sounds like a great idea; I only see references to it in test/unit/support.js and those can go away as well since we don't guarantee any jQuery.support properties. Would you like to file a PR?

@markelog
Copy link
Member

markelog commented Dec 24, 2014

XMLHttpRequest object is supported in all modern browsers

Right, but you can disable it in IE, but i'd say we shouldn't support that anymore.

try..catch was always weird me out here, it's understanble why it was used for activeX but I hear it's pointless.

xhr method was never documented and tested so i'd say we should get rid of it too.

Not sure about support.cors property, we never used it anywhere else, but it might be useful for tests, so i either remove it or add tests for it.

/cc @jaubourg

@FarSeeing
Copy link
Contributor Author

Not sure about support.cors property, we never used it anywhere else, but it might be useful for tests, so i either remove it or add tests for it.

withCredentials property has appeared only in IE10 (MSDN) so you cannot just drop that check.

FarSeeing added a commit to FarSeeing/jquery that referenced this issue Dec 26, 2014
FarSeeing added a commit to FarSeeing/jquery that referenced this issue Dec 26, 2014
@mgol
Copy link
Member

mgol commented Dec 26, 2014

@markelog

XMLHttpRequest object is supported in all modern browsers

Right, but you can disable it in IE, but i'd say we shouldn't support that anymore.

I tend to agree, especially on master (I'd like to hear @dmethvin's thoughts on that, though).

Not sure about support.cors property, we never used it anywhere else, but it might be useful for tests, so i either remove it or add tests for it.

IMO we should keep it. Our detection code is not perfect in weird setups so it's nice if people can workaround potential problems just by setting jQuery.support.cors to true (even if we don't officially support it). See e.g. #1881.

@jaubourg
Copy link
Member

XMLHttpRequest object is supported in all modern browsers

Right, but you can disable it in IE, but i'd say we shouldn't support that anymore.

I tend to agree, especially on master (I'd like to hear @dmethvin's thoughts on that, though).

Hence the try/catch statement that prevents crashing at startup.

Not sure about support.cors property, we never used it anywhere else, but it might be useful for tests, so i either remove it or add tests for it.

IMO we should keep it. Our detection code is not perfect in weird setups so it's nice if people can workaround potential problems just by setting jQuery.support.cors to true (even if we don't officially support it). See e.g. #1881.

We should definitely keep support.cors, meaning we need to create an xhr at startup, meaning we need the try/catch.

@mgol
Copy link
Member

mgol commented Dec 26, 2014

We should definitely keep support.cors, meaning we need to create an xhr at startup, meaning we need the try/catch.

Does IE with native XHR disabled still have the XMLHttpRequest global but it just crashes? Otherwise it'd be better to change the code so that it's a noop or sth like that and not rely on try/catch.

@jaubourg
Copy link
Member

(on a side note, we also need to think about those non-browser environments that may not have an xhr implementation into which we may not want to fail miserably)

Does IE with native XHR disabled still have the XMLHttpRequest global but it just crashes? Otherwise it'd be better to change the code so that it's a noop or sth like that and not rely on try/catch.

Whether it's undefined or throws an exception when the constructor is invoked is irrelevant since both cases are handled by the try/catch. It's pretty solid as is which was the point.

@mgol
Copy link
Member

mgol commented Dec 26, 2014

(on a side note, we also need to think about those non-browser environments that may not have an xhr implementation into which we may not want to fail miserably)

Right; I'll land #1949 in a moment so that we get covered here.

@mgol
Copy link
Member

mgol commented Dec 26, 2014

Whether it's undefined or throws an exception when the constructor is invoked is irrelevant since both cases are handled by the try/catch.

Right but if it never throwed then it'd be better to avoid try/catch. But I guess we don't have the guarantee.

@jaubourg
Copy link
Member

But I guess we don't have the guarantee.

Exactly. The module we're talking about being ajax, I tend to apply a "better safe than sorry" policy ;)

@FarSeeing
Copy link
Contributor Author

Does IE with native XHR disabled still have the XMLHttpRequest global but it just crashes?

I've checked IE11 — you just cannot disable XHR at all. Will check IE9 and IE10 later.

@FarSeeing
Copy link
Contributor Author

Checked IE9: even when you disable XHR, window.XMLHttpRequest is still available.

@markelog
Copy link
Member

IMO we should keep it. Our detection code is not perfect in weird setups so it's nice if people can workaround potential problems just by setting jQuery.support.cors to true (even if we don't officially support it).

Then we should document and support it, otherwise we will face more problems in the future. But i wouldn't do that, the only reason i see to leave it in, is for the tests.

Besides, there are way to do that, documented way. Whereas setting jQuery.support.cors is like using ajaxSetup which is not a preferable way, if we say that it's okay, then we're inviting user to change other support properties if he feels like it :-(.

@jaubourg it would be interesting to see in what unsupported envs we would fail, if we would, it would be on the low amount of users, but if we wound't, then we could avoid the "just in case" code.

@dmethvin
Copy link
Member

We have too many undocumented edges and I would like to reduce them. jQuery.support is something we've been trying to kill as an external object.

@FarSeeing
Copy link
Contributor Author

Second thought: why not just remove support.cors?
If the client does not support withCredentials property, but provides some other way to make cross-domain requests (like easyXDM library), it will not have any other way to send request but to force support.cors property. Real life example.

@timmywil
Copy link
Member

We'll investigate further for 3.1.0.

@timmywil timmywil modified the milestones: 3.2.0, 3.3.0 Mar 6, 2017
@timmywil timmywil self-assigned this Mar 27, 2017
@timmywil timmywil removed this from the 3.3.0 milestone Dec 11, 2017
@timmywil timmywil added this to the 3.4.0 milestone Dec 11, 2017
@timmywil timmywil modified the milestones: 3.4.0, 4.0.0 Sep 24, 2018
mgol added a commit to mgol/jquery that referenced this issue Aug 21, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since jquerygh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes jquerygh-1967
Ref jquerygh-4347
@mgol
Copy link
Member

mgol commented Aug 21, 2019

jQuery.support.ajax & jQuery.support.cors have been removed in #4347 and will not be defined in jQuery 4.0. The remaining part is the try-catch around jQuery.ajaxSettings.xhr which I'm removing in #4467.

mgol added a commit to mgol/jquery that referenced this issue Aug 22, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since jquerygh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes jquerygh-1967
Ref jquerygh-4347
mgol added a commit that referenced this issue Aug 26, 2019
Previously, jQuery.ajaxSettings.xhr, contents were wrapped in a try-catch
as we defined jQuery.support.ajax & jQuery.support.cors executed during the
jQuery load and we didn't want to crash if IE had native XHR disabled (which
is possible). While jQuery hasn't supported the ActiveX-based XHR since 2.0,
jQuery with XHR disabled could still be used for its other features in such
a crippled browser.

Since gh-4347, jQuery.support.ajax & jQuery.support.cors no longer exist, so
we don't need the try-catch anymore.

Fixes gh-1967
Closes gh-4467
Ref gh-4347
@lock lock bot locked as resolved and limited conversation to collaborators Feb 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.