-
Notifications
You must be signed in to change notification settings - Fork 292
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
major: output ESM for .mjs or "type": "module" builds #720
Conversation
Codecov Report
@@ Coverage Diff @@
## main #720 +/- ##
==========================================
+ Coverage 73.52% 73.69% +0.16%
==========================================
Files 13 13
Lines 476 479 +3
==========================================
+ Hits 350 353 +3
Misses 126 126
Continue to review full report at Codecov.
|
I've rebased this PR to the webpack update at #723. |
f0db790
to
bade59e
Compare
To replicate, clone the repo and run:
This gives the error:
where the contents of that file are:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you try that?
This reverts commit 6a3991d.
Tests are failing here pending webpack bug webpack/webpack#13744. |
let startTime = Date.now(); | ||
let ps; | ||
const buildFile = eval("require.resolve")(resolve(args._[1] || ".")); | ||
const ext = buildFile.endsWith('.cjs') ? '.cjs' : '.js'; | ||
const esm = buildFile.endsWith('.mjs') || buildFile.endsWith('.js') && hasTypeModule(buildFile); | ||
const ext = buildFile.endsWith('.cjs') ? '.cjs' : esm && (buildFile.endsWith('.mjs') || !hasTypeModule(buildFile)) ? '.mjs' : '.js'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious why you didn't decide to preserve the extension of the input file?
I would expect this:
.cjs => .cjs
.mjs => .mjs
.js => .js
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Style we do keep the extension so that .cjs
-> .cjs
and .mjs
-> .mjs
. The issue with .js
is that it will be read by webpack as supporting both CJS and ESM, so isn't enough information on its own, hence the boundary check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, thanks! 🎉
Thanks @sokra for the review, updated. |
This adds support to ncc for a new
esm
option to output ES modules from the build. It defaults to true when the input file is ".mjs" or the input file is contained within a "type": "module" boundary.The support is based on Webpack's experimental support which seems to have a few edge cases that I have posted in webpack/webpack#13639.
If releasing this feature it would be best to treat it as having an experimental status just like the Webpack feature it exposes.
Resolves #679.
To be merged after #723 and vercel/webpack-asset-relocator-loader#140, #724.