Use async AMD methods

[mroderick]

This pull request is under 🚧, things are likely to be broken. I am opening it now, to get feedback and discussion on the implmentation.

---

As @neonstalwart points out in #523, there's opportunities for the AMD loader to bite us, when loading dependencies asynchronously. This only happens when using a non-built version of Sinon, but I think it's worth fixing.

The calls to synchronous require should fail in compliant implementations, as described in the amdjs wiki, but clearly it doesn't in all implementations.

Anyway. The first commit for this pull request adds a method to define the api using AMD's define, and not attempt to use synchronous require (which is not guaranteed to work).

Thoughts? Shall I continue down this path, and make sure that all the dependency resolution is bulletproof?

[neonstalwart]

@mroderick those calls are all fine - changing them won't fix #523. those calls to require will work for any AMD loader that supports scanning the factory function (requirejs, dojo, cujojs/curl, ...). they are not the issue but thank you for opening this PR - it made me realize that f246450 has broken AMD.

the issue in #523 is that the sinon/lib/sinon/stub module should not rely on side-effects from sinon/lib/sinon but instead its own code should explicitly ensure its dependencies are fulfilled. each of the modules should really have their own equivalent of makePublicAPI that includes the calls to require for the dependencies which are currently (wrongly) assumed to synchronously exist due to side-effects from sinon/lib/sinon.

sinon seems to have a history of half-baked solutions for AMD and so by now i'm sure @cjohansen is probably tired of all the failed attempts to get it right. i'd like to help make it right - it will take some effort though. if all the dependencies for each module (including their dependency on sinon) are made explicit we'll see that there are currently some implicit circular dependencies but i believe these can be overcome. i'll try to find the time to work on this and see what issues i face.

[mroderick]

@neonstalwart you're welcome to add to this pull request.

This first commit is just the beginning, I am planning to go through all the places you highlighted in #523. Reviews of my commits would be very welcome, so we can fix this properly.

[neonstalwart]

@mroderick i don't think we actually need the changes in this PR. it just duplicates the list of dependencies (once for node and again for AMD) which makes it harder to maintain. i'll leave a comment here when i've got something that's started in the right direction. first thing is to revert fe586d4

[mroderick]

@neonstalwart Perhaps I need to re-read the require(string) part of the AMD spec again?

[neonstalwart]

yeah... from this part on

Dependencies can be found in an AMD module when this form of define() is used:

[neonstalwart]

@mroderick here's a branch where i'm working towards fixing #523 https://github.com/neonstalwart/Sinon.JS/compare/amd-refactor?w=1

it's probably going to touch all the files under lib since i'm making the dependency on sinon explicit

[cjohansen]

Just to throw this in: The plan is to move Sinon to use Browserify for packaging. I'm thinking we will make SInon a plain node module, and build a browser version with Browserify. This way we probably don't need to bother with AMD expect for in the final build?

[neonstalwart]

the standalone browserify option has worked well for other CJS libs i've consumed via AMD. that's probably not a bad idea since it would take care of CJS, AMD and script/global.

@cjohansen have you already figured out what it will take to get sinon working with browserify? i'm wondering how much of #523 might still be necessary for browserify to properly discover the proper dependencies. it's possible that #523 may be closer to what you need for browserify than what's currently in master - not sure though.

[cjohansen]

Interesting. I haven't really investigated yet. I know @mantoni did a POC a while back. So far I've started with extracting lolex. I'll keep these open for consideration as we move towards Browserify then. Thanks for your help.

[mroderick]

I vote we merge #523 and abandon this PR

[cjohansen]

Done!