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

Allow global configuration of XHR and XDR constructors #60

Closed
mmacaula opened this issue Jan 1, 2015 · 9 comments
Closed

Allow global configuration of XHR and XDR constructors #60

mmacaula opened this issue Jan 1, 2015 · 9 comments

Comments

@mmacaula
Copy link

mmacaula commented Jan 1, 2015

Came from conversation in #11, it would be nice to be able to configure the constructors of the XHR and XDR. From @Raynos:

require('xhr').set({
  XHR: function () { ... },
  XDR: function () { ... }
})
@dominykas
Copy link
Contributor

Surely you'd also need a reset of some sort?

@Raynos
Copy link
Collaborator

Raynos commented Jan 2, 2015

Nope. your mutating the global state of the xhr library. It's your responsibility to set it back to the original state.

The use case for .set() is to overwrite the XHR constructor so that you can globally set some default headers or something.

@dominykas
Copy link
Contributor

To restore the original state, one must be aware of what the original state is. Since both XHR/XDR are private via closure, there is no way to know the original state. Both contain initialization logic (noop, withCredentials, etc), so either both need to be exposed in public or there is a need to be able to reset them (could be done by passing in null to set - although I'm not sure that's a good contract).

Ofc, a much simpler solution would be to not keep them as global state and to simply move the construction inside the exported method... But we've been through that discussion...

@naugtur
Copy link
Owner

naugtur commented Jan 2, 2015

That's why I'd propose this:

var xhr = require('xhr')
xhr.internals.XHR = yourXHRmock

And that would be compatible with all sorts of mocking libs, like sinon

var stub1=sinon.stub(xhr.internals,"XHR")
stub1.restore()

And btw, I checked ampersand-sync and their package.json says xhr: ^1.10. I'm going to make a PR to migrate to 2.0 during the weekend (hopefully)

@Raynos
Copy link
Collaborator

Raynos commented Jan 2, 2015

@naugtur makes a good suggestion.

I would recommend xhr.XHR and xhr.XDR though.

@naugtur
Copy link
Owner

naugtur commented Jan 5, 2015

I thought xhr.XHR would be confusing for what it actually means.

BTW. Didn't make the PR to ampersand yet, waiting for the discussion to go somewhere.

@Raynos
Copy link
Collaborator

Raynos commented Jan 5, 2015

we can do xhr.XMLHttpRequest or xhr.XDomainRequest :D thats less confusing.

@naugtur
Copy link
Owner

naugtur commented Jan 5, 2015

Ok, one more thing - why XDR at all? If you mock XHR, you will surely mock it with an implementation that is in line with standards and therefore XDR is not needed. Or am I missing something?

@Raynos
Copy link
Collaborator

Raynos commented Jan 5, 2015

that's upto the user to decide. He can mock both or one.

dmlap added a commit to dmlap/xhr that referenced this issue Jul 7, 2015
Fixes naugtur#60. Add a new method to the module object to override the constructors for XMLHttpRequest and XDomainRequest. Allows simpler integration with request mocking libraries like sinon.
dmlap added a commit to dmlap/xhr that referenced this issue Jul 8, 2015
Fixes naugtur#60. Add properties to the module object to override the constructors for XMLHttpRequest and XDomainRequest. Allows simpler integration with request mocking libraries like sinon.
dmlap added a commit to dmlap/xhr that referenced this issue Jul 8, 2015
Fixes naugtur#60. Add properties to the module object to override the constructors for XMLHttpRequest and XDomainRequest. Allows simpler integration with request mocking libraries like sinon.
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

No branches or pull requests

4 participants