Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enable esm named exports #2382

Merged
merged 2 commits into from
Nov 3, 2021

Conversation

perrin4869
Copy link
Contributor

@perrin4869 perrin4869 commented Jun 10, 2021

Adds support for esm named exports

Fixes issue #2361

I tried to keep this as backwards compatible as possible, but some minor (undocumented) changes were made which may break people importing from the pkg directory directly.
Specifically we can't do require('sinon/pkg/sinon.js') or require('sinon/pkg/sinon-no-sourcemaps.js') anymore, as these were changed into cjs files.
An additional pkg/sinon.js file is produced, identical to pkg/sinon.cjs, to keep the WebWorker functional. importScripts seems to fail on cjs files.

How to verify

  1. Check out this branch
  2. npm ci
  3. npm test
  4. import { stub } from "sinon" from an esm javascript file on node >= 12 should work

Edit: this is more of a band-aid, since the preferred solution would be an esm rewrite and using rollup to produce a cjs and esm build.

@codecov
Copy link

codecov bot commented Jun 10, 2021

Codecov Report

Merging #2382 (59aaaf6) into master (62b29c0) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #2382   +/-   ##
=======================================
  Coverage   96.07%   96.07%           
=======================================
  Files          41       41           
  Lines        1883     1883           
=======================================
  Hits         1809     1809           
  Misses         74       74           
Flag Coverage Δ
unit 96.07% <ø> (ø)

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


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 62b29c0...59aaaf6. Read the comment docs.

@fatso83
Copy link
Contributor

fatso83 commented Jun 10, 2021

Hi, Perrin. Thanks for this. As you probably saw in #2361 (comment), I had a go at this and found that, as you say, the best solution would just be to rewrite into ESM. I did have a go, but I found that the stuff I did always broke some kind of use case in some environment, so curious how this checks out.

Thanks!

@perrin4869
Copy link
Contributor Author

Honestly I was thinking that even if it's a band-aid, this PR would still require a major version bump just to be on the safe side.
That said, we could still tweak around the exports field to try to make it as backwards compatible as possible.
Dropping support for node 10 would make things slightly simpler I think, it's a good chance as it has reached EOL.
I tested this on node 12 through 16 loading as commonjs and esm, and got positive results, the rest of the use-cases I wouldn't really know... but I did make sure tests are passing...

@fatso83
Copy link
Contributor

fatso83 commented Jun 10, 2021

I was just thinking of these use cases:

  1. bundlers should not start having problems
  2. node in ESM mode (i.e. loading usign import in *.mjs scripts) should work
  3. node in require mode should work

Does the second point work with this? Or which envs work and how (i.e. which files need to be imported)?

@perrin4869
Copy link
Contributor Author

1 I haven't tested neither webpack not rollup, but in theory they should work
2 and 3 I tested the following way:

❯ cat package.json
{
  "type": "module",
  "dependencies": {
    "sinon": "github:perrin4869/sinon#esm-named-exports"
  }
}
❯ cat index.js
import sinon from "sinon"
import { stub } from "sinon"

console.log(sinon)
console.log(stub)
❯ cat index.cjs
const { stub } = require('sinon');
console.log(stub);

Node >= 12 behaved as you'd expect

@mroderick
Copy link
Member

Related to the discussion in this PR, I added some detail on converting the whole codebase to ESM: #2361 (comment)

@perrin4869
Copy link
Contributor Author

Well, while I can totally get being hesitant about merging this in, this does get the job done for me now, and I am not sure how realistic this esm rewrite really is...
Maybe we could release this as a beta for people to test?

@fatso83
Copy link
Contributor

fatso83 commented Jul 27, 2021

I think this just stalled on me (and or others) just forgetting about it pre-holiday. Not intentional from my side, in any way. I do not mind merging this (AFAI can remember).

@perrin4869
Copy link
Contributor Author

I'd love to see this merge as this is one of the biggest pain points for me moving to esm modules :)

@fatso83
Copy link
Contributor

fatso83 commented Nov 3, 2021

Released as Sinon 12! Thanks @perrin4869 <3

@fatso83
Copy link
Contributor

fatso83 commented Nov 4, 2021

@perrin4869 Seems this broke webpack bundling for CJS. See #2411. Any quickfix?

@perrin4869
Copy link
Contributor Author

ah dammit ><;; Let me look into this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants