-
Notifications
You must be signed in to change notification settings - Fork 30.9k
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: module.exports on CJS modules #57366
base: main
Are you sure you want to change the base?
Conversation
Review requested:
|
80e32fb
to
7c1bb9b
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #57366 +/- ##
=======================================
Coverage 90.20% 90.21%
=======================================
Files 630 630
Lines 185307 185311 +4
Branches 36269 36270 +1
=======================================
+ Hits 167162 167173 +11
- Misses 11084 11090 +6
+ Partials 7061 7048 -13
🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
CI seems to be fully passing here except for the ubuntu2204-armv7l runner which seems to have some problems starting up. Unless anyone objects will land anyway tomorrow. |
They're failing in the fanned job because they succeeded in https://ci.nodejs.org/job/node-test-pull-request/65639/ and the temporary binaries were deleted. Basically https://ci.nodejs.org/job/node-test-pull-request/65639 and https://ci.nodejs.org/job/node-test-pull-request/65640/ raced each other and now the only way to get a clean CI run would be to start a new one (not resume). |
Resolves #57295.
While the special
"module.exports"
name was defined for exporting from ESM modules for their representation in CommonJS, when writing a CommonJS with a"module.exports"
export name will cause a V8 error.The actual issue here is that we don't check if
module.exports
is already an export when defining the module wrap exports and as a result it is defined twice causing the failed V8 check.To avoid unnecessary work, we also update the code path to instead of settin this export and then override it later, just not setting it at all. To avoid unnecessary
hasOwnProperty
checks, these checks are moved earlier in the sequence as well.//cc @nodejs/loaders