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

Updated #2318 - Use raynos/xhr - rebased and fixed test errors #2594

Merged
merged 1 commit into from
Sep 15, 2015

Conversation

heff
Copy link
Member

@heff heff commented Sep 15, 2015

closes #2318

The only major change from #2318 is I moved sinon XHR stubbing to the cover all tests, because with raynos/xhr we have to stub it before the xhr module is ever loaded.

@heff heff added this to the v5.0.0 milestone Sep 15, 2015
* });
*
* Check out the [full
* documentation](https://github.com/Raynos/xhr/blob/master/README.md)
Copy link
Member

Choose a reason for hiding this comment

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

https://github.com/Raynos/xhr/blob/v2.1.0/README.md would be safer but not a huge issue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good call

@gkatsev
Copy link
Member

gkatsev commented Sep 15, 2015

Showing 8 changed files with 36 additions and 211 deletions.
Woot. LGTM

@heff
Copy link
Member Author

heff commented Sep 15, 2015

Thanks!

- 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
@heff heff force-pushed the review-dmlap-expose-xhr branch from b28ab03 to 24cdbde Compare September 15, 2015 22:26
@heff heff merged commit 24cdbde into videojs:master Sep 15, 2015
@naugtur
Copy link

naugtur commented Sep 16, 2015

In xhr 2.0+ (and sone earlier versions) there are multiple ways you can stub or replace xmlhttprequest any time. Sorry I didn't notice what you did earlier. Adding a single line after the one with sinon stub would suffice.
I can help you get that done if you wish to go back to your previous stubbing location.

Anyway, I'm glad to see more adoption :)

@heff
Copy link
Member Author

heff commented Sep 16, 2015

Thanks for the note @naugtur (and for the help in getting this done!). It looked like the other ways to stub xhr required us to change the app code to allow passing an xhr object in. That didn't seem ideal, and the global stub is actually kind of nice because it removes the issue from any future tests. If there's other ways to stub that I'm missing though I'd be interested to know.

@naugtur
Copy link

naugtur commented Sep 16, 2015

I'd suggest using the other option from the docs (at the end of README.md) and doing just

xhr.XMLHttpRequest = MockXMLHttpRequest

Whenever you do that, from then on, xhr will use MockXMLHttpRequest even if it was required earlier in some other module.

@heff
Copy link
Member Author

heff commented Sep 16, 2015

Ah great! Missed that option. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants