-
Notifications
You must be signed in to change notification settings - Fork 154
Add support for mixed ESM and CJS via mixedModules
flag
#26
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
Conversation
@@ -1,3 +0,0 @@ | |||
[ | |||
"test/unit/require-esm/input.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 deleted this test because it was contrary to the new behavior. Instead I created a new test.
Codecov Report
@@ Coverage Diff @@
## master #26 +/- ##
==========================================
+ Coverage 89.88% 89.89% +0.01%
==========================================
Files 10 10
Lines 979 980 +1
==========================================
+ Hits 880 881 +1
Misses 99 99
Continue to review full report at Codecov.
|
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.
This is completely non-standard behavior and shouldn't be allowed. The provided user code in the referenced links is not valid JavaScript.
Mixing CommonJS isn't allowed by Webpack, Node, or and standards body.
I'm not in favor of allowing this, and I'm sorry that Next.js ever supported this.
This was a side-effect of Next.js fixing a pre-Babel-v7 bug.
Also, I don't believe ncc
ever supported this behavior. The only behavior ncc
should've supported was a ES Modules file requiring a CommonJS file, and vice-versa. Not mixing the two within a single file.
@Timer I just confirmed that
This change is necessary to maintain backwards compatibility. I would be open to using a flag to opt-in to this non-standard behavior if you prefer. |
Ah, this is because Looks like we might be stuck supporting this behavior until Node 12 is LTS with officially defined behavior. |
mixedModules
flag
@Timer I added a flag |
This adds support for mixing ESM and CJS so that we have feature parity with
ncc
, which is basicallywebpack
.This will fix all the issues that were reported in the latest release:
Update
I spoke with Timer and it sounds like Next.js might want to follow the Node.js behavior. If that's the case maybe we don't want this PR to land. We need to discuss with the whole team.
Perhaps we should enable backwards compatibility through a
config
flag likemixedModules: true
. That way user's who have broken deployments can opt-in to this behavior.