Skip to content

Use XHR + POST for request if CORS supported #420

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

Merged
merged 2 commits into from
Dec 28, 2015
Merged

Use XHR + POST for request if CORS supported #420

merged 2 commits into from
Dec 28, 2015

Conversation

benvinegar
Copy link
Contributor

Fixes #412

function makeRequest(opts) {
var hasCORS =
'withCredentials' in new XMLHttpRequest() ||
typeof XDomainRequest !== 'undefined';
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not an expert here, so lemme get this straight:

  • You're checking for either XMLHttpRequest that has the proper support. Is XMLHttpRequest guaranteed to exist? I assume if it's undefined, we'll have an issue calling new XMLHttpRequest()
  • Then falling back to checking for XDomainRequest, which is IE specific. This is great, but then inside makeXhrRequest, XDomainRequest isn't used or even attempted to be used. So it's not clear to me what good this check is doing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is XMLHttpRequest guaranteed to exist?

Realistically – yes, unless someone has removed it (e.g. the host page or a browser extension).

But I'll add the check.

Then falling back to checking for XDomainRequest, which is IE specific. This is great, but then inside makeXhrRequest, XDomainRequest isn't used or even attempted to be used. So it's not clear to me what good this check is doing.

Yep, that was a mistake.

Copy link
Contributor

Choose a reason for hiding this comment

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

Realistically – yes, unless someone has removed it (e.g. the host page or a browser extension)

What about the IE case though since they use XDomainRequest? Or do I misunderstand this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about the IE case though since they use XDomainRequest?

The XMLHttpRequest object still exists, and can be safely queried for presence of withCredentials.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, ok. I see. IE's XMLHttpRequest object just isn't capable of issuing a CORS request.

@benvinegar
Copy link
Contributor Author

I've tested / verified the latest version with a grab bag of browsers:

  • Chrome 46
  • Firefox 41
  • IE10, IE11
  • IE8, IE9 (XDomainRequest)
  • Edge
  • Safari 5
  • Android 4.0.4 Stock Browser

@benvinegar benvinegar merged commit 4de0c94 into master Dec 28, 2015
@mattrobenolt mattrobenolt deleted the xhr-post branch January 4, 2016 17:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants