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]

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%)	⬇️

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));

[fatso83]

I think this is (mostly) fine, so thanks for doing it! There are just a couple of changes I would like changed, but they would require some input from the other maintainers:

1. Remove built bundles from Git. We are not doing this in Sinon, so I do not see why we ever started doing it here, even though it just updates once per release.
2. Put built bundles into some kind of distribution folder. In sinon we use pkg, but dist or something is also fine. It's fine that only NPM has these files.

What do you think, @sinonjs/nise-core?

[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).

[mantoni]

Does this make it a breaking change? Are people possibly depending on this? @fatso83

[43081j]

people could technically be importing the file by path, we could leave a file there for sole purpose of exporting the pkg/ one somehow i guess

[fatso83]

I think this is a breaking change, yes. On the other hand, I just merged another breaking change, so ... not an issue if both are released at the same time.

[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...).

[fatso83]

its also not really the most ideal browser module, its really a require-polyfilled CJS module being re-exported.

Not sure how an "ideal" browser module would look like? The "require-polyfilled CJS module being re-exported" is hard to avoid. It is, after all, a project consisting of a bunch of CJS modules, so you would get a single bundle no matter, along with a bunch of defined exports. Not sure what the effective difference would be? This just sounds like aesthetic concerns to me 🤷‍♂.

In any case, we do a lot of strange things in Sinon and the support libraries that break a lot of assumptions that most utils take for granted, having to support alternate globals, window in Node environments, etc for everyone to be happy, and that gets hairy. That we had issues with Rollup is not a huge surprise really, and I don't think Rollup would be to blame. It's just exotic stuff, basically. But I do not mind changing to Rollup or something else, but I do mind having to support lots of different builders and their configs, if I can do away with one for both test (mochify) and build, which is why (I assume) we removed Rollup when we could do without it.

For all I know, the issues we had with Rollup are already fixed with the work to extract global handling into something we control using @sinonjs/common/global, so it might be feasible to use it today.

[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: sinonjs/sinon#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

[mantoni]

👍 I think this is good to merge. @fatso83, since you mentioned that you have a new major release planned, I'll leave merging to you.

[SimenB]

can we test this using actual node esm instead of the esm package? It's unflagged on node 13 and 14, and will probably be unflagged in next minor of node 12 (nodejs/node#33055).

Just skip the tests on nodes that don't support it.

EDIT: I see this was discussed above - seems to work fine for me when testing locally?

[43081j]

its not the same. node's ESM implementation is still vastly different to the browser's ESM implementation because of all the interop cruft it has and limitations.

the esm module just happens to be a half way house.

really we should be testing it in node and the browser directly, to make sure both their module systems work fine with it. but its a task worthy of its own pr i think

[SimenB]

Node's implementation is at least spec compliant, using esm is just a hack with broken semantics.

I don't disagree it should be tested in real browsers, but if testing in Node it should be testing with a real implementation, not a some weird FrankenEsm. E.g. esm module still allows require, named import of CJS, omitting file extension, directory import etc which are all against spec. Having a test passing with esm is no proof ESM is implemented correctly.

It's stricter if you use an .mjs extension^*, but since we don't in this case that doesn't affect things.

FWIW, loading the generated file works fine with Node's built in ESM (as long as it's named .mjs)

^*) almost compliant, expect it allows directory imports and leaving out the extension (maybe more, didn't test)

[stale]

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: sinonjs/sinon#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.