AMD: Built version from master is broken

[mroderick]

I've tried to create a pull request to verify that the API is working in AMD contexts, and it seems to be working fine in the source version.

The current master does not build Sinon in a way that works with RequireJS.

Steps to reproduce
https://gist.github.com/mroderick/82fcb7caa43aced684c8

Some of this was discussed in #553

@neonstalwart @cjohansen @mantoni I think this is all that is blocking us from doing a new release and drawing a line in the sand. I am too tired to wrap my head around this, if any of you could have a look that would be great.

[mroderick]

I've tried doing builds using r.js and requirejs dependent tools, but RequireJS doesn't include more than the top-level dependencies in a build, so that's not going to work.

[mroderick]

One crazy solution that would solve this issue and take us closer to 2.0 would be to move everything into one file. Then there would only be one place dealing with Vanilla/CommonJS/AMD, and we wouldn't have to merge anything.

This is not something that I would prefer, but it would fix the issue for the users in the short term, only the developers would pay the cost for awhile.

[neonstalwart]

i'm going to see if i've got a few minutes to look at this. the way you describe it sounds worse than i had expected - maybe we have different expectations. i don't think it needs to work to be built with r.js (but i had expected it should be able to be built) because since sinon is a utility to help with testing it's not typical to try to build it - i.e. using sinon from source is the way i'd expect everyone to be using it even if they are testing application code that has been built.

[neonstalwart]

The current master does not build Sinon in a way that works with RequireJS.

i expect that the current master should work - what's not working?

Steps to reproduce
https://gist.github.com/mroderick/82fcb7caa43aced684c8

your gist likely doesn't work for a number of reasons

1. you may need some configuration of the loader so that it knows where to find 'sinon'
2. you are not referencing the exports of sinon in the callback to require. instead, you are referencing a global variable called sinon which is intentionally not being set when using sinon as an AMD module. however, the reason you find something is because of things like this which are "leaking" a sinon global even when used via AMD.

the script block loading sinon might need to look something more like this:

require.config({
  packages: [
    { name: 'sinon', location: 'path/to/cjohansen/Sinon.JS', main: 'lib/sinon' }
  ]
});
require(['sinon'], function (sinon) {
    console.log('sinon: ', sinon);
});

i've created a live working demo at http://jsbin.com/keqihatozenu/1/edit that shows sinon from current master loading successfully via AMD from source.

[JHKennedy4]

Could you please clarify what is necessary to use Sinon with RequireJS? I downloaded the latest numbered release and have been attempting to require it as a typical AMD module without any success. I've attempted shimming the library and I've tried to replicate your example in my projects configuration file without success.

If there is configuration required, it should probably be documented somewhere in the README.

[neonstalwart]

using sinon via AMD should be possible in the next release. the latest release is not ideal (if it's even usable)

[JHKennedy4]

Is there a previous compatible release?
On Sep 8, 2014 6:20 PM, "Ben Hockey" notifications@github.com wrote:

using sinon via AMD should be possible in the next release. the latest
release is not ideal (if it's even usable)

—
Reply to this email directly or view it on GitHub
#555 (comment).

[neonstalwart]

I don't know. master should work. are you using a package manager to install sinon? you might be able to specify a specific commit to use.

[JHKennedy4]

I was installing the latest pre-built version from sinon.org. I'll just start working my way back releases until I find one that works.

[Avaq]

I was having a similar issue involving CommonJS after installing version 1.10.3 with Bower. I tracked the issue to a mistake made here.

Rather than eagerly loading via require, it should first be established whether sinon is already present, like done in all the other modules.

Copying the logic from one of the other modules and applying it there has solved all my CommonJS loading problems.

I hope this helps someone.

[zebulonj]

I believe I can add some insight into WHY the current release (1.10.3) is not working with Require.js. Line 38 of the built file defines a new define(...) function.

function define(mod, deps, fn) { if (mod == "samsam") { samsam = deps(); } else if (typeof fn === "function") { formatio = fn(samsam); } }

This doesn't overwrite the Require.js define(...) method, because it is contained in a closure; however, this local version of define(...) is what is called at line 971 to define the module in an AMD context.

if (isAMD) {
   define(makePublicAPI);
}

The Require.js define(...) method is never called.

[zebulonj]

@JHKennedy4 Not the ideal solution, but I'm using a Require.js shim configuration to make it work for now.

require.config({
    paths: {
        'sinon':    'lib/sinon-1.10.3'
    },

    shim: {
        'sinon': {
            exports: 'sinon'
        }
    }
});

[atesgoral]

I imagined the "lib" folder was the "source" folder and the "pkg" folder was meant to be the "distributable" folder. Is there a reason seemingly everyone is including Sinon straight from the "lib" folder?

[atesgoral]

I'm also actively trying to find a solution to the AMD loading issue(s). One thing I've noticed is that the makePublicAPI method in lib/sinon.js uses synchronous require() calls to bring in dependencies. Under AMD context, the module initialization should have probably looked something like this instead:

    if (isAMD) {
        define([
            "module",
            "./sinon/spy",
            "./sinon/call",
            "./sinon/behavior",
            "./sinon/stub",
            "./sinon/mock",
            "./sinon/collection",
            "./sinon/assert",
            "./sinon/sandbox",
            "./sinon/test",
            "./sinon/test_case",
            "./sinon/match"
        ], function (
            module,
            spy,
            call,
            behavior,
            stub,
            mock,
            collection,
            assert,
            sandbox,
            test,
            test_case,
            match
        ) {
            module.exports = sinon;
            sinon.spy = spy;
            sinon.spyCall = call;
            sinon.behavior = behavior;
            sinon.stub = stub;
            sinon.mock = mock;
            sinon.collection = collection;
            sinon.assert = assert;
            sinon.sandbox = sandbox;
            sinon.test = test;
            sinon.testCase = test_case;
            sinon.match = match;
        });
    } else if (isNode) {
        try {
            formatio = require("formatio");
        } catch (e) {}
        makePublicAPI(require, exports, module);
    }

Though, trying this on my node_modules copy locally didn't help much... Still investigating...

[atesgoral]

More context: I'm using Sinon with Intern. I have the option to use either Dojo or RequireJS as the module loader. The behaviour is slightly different, with Dojo seeming to fare better than RequireJS. The issue I'm having with Intern + Dojo + Sinon is sporadic. Everything works fine and tests pass, about 33% of the time. When things fail, it seems to be a result of a timing issue with the loading of Sinon submodules. I suspect this to be a result of the synchronous require() calls I mentioned above.

[neonstalwart]

Is there a reason seemingly everyone is including Sinon straight from the "lib" folder

with AMD, there should be no need to do a build for things to work. properly written AMD will work from source and, in addition, the build of sinon has been unusable with AMD for quite some time due to an internal define function that gets added to the build which shadows/masks the define from an AMD loader making it ineffective.

uses synchronous require() calls to bring in dependencies. Under AMD context, the module initialization should have probably looked something like this instead:

this is a misunderstanding on your part about how AMD works. the "synchronous" calls to require in makePublicAPI are actually interpreted by an AMD loader as part of what is sometimes referred to as the Simplified CommonJS wrapper and are not really synchronous despite the way it looks. yes, the call to require is synchronous but the resolution of those dependencies remains asynchronous. when you call define with no dependencies (i.e. the only argument you pass is a function) then the loader scans the body of that function (by means of Function.prototype.toString) looking for calls to require and creating an implicit list of dependencies to load before running the module's factory function.

as an example, the 2 snippets below are effectively the same:

define(['foo/bar'], function (bar) {
  // ...
});

define(function (require) {
    var bar = require('foo/bar');
    // ...
});

EDIT: although makePublicAPI was not breaking due to these "synchronous" requires there are other issues with makePublicAPI that are addressed as part of #523 in the loadDependencies functions which were added.

When things fail, it seems to be a result of a timing issue with the loading of Sinon submodules. I suspect this to be a result of the synchronous require() calls I mentioned above.

yes, this is what i have fixed in the current master of sinon - have you tried using master rather than the latest release? the cause of this issue is described in much more detail in #523 which is the PR that was merged to provide the necessary changes to get sinon working with an AMD loader.

[neonstalwart]

@atesgoral in short, try master (from the lib folder) and then let me know if any issues still exist - i believe they should have all been addressed.

[atesgoral]

@neonstalwart Awesome x 3 ! Will check master and report back.

P.S. I've been using AMD since it first came to be, both built & non-built, but I apparently have remained ignorant about the Simplified CommonJS wrapper feature all this time (since I've been only using the explicit dependency syntax).

[atesgoral]

@neonstalwart Confirmed to be working from master. I pointed my package.json to the latest commit hash on master and test runs seem to be stable now. Thanks! 🍰

[Munter]

Lovely. Would it be possible to do a new release with this fix?

[cjohansen]

Wohoo, well done @neonstalwart! Will cut a new release

[felixhammerl]

this is a breaking change for the pre-built version in an amd environment and yet, it only receives a minor version bump, which broke all of my CI builds ... bummer, man.

require.config({
    shim: {
        sinon: {
            exports: 'sinon',
        }
    }
});

is there a reason why you don't use node.js-syntax at development time and compile to UMD? AMD is no excuse for dragging along all of your dependencies at runtime and not using a single-file approach. Also, compiling to UMD would have fixed all of the issues mentioned here, including mine.

[Offirmo]

@felixhammerl same here, this broke tests in my company. We since locked sinon version to 1.10.3. What is UMD ?

[felixhammerl]

universal module definition
the umd example repo is maintained by jrburke himself (aka. the dude that created requirejs, volo, ...), along with addy osmani and other, so we those guys know what they are doing :) UMD is the 2014 way of saying: i don't give a damn about your runtime, just use this in whichever enviroment pleases you. which is kind of what sinon should be doing. the way i see it in the source files is that there is already soemthing vaguely resembling UMD, but in a somewhat weird way. so you can either write UMD headers, which can be tedious and you still have the problem what is what at runtime, or you can just write node.js-styled commonjs (which also has a waaaaaaay nicer syntax), build it and don't actually care where it is run. oh, and you'll probably get es6 modules thrown in for free when the time comes.

[cjohansen]

I'm sorry this is breaking stuff. sigh I'm so tired of all the accidental complexity in JavaScript module systems. Our plan is to develop Sinon "the node way" and build with browserify going forward. This was intended as a last release from the current source to help sort out some AMD issues.

If you have any pointers to how we can mitigate the breakage without regressing, I'd be grateful. Otherwise, I'm sorry, and all I can say is this will improve going forward.

[Offirmo]

@cjohansen sinon is awesome, and I agree js modularity sucks (for now). Thanks and keep up the good work !

[neonstalwart]

the need to use the shim configuration is a symptom of what was broken previously. now that it's fixed, you don't need to use shim any more - just use sinon like any other AMD module.

you're right that what's here now is similar to some of the UMD patterns but not quite exactly the same as any of them. my goal was to change the existing (broken) pattern as little as possible while moving to something which worked. as far as I know, if you use the shim config with a UMD pattern that does not expose a global when loaded via AMD then it would also have the same result you're seeing here. the shim config is for loading code which is not modularized.

again, the shim config you're using is evidence that sinon was broken previously (I have no doubt that even @jrburke will agree with me on this) and if you remove the shim config and still have issues then please mention me in any issues you raise and I'll be glad to help.

due to how broken this was previously it's unfortunately no surprise that whatever workarounds people had to use previously to get it working might now be broken by this fix. please, let me help you however I can.

thanks,

ben...

On Oct 29, 2014, at 04:28, Felix Hammerl notifications@github.com wrote:

this is a breaking change for the pre-built version in an amd environment and yet, it only receives a minor version bump, which broke all of my CI builds ... bummer, man.

require.config({
shim: {
sinon: {
exports: 'sinon',
}
}
});
is there a reason why you don't use node.js-syntax at development time and compile to UMD? AMD is no excuse for dragging along all of your dependencies at runtime and not using a single-file approach. Also, compiling to UMD would have fixed all of the issues mentioned here, including mine.

—
Reply to this email directly or view it on GitHub.

[cjohansen]

@neonstalwart thanks again for your patience and expertise in this issue!

[felixhammerl]

the easiest and least invasive way to fix the issue for me was just to throw the pre-built module in the global scope, remove it from the individual tests and go along my merry way.

@neonstalwart @cjohansen no flame intended :) this is absolutely awesome then! i do realize that people obviously found the weirdest ways to get it to work and that this fix obviously had to break some of them. i really really like sinon.js and use it in every single lib of http://emailjs.org/ and it's great to see that there's so much progress going on here 👍

[NickTomlin]

Edit:

This was due to the way the karma-requirejs plugin resolves file paths not sinon. The path of lib/sinon/test.jswas being rewritten which caused the strange failure below (as well as intermittent timeouts).

---

I'm using Sinon and require.js with Karma. I get the following error trying to require sinon:

  Uncaught TypeError: undefined is not a function
  at /Users/nicktomlin/workspace/pocketvim/node_modules/sinon/lib/sinon/util/fake_xml_http_request.js
:330

Sinon is defined within fake_xml_http_request but the extend method from from lib/sinon/extend.js has not be defined on Sinon. I'm not sure if this is a race condition with way require.js is resolving the dependencies.

I'm serving the entirety of sinon/lib and configuring require.js like so:

  packages: [
    { name: 'sinon', location: 'node_modules/sinon', main: 'lib/sinon' }
  ],

I'm unable to use the built file, since require.js is loaded in for my tests, and sinon (rightly) detects that it is in an AMD environment and tries to load all dependencies externally.

[jhnns]

Looking forward to the browserify-version. 👍
Until then sinon is unusable with webpack (which is similar to browserify) webpack/webpack#304

[mroderick]

I've created a test case that everyone should be able to run, that demonstrates this issue properly.

https://github.com/mroderick/sinon-555

As I understand it, the built Sinon.JS (from pkg and the website) loaded via AMD/RequireJS throws all sorts of errors. This might have always been the case.

I think that it's a reasonable expectation that Sinon.JS built version at least works with AMD/RequireJS (and other AMD loaders). I don't think that it's a reasonable expectation that source version also works with AMD/RequireJS.

Anyway, since it doesn't seem to have ever worked (at least not in a straightforward way without adding to the RequireJS config), I think we should park this issue for 1.x and make sure that it does work in 2.x (and that there's tests and clear documentation for doing so).

Opinions?

[neonstalwart]

#601 makes the test repo work. feel free to reformat the build script - i added some indentation to the output so that it was easier to debug.

[jhnns]

Just wanted to leave a note that the current sinon 2.0-branch is working perfectly with webpack. 👍

webpack/webpack#304

[kentcdodds]

@jhnns, thanks for the tip of using the 2.0 branch. Are you aware of whether this is still necessary?

[leftcoastgeek]

does anyone have the 1.x branch working with webpack? I'd like to use that branch since its actively being developed. I've not been able to get the npm or prebuilt version of sinon to work with webpack.