-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
fix: support deep CJS re-exports when using ESM #13170
Conversation
@@ -792,9 +786,7 @@ export default class Runtime { | |||
const namedExports = new Set(exports); | |||
|
|||
reexports.forEach(reexport => { | |||
const resolved = this._resolveCjsModule(modulePath, reexport, { | |||
conditions: this.esmConditions, |
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 specifically was the bug - we passed ESM conditions when figuring out CJS require
s.
I moved all conditions into _resolveCjsModule
and _resolveModule
, that way this typo is way harder to make (and we always passed the same options anyways, no need for it to be an argument at all)
@@ -1,5 +1,8 @@ | |||
{ | |||
"type": "module", | |||
"devDependencies": { | |||
"discord.js": "14.3.0" |
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 can't be bothered to recreate this case, just installing the module which reproduced is easier 😅
Essentially: ESM imports CJS, which re-exports (via CJS) modules which use exports
to differentiate between ESM and CJS. Jest then picked out ESM when it should have picked out CJS
This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Summary
Fixes #12759
Test plan
Test added