Refactor experimental-modules check into runMain #21350
Conversation
|
its worth noting this isn't just a refactor, it actually fixes a bug where the loader can't run cjs. i can't quite figure out the repro though. edit: moving this code ensures that the loop never happens. /cc @guybedford could you add a test for this? |
|
@devsnek well spotted on the failure case. Note that the failure case also exposes another bug that duplicate import executions of module jobs don't seem to be properly deduped through |
|
(CI failure unrelated) |
test/es-module/test-esm-cjs-main.js
Outdated
| assert.strictEqual(require.main.exports.asdf, 'asdf'); | ||
| const path = require('path'); | ||
|
|
||
| const entry = path.resolve(__dirname, '../fixtures/es-modules/cjs.js'); |
There was a problem hiding this comment.
isn't there like common.fixture or smth for this
|
@guybedford is this ready to land? |
|
This is being reported as a regression in 10.5.0 already, see e.g. nodejs/help#1349 I’ve pushed a commit addressing @devsnek’s nit + adding a |
|
|
|
@devsnek it turns out because of this bug that test wasn't actually running to completion, and the file path was incorrect. I've resolved this here now. |
|
Merged in 883e1cd. |
PR-URL: #21350 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
PR-URL: #21350 Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Michaël Zasso <targos@protonmail.com> Reviewed-By: Gus Caplan <me@gus.host> Reviewed-By: Bradley Farias <bradley.meck@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de> Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
This provides a slightly cleaner separation, moving the top-level ESM load to higher up in the bootstrap as it should be.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passes