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

Ajax: Simplify jQuery.ajaxSettings.xhr #4467

Merged
merged 1 commit into from
Aug 26, 2019
Merged

Conversation

mgol
Copy link
Member

@mgol mgol commented Aug 21, 2019

Summary

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
Ref gh-4347

-8 bytes

Checklist

@mgol mgol added the Ajax label Aug 21, 2019
@mgol mgol added this to the 4.0.0 milestone Aug 21, 2019
@mgol mgol self-assigned this Aug 21, 2019
@mgol mgol mentioned this pull request 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
try {
return new window.XMLHttpRequest();
} catch ( e ) {}
return new window.XMLHttpRequest();
Copy link
Member

Choose a reason for hiding this comment

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

In environments like jsdom the user would need to ensure there's a fake XHR stub in window or jQuery will throw an error here. That's a bit of a different behavior than currently, where it returns undefined. Any attempt to use $.ajax will fail either way, but I wonder if anyone calls $.ajaxSettings.xhr() to detect whether XHR is enabled. Probably not?

Copy link
Member

Choose a reason for hiding this comment

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

A call to $.ajax would fail anyway. Only difference is the exception might actually be a little more informative with this change. I'm pretty certain nobody actually calls $.ajaxSettings.xhr() as a means to control if there is an XHR implementation and the change does not preclude from overriding $.ajaxSettings.xhr if needs be.

It's a +1 for me.

@mgol mgol merged commit abdc89a into jquery:master Aug 26, 2019
@mgol mgol deleted the xhr-simplify branch August 26, 2019 17:01
@mgol mgol removed the Needs review label Aug 26, 2019
@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.
Labels
Development

Successfully merging this pull request may close these issues.

Simplify XHR detection
4 participants