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

Add XHR responseType tests #1006

Merged
merged 1 commit into from Jul 30, 2013
Merged

Add XHR responseType tests #1006

merged 1 commit into from Jul 30, 2013

Conversation

@mathiasbynens
Copy link
Contributor

@mathiasbynens mathiasbynens commented Jul 28, 2013

I’ve used the same tests here: http://mathiasbynens.be/demo/xhr-responsetype

@mathiasbynens
Copy link
Contributor Author

@mathiasbynens mathiasbynens commented Jul 28, 2013

Ah, grunt travis was failing because in Safari/WebKit/PhantomJS setting xhr.responseType to a value it doesn’t support throws an error:

SYNTAX_ERR: DOM Exception 12: An invalid or illegal string was specified.

Gotta love unit tests. Amended the commit, wrapping the responseType assignment in a try…catch.

mathiasbynens added a commit that referenced this issue Jul 28, 2013
@patrickkettner
Copy link
Member

@patrickkettner patrickkettner commented Jul 30, 2013

Is there a reason you are using multiple tests, rather than using a bool (like the audio tests)? You'd be able to reduce the code considerably, since its basically the same test with a few different strings.

@stucox
Copy link
Member

@stucox stucox commented Jul 30, 2013

Personally (and @ryanseddon has said this in the past too), I prefer separate tests to using subproperties – they're easier to work with, fit better when using classes etc. But maybe this is a valid application for subproperties.

Even if sticking with multiple tests, we could reduce the duplication:

  • Detects for individual request types could have an AMD dependency on Modernizr.xhrresponsetype, to avoid repeating the if (typeof XMLHttpRequest == 'undefined') { … and 'response' in xhr parts

  • Could add a testXhrType() function to src/ which the individual tests could use:

    testXhrType('arraybuffer')

@mathiasbynens
Copy link
Contributor Author

@mathiasbynens mathiasbynens commented Jul 30, 2013

Thanks for the feedback!

@stucox I went with separate tests that don’t depend on each other because these tests are very specific. If you ever need them, chances are you only need one of them, but not more than that. Let’s say you use responseType='document' in your app and you want to detect support for it. In that situation it would be slower to run Modernizr.xhrresponsetype first, only to continue to Modernizr.xhrresponsetypedocument afterwards. Two XMLHttpRequest instances will have been created (and garbage collected) at that point, just to get the result for a single feature test. In other words, my patch is optimized for the common case where only a single responseType test is used.

TL;DR -1 to introducing an AMD dependency on Modernizr.xhrresponsetype, but +1 to adding a testXhrType() to src/. (I didn’t know that would be allowed.) I’ll tweak my pull request.

@mathiasbynens
Copy link
Contributor Author

@mathiasbynens mathiasbynens commented Jul 30, 2013

Amended the commit. Please review.

@stucox
Copy link
Member

@stucox stucox commented Jul 30, 2013

Looks good.

OCD Stu would like to see balanced gaps above and below the text in the /* DOC */ blocks though 😝 – currently you've got a space above but no space below.

@mathiasbynens
Copy link
Contributor Author

@mathiasbynens mathiasbynens commented Jul 30, 2013

@stucox Done. I had copied the DOC block from feature-detects/network/eventsource.js which had the same issue.

@stucox
Copy link
Member

@stucox stucox commented Jul 30, 2013

Top stuff. Thanks Mr Bynens!

stucox pushed a commit that referenced this issue Jul 30, 2013
@stucox stucox merged commit 7a84706 into master Jul 30, 2013
1 check passed
@mathiasbynens mathiasbynens deleted the xhr-responsetype branch Jul 30, 2013
patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this issue Feb 22, 2015
patrickkettner pushed a commit to patrickkettner/Modernizr that referenced this issue Feb 22, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants