Export Sinon as an ES module

[fatso83]

Purpose

Implements #1805 by using Rollup to export the common js bundle. Enables Sinon to be used in <script type="module> blocks without polluting the global namespace.

How to verify - mandatory

1. Check out this branch
2. npm link
3. cd /tmp; mkdir foo; cd foo; npm install esm; npm link sinon;
4. node -r esm -e 'import sinon from "sinon"; console.log("sinon.stub()", sinon.stub().returns(42)())'
5. Verify that it outputs: sinon.stub() 42

Checklist for author

• npm run lint passes

One might wonder if there is a need for having both browserify and rollup, but IMHO it adds little maintenance weight. We could get rid of one of them, but perhaps not in this PR.

[brettz9]

Just taking a quick look before getting a little time to test, but should the format be "es"? I don't see "esm" as an alias for format at https://rollupjs.org/guide/en

[fatso83]

Worked accidentally, I guess 😄 Still produced an EcmaScript module with export default sinon. Fixed in later commit. Thanks for noticing.

[fatso83]

What I suspect perhaps will cause trouble is the use of the os module ...

(!) Unresolved dependencies
https://github.com/rollup/rollup/wiki/Troubleshooting#treating-module-as-external-dependency
os (imported by node_modules/supports-color/index.js, commonjs-external:os)

[fatso83]

Not totally sure how to automate the last step of verifying in the browser, but I have made a manual test that shows we're not quite there yet ...

I added a index.html file in pkg with this content:

<script type="module">
    import sinon from '/sinon-esm.js';

    console.log('sinon is here', typeof sinon);
</script>

I then fired up a webserver on port 8000, and pointed chrome to localhost:8000. The remaining problem now is the module global keyword. Chrome chokes when encountering:

    module.exports = nextTick;

I guess this is due to the rollup-plugin-commonjs not handling the conditional nature of these exports:

if (!hasNextTick && !hasSetImmediate) {
    module.exports = nextTick;
}

[fatso83]

The modules are currently not possible to statically convert to esm. through some minimally invasive changes I think we should be able to make it work, though by hiding the runtime detection of what is exported behind a getter. Not doing this today, and for what I know, some of that logic might be in external dependencies (would have to manually inspect - no time today). If people could delve into this, it would be great.

[mroderick]

This is an exciting PR to follow!

[fatso83]

For some reason - and I have no idea why - the bug with the unresolved export seems to have vanished. I have tried the bundle in both Chrome and Firefox.

My only "problem" is that I haven't been able to automatically create a verification step, as I can't find a way of making Puppeteer verify that a test script has been run. But manual inspection seems to indicate this is working.

[mroderick]

That is what I am seeing too:

$ node test/es2015/test-module-is-runnable.js
Starting browser ...
server responded with sinon module
Failed:  Error: waiting for function failed: timeout 5000ms exceeded
    at Timeout.WaitTask._timeoutTimer.setTimeout (/Users/mroderick/code/sinonjs/sinon/node_modules/puppeteer/lib/FrameManager.js:844:60)
    at ontimeout (timers.js:475:11)
    at tryOnTimeout (timers.js:310:5)
    at Timer.listOnTimeout (timers.js:270:5)

[fatso83]

That my non-working script didn't work was already established 😆 Everything seems to work when testing with a browser, though, so in a way it's complete, but I am not comfortable merging this until we have an automatic verification step. I resorted to trying StackOverflow ... Let's see how that pans out.

[mantoni]

@fatso83 Can you share the test file you're using? It's not in this PR ... or am I overlooking something?

[fatso83]

@mantoni I didn't commit the test file, as it wasn't working, but it's the gist I linked to. It should pretty much just be

curl https://gist.githubusercontent.com/fatso83/b5d7d3aa366fc4e5d6b622a5b8c6c338/raw/2d62279219ad9f57ee0db6bf651428ae48fbb72b/test-module-is-runnable.js > test/es2015/test-module-is-runnable.js; 
cd test/es2015; 
node test-module-is-runnable.js

[mantoni]

@fatso83 Ah, ok. Maybe you want to try evaluate like mochify does this:

https://github.com/mantoni/mochify.js/blob/e993f8b37dfb1c27f9ca8dcd91ab450b127e5a95/lib/chromium.js#L155

[fatso83]

@mantoni That wouldn't work, AFAIK (and I already tried). ES Modules are async and non-blocking. If you evaluate immediately after loading the page the script hasn't had time to load the Sinon module.

Also, I started out with an evaluate call: a setTimeout wrapped in a Promise, but on timeout it still couldn't find any changed globals. Doing the waitForFunction is essentially that (or, more like setInterval with a timeout). On the other hand, if I start a real Chrome instance and point to the server I can evaluate manually in the console almost immediately after page load and see that window.sinonLoaded is defined.

[mantoni]

Yeah, I evaluate the script synchronously and then monitor the console events from the page. This way I get notified asynchronously without polling.

[fatso83]

@mantoni Thanks for the tip on console.log. In the end, it was this that made me realize I was missing the right mime type on the resources provided by the http server, making the browser refuse to execute the contents of the module.

I have now implemented a check that does a simple test of the stubbing functionality. It works and builds as expected, meaning this should be ready for review 😃

[mroderick]

These tests are failing in older node

[fatso83]

💥

✔️ Travis builds passing

[mantoni]

👍