Consider moving XHR/XDR aliases inside createXHR()

[dominykas]

Having these two lines outside of the main exporter function means, that the aliases are created before the module is required:

var XHR = window.XMLHttpRequest || noop
var XDR = "withCredentials" in (new XHR()) ?
        window.XMLHttpRequest : window.XDomainRequest

And since the aliases are already created, it is impossible to stub/mock XMLHttpRequest with e.g. http://sinonjs.org/docs/#server Considering that it is also impossible to fake out the exported function itself, it becomes practically impossible to properly unit test anything that uses the xhr module without having an actual HTTP server, which is not always feasible.

If the above two lines were inside createXHR() - we could hijack/fake XMLHttpRequest before calling the exported function. Aside from negligible performance impact, I don't really see any good reason not to do so - does one exist?

[Raynos]

unit testing is trivial

var xhr = require("xhr")

var myController = function (opts) {
  var request = opts.request || xhr

  return { fetchStuff: function () { ... } }
}

globally punching window.XMLHttpRequest is the wrong solution. However I added an option to pass an xhr instance in as an argument

xhr({ xhr: new window.XMLHttpRequest() })

[dominykas]

Ah now :) We could argue about where exactly the faking/mocking should happen for daaaays :) Just like about how much should the tests be integration vs unit...

I'm not sure that creating an alias of a global object, that one does not control, and behaves, essentially, like a private static property, is the right solution either (but I hate singletons in general).

I must admit, that I have a promise wrapper around the thing, so it will have to be tomorrow before I try it.

It would probably break semver, but passing in a constructor function, rather than the constructed object seems somewhat more appropriate to me right now, but I might change my mind in the morning :)

[Raynos]

Let me know if it works for you.

[dominykas]

Yeah, this does the trick, although I will end up basically re-implementing the whole "if cors xdr else xhr" thing and then still hijack the global XHR with sinon's fakeServer - creating a fake object and reimplementing the whole API just for tests would be too much of a hassle.

Any reasons not to move the aliases inside the exported function?

[Raynos]

They are my top level dependencies. Having them change out from underneath me is not something I want, whether for good (testing) or for bad (lol new magic xmlhttprequest).

It just seems like a complete hack.

[dominykas]

But you can't rely on them not having been changed before the module is even require()d... So, considering that you already assume that the XHR is the XHR (with correct interface, etc) - are there any other benefits the current approach brings?

[Raynos]

Closing this issue.

I think this is solved, if you have any other concerns let me know

[mmacaula]

I realize it is closed, but I'd like to add a +1 to this issue. Trying to test with jasmine-ajax on my end and it is very clunky to have to pass the stubbed XHR in just for testing. @dominykas is right, you can't be certain anyway that the XHR is the one you want if it's changed before it's required.
My two cents.

[naugtur]

You can pass xhr option to the request to give any implementation instead of XMLHttpRequest or XDR. [https://github.com/Raynos/xhr/blob/792c921988da1e34c6a5a227a5921f5f398ef2c3/index.js#L90]

Does this solve your issue?

[mmacaula]

So yes, technically I can do what you suggested, just as I'm sure the OP could. It just makes it difficult for testing. Just some background, I'm using the ampersand project which uses xhr as a dependency for fetching models. Normally I just call fetch and leave it up to the library to fetch my models using xhr. Great, now I'm trying to do some testing for it, just as the OP was. Ampersand allows me to pass my own XHR in, but its clunky to change all my production fetch code to take in an optional xhr object that is only used for testing.

Beyond my own problems though, I would think that this library would want to be compatible with major ajax testing libraries like jasmine-ajax and sinon and work out of the box. I understand the concern that it can be hacked and used improperly, but I'm not sure using anything that is passed in is better and doesn't really prevent hijacking from happening anyway if XHR is replaced before the library is required.

Regardless, I wanted to say thanks for all the work on the library!

[Raynos]

@mmacaula I disagree with that approach of testing.

Anything that manipulates globals is not something that will always work.

My best recommendation for you is to monkey patch globals first before importing the ampersand or xhr library.

[mmacaula]

That won't work for me unfortunately. The way jasmine ajax works, you monkey patch before your test, and reverse it afterwards and by then it's too late. I have other tests in the suite that have already required xhr.

So if that's out, would you be amenable to other approaches that could help me out? Say, providing a way to configure the library to use a different constructor rather than having to pass an instance? something like:

require('xhr').configure('XHRConstructor', defaultConstructor);  // or something else like a fake one?

Beyond the uses for testing, this could be helpful for globally configuring xhr to have certain fields defaulted.

[Raynos]

@mmacaula I think your suggestion is reasonable.

require('xhr').set({
  XHR: function () { ... },
  XDR: function () { ... }
})

[mmacaula]

Glad to hear it, would you like me to create a new issue for that?

[Raynos]

@mmacaula sure :)