loading via AMD can break

[neonstalwart]

the way in which sinon is trying to load via AMD is prone to breaking - particularly when the 'A' (asynchronous) in AMD comes into play.

i was randomly seeing issues where the following line was throwing an error because returns was not a function:

var fn = sinon.stub().returns(value);

i started digging into the way sinon is loaded with AMD and i'm fairly sure there are multiple possible race conditions that could occur which could cause sinon to fail. i haven't done an exhaustive search but i'll share the 2 i was able to find after spending a few minutes inspecting the code. for reference, i'm looking at v1.10.3 (https://github.com/cjohansen/Sinon.JS/blob/v1.10.3/lib/sinon.js#L341-L354)

1. https://github.com/cjohansen/Sinon.JS/blob/v1.10.3/lib/sinon/spy.js#L409 is executed as soon as the script for spy.js has been injected. at that time, there is no guarantee that sinon.spyCall has been defined - call.js may not have arrived/executed by that time. fortunately, at least within sinon itself, it looks like everything refers to sinon.spyCall rather than sinon.spy.spyCall. maybe that line can be removed? alternatively, move the spy.spyCall assignment into sinon.js

   sinon.spyCall = sinon.spy.spyCall = require('./sinon/call');

2. https://github.com/cjohansen/Sinon.JS/blob/v1.10.3/lib/sinon/stub.js#L135-L149 is executed as soon as the script for stub.js has been injected. at that time, there is no guarantee that sinon.behavior has been defined - behavior.js may not have arrived/executed by the time the stub.js code is executed. this is in fact the issue i'm seeing.

i stopped looking at this point because i found what i was looking for but i would not be surprised to find more. a quick way to find more of these would be to look at each file and see what sinon.* properties it references in the path of code which is executed immediately when that script is loaded and if the property is not one of the ones defined in lib/sinon.js then those references will randomly fail due to the asynchronous nature of AMD.

[neonstalwart]

i went ahead and looked through the other files:

• https://github.com/cjohansen/Sinon.JS/blob/v1.10.3/lib/sinon/mock.js#L30 - sinon.match
• https://github.com/cjohansen/Sinon.JS/blob/v1.10.3/lib/sinon/sandbox.js#L63 - sinon.collection

[neonstalwart]

this passes all the tests except that the built version does not work. this is because of the odd simulation of AMD's define that is injected into the build. this simulated define makes the distribution unusable with a real AMD loader so maybe something can be done to change it so that only the samsam and formatio code finds that simulated define in scope and let the sinon code figure out if it's actually being loaded via AMD or not.

this.sinon = (function () {
var samsam, formatio;
// another closure to avoid affecting the sinon code
(function () {
    function define(mod, deps, fn) { if (mod == "samsam") { samsam = deps(); } else if (typeof fn === "function") { formatio = fn(samsam); } }
    define.amd = {};
     // <-- insert samsam here
     // <-- insert formatio here
})();
// <-- insert sinon here
return sinon;}.call(typeof window != 'undefined' && window || {}));

in fact, i'm going to add a commit to do that...

[neonstalwart]

ok, build works now

@cjohansen what do you think?

@mroderick this is what i believe it takes to get sinon working properly with AMD.

[neonstalwart]

hold on this for now... need to work through a few more things. will post back when it's ready to be looked at

[neonstalwart]

this should be ready again for consideration

[cjohansen]

Wow! Sorry to pitch in so late when you did so much work. Like I mentioned in another issue: We're planning to move Sinon to Browserify, using node require for internals, and the build a browser-friendly module with Browserify. Do you think all this is still worthwhile in that setting? I guess the goal is to use Sinon from source? Or are there more reasons for doing this?

Again, sorry for pitching in this so late - I've been on vacation.

[neonstalwart]

you may find you still need a large portion of this with browserify since this makes all the dependencies explicit and breaks some circular dependencies. i'm not exactly sure though. if someone tries browserify on the current master and there are issues, this branch might help in showing options to work through some of them.

[mroderick]

All the tests pass in both node and phantomjs on my machine. The code looks solid to me. This will be a valuable improvement as it fixes current issues and gives us breathing room to implement browserify at our own pace.

🚢

[mroderick]

Oh, and it needs a rebase :)

[mantoni]

Great work! This will definitly help with splitting Sinon up into smaller modules and browserifing it. This work goes further than my "hack" (see #358) and is better structured. 👍

[neonstalwart]

@mroderick i've rebased and force pushed. i think the only change that needed to be applied manually was 15a7e8e because that code moved to another file as part of my changes. please take a look and see if i missed anything. thanks.

[mroderick]

Looks fine to me

[cjohansen]

👍