add esm bundle

[43081j]

Fixes #317

I copied what you said... what sinon currently does.

IMO its pretty hacky, though, i'd recommend you look into just using rollup or something in both repos in a bigger PR.

ive tested it on a repo or two using bundlers and this does indeed fix the issue.

[codecov[bot]]

Codecov Report

Merging #320 (a6e9d3a) into master (d07b735) will decrease coverage by 0.16%. The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master     #320      +/-   ##
==========================================
- Coverage   93.15%   92.98%   -0.17%     
==========================================
  Files           1        2       +1     
  Lines         555      556       +1     
==========================================
  Hits          517      517              
- Misses         38       39       +1     

Flag	Coverage Δ
unit	92.98% <0.00%> (-0.17%)	:arrow_down:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
src/fake-timers-esm.js	0.00% <0.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d07b735...a6e9d3a. Read the comment docs.

[mantoni]

Wow, thank you for sending a PR for this so quickly! 🥇

Yes, I agree that this is pretty hacky, but at least we can control exactly what is going on. We tried using rollup first, but the result had other problems. I don't recall exactly what it was.

I don't feel comfortable merging this without any tests though. We have a test case in Sinon to test the esm module which is even more hacky 😆

https://github.com/sinonjs/sinon/blob/d2b4b726529f167d908747d2173b5491794ac718/test/es2015/check-esm-bundle-is-runnable.js

Maybe @fatso83 has an idea how we can generalize this? Otherwise, a copy of that test would also do.

[mantoni]

I've looked around some more and found another test case for esm that runs on Node:

https://github.com/sinonjs/sinon/blob/d2b4b726529f167d908747d2173b5491794ac718/test/es2015/module-support-assessment-test.es6

Both test cases are triggered using these npm scripts:

{
  "test-esm": "mocha -r esm test/es2015/module-support-assessment-test.es6",
  "test-esm-bundle": "node test/es2015/check-esm-bundle-is-runnable.js"
}

I don't know enough about the esm module being used there to help with the setup.

[43081j]

yeah im not sure its so simple.. we use sinon's own assertions too in tests so running in esm wouldn't be able to resolve that afaik

the test only needs to assert that the right exports exist, tbh..

also no clue whats up with the lint failing atm

[fatso83]

The two ESM related test suites are doing quite different things. The Puppeteer thing is verifying that the produced bundle is actually working when used directly by a ES Module supporting environment (with no esm, Webpack or other software intermingling). The other test suite is testing how Sinon behaves around ES Modules, as some parts of them are read-only and thus non-stubbable.

The Puppeteer script is indeed quite hairy, but for what it is supposed to do: test that a produced bundle is actually runnable in a browser, I think it should be quite easy to make usable for other use cases:

• Make the "test script" portion replaceable, basically reading in some test file.
• Parameterize the bundle name

So I could add a new @sinonjs/test-bundle util, or perhaps quicker: rework the existing module to work something like this (replacing process.exit with Promises):

const checkBundle = require('@sinon/runnable-esm');

checkBundle({
    testScript: './test-lolex-esm-exports.mjs', 
    bundlePath: os.path(__dirname, '../fake-timers-esm.js'
}).catch(err => process.exit(1));

[43081j]

i've rebased onto master and done the suggested changes.

the tests seem like something that, while it works in sinon, needs thinking about properly. so im not sure if we should pile it onto this PR (given i may not have the time, too). if there's a simpler way for us to assert the right exports exist in the built esm bundle, we should for now i think until we figure it out properly.

[fatso83]

This also works as a test for the exports:

test-package.js

import * as FakeTimers from "./pkg/fake-timers-esm.js";

let hasRun = false;
const org = setTimeout;
const clock = FakeTimers.install();
setTimeout(() => (hasRun = true));
clock.tick();

if (!hasRun) {
    console.error("Failed to tick timers in ES Module");
    process.exit(1);
}

node -r esm test-package.js 

Some variation of this could be worth adding to validate the exports? Maybe asserting the exported types are function? Was not able to use Node's built-in (experimental) ES Module support by renaming it to test-package.mjs, for some reason, so had to add esm.

---

On a different note, I exported our "test bundle in browser environment" script from Sinon into a separate project. That's for a different time, though (still struggling with a last issue).

[43081j]

its a weird one, because even node's esm implementation isn't the same as how browsers work. they do a lot of funky stuff to make commonjs interop work. ideally we'd want to run this test in an actual browser i think, maybe similar to your puppeteer idea but just a quick script it evaluates in headless chrome..

edit:

for now ill do the node esm test but look at some way to do the puppeteer thing nicely in future

[fatso83]

maybe similar to your puppeteer idea but just a quick script it evaluates in headless chrome..

@43081j That's what puppeteer does. It lets you run a quick script in headless chrome. If there is an easier way of loading and evaluating a quick script that avoids spinning up a headless server, then I am all for it.

[43081j]

yup i know, ill have a play around with it today if i can. a test which just evaluates a script to test the exports exist in browsers would do IMO. and an equivalent one in node with esm.

[43081j]

i added your test... it works but to do the puppeteer side of it gets sketchy really quickly.

basically, the thing we produce at the min isn't such a valid ES module for node in "esm mode". it'll complain about using require. its also not really the most ideal browser module, its really a require-polyfilled CJS module being re-exported.

what i really think should happen in future is move to rollup and output an ES module directly. you can have rollup also output a UMD bundle, if you must.

if you had problems last time you tried it, i really do recommend you try again and figure them out so we end up with less hackery going on (such as the misuse of a scoped variable in the esm sources to get it to work...).

[mantoni]

@fatso83 I looked it up in the history. It was as you say: We removed rollup to only have to deal with one bundler instead of two. Here is the original PR: https://github.com/sinonjs/sinon/pull/2127

[43081j]

It isn't really an aesthetic concern. Right now, we do some real questionable stuff:

• Assign the exports to what looks like a global variable
• This global variable, because of the source being injected into a bundle, becomes a scoped variable
• This allows the "ES module" to export that variable

which means we have the hacky looking esm file in src/ that fails any normal lint for that reason.

on top of that, we have a build script cobbling together bits of JS into the output file manually.

if we used rollup, that whole build script can go away and we can instead rely on an already well tested tool to output the umd and esm bundles. allowing us to specify a browser global, and to output a valid es module out of the box.

[43081j]

sorry @fatso83 totally missed this somehow.

added the missing file and rebased onto master

[mantoni]

The discussion about how be bundle for esm is a broader topic because it should be done the same in all Sinon projects. We can continue this in a separate issue.

For this PR to be merged, the builds need fixing. Otherwise this is looking good to me.

[43081j]

ive fixed the build. coverage is down because of the esm file now

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

[43081j]

@SimenB im having trouble finding time at the min to finish this off. especially if you want to use node's ESM implementation as new tests will need writing and a new test harness/setup.

ill catch the branch up but not sure when ill get chance to do those changes yet

[43081j]

i still don't really have the time to write a whole test suite for this but i do remember why i used esm now.

annoyingly, node made the decision to not allow you to specify what the type of an individual file is on the command-line (you can pass input-type=module for stdin but not for a file).

we could rename fake-timers-esm-test to fake-timers-esm-test.mjs but then it'd try import fake-timers-esm.js which node would wrongly assume is legacy commonjs again. we would have to rename it to .mjs

if you want to rework all the tests, i think you will have to do it especially since you understand how you want it more than i do. i'd be happy to quickly change it to use node's ESM at least but i don't know how, given the above.

[fatso83]

Hi, awakening this little thing. I recently ventured into similar territory in Sinon: https://github.com/sinonjs/sinon/issues/2361

The conclusion there, to produce ESMs usable by Node and bundlers alike, as well as non-esm projects:

To me, the easiest route forward seems to be to simply make this project use ESM all the way, set type: "module" and possibly be done with it (still producing the bundles in pkg/, though)

[luckylooke]

any updates? 😏

[43081j]

i haven't really had time still, but i think agree with @fatso83

i think we should just go all in on ESM (`type: "module") and use a bundler to produce a CJS entrypoint if we must (personally i'd just not ship one, i'd require legacy CJS users to use the old package)

my comments are no longer relevant, back then node esm was a bit sketchy. these days its pretty stable.

we still need some browser tests but it can be another PR

[fatso83]

@mroderick is currently working on converting all Sinon packages to ESM with a proper, non-hacky tool chain. Closing for the mentioned reasons above.