Use Raynos/xhr for XMLHttpRequests #2318
Conversation
Get rid of our custom XHR shim. Export it as videojs.xhr.
| @@ -249,7 +249,7 @@ var parseCues = function(srcContent, track) { | |||
| }; | |||
|
|
|||
| var loadTrack = function(src, track) { | |||
| XHR(src, Fn.bind(this, function(err, response, responseBody){ | |||
| XHR({ uri: src }, Fn.bind(this, function(err, response, responseBody){ | |||
There was a problem hiding this comment.
nit: I believe xhr does this for us already.
There was a problem hiding this comment.
Looks like it does. I'm not unhappy with this version though. You?
|
LGTM |
|
Raynos/xhr is incompatible with sinon because it captures a reference to window.XHR on module load. We use fakeXHR in a bunch of projects for testing purposes. It would be a real pain to have to find an alternate solution for testing. |
|
This shouldn't be pulled in unless naugtur/xhr#80 gets merged and released. |
While we wait for Raynos/xhr to allow for downstream testing, expose our own XHR shim. An alternative to videojs#2318.
|
The XHR update has been merged but not released. Can we point to master for now? |
|
Another shortcoming: it doesn't seem possible to distinguish between request timeout errors and other failure conditions. HLS has different behavior for requests timeouts (aggressively downshifting quality) versus other errors (move along to the next segment). We would have trouble adopting Raynos/xhr until it was possible to distinguish between the two. |
|
Ugh. Want to submit an issue? Maybe there's a way of discovering that we're not aware of. |
|
nope, there's no good way of figuring it out. It just returns an error with "unknown" as the message. |
|
We can't use the response status code, or lack of, or anything that knows |
|
Closing in favor of exposing our internal helper for now. |
|
Opened naugtur/xhr#88 despite the naysayers |
While we wait for Raynos/xhr to allow for downstream testing, expose our own XHR shim. An alternative to videojs#2318.
|
All latest changes are published as xhr@2.0.3 If you want to have separate handling for timeout in browsers that support it correctly, it's actually possible ever since 1.something. We'll see if there's a reason to get naugtur/xhr#88 implemented, but I need more context. |
|
Reopening at least as a placeholder, since it looks like we can get this in for 5.0. |
|
@dmlap I know there's an open issue around this, but if the verdict is we can make this work as is for HLS, I'm gonna go ahead and pull this in to get this closed down. Any objection? |
|
No, we can't get this working as-is for HLS. It sounds like there is a good possibility that will change soon but it's not ready yet. |
|
Ok sorry, I thought there was just a nice to have left. Moving to 5.1, and we'll see where it gets in the next week. |
|
Hi, I'm back from vacation. Check out naugtur/xhr#92 in case you want to share feedback before I release it. |
- Get rid of our custom XHR shim. Export it as videojs.xhr. - Updated XHR to be stubbed everywhere in tests to prevent errors. - Added npm install to the review process closes videojs#2318 closes videojs#2594
Get rid of our custom XHR shim. Export it as videojs.xhr.