Modules lkgr rebase #53
Conversation
|
Took the liberty to squash some of the duplicated commits related to the loader revert. |
|
oh fun things are actually broken... going to dig in |
|
Fun indeed. Looks like it's - nodejs/node@b05fd4b, where the ESM init should happen after debug log. |
|
I've only glanced at it briefly, but something like moving this require - https://github.com/nodejs/node/blob/master/lib/internal/main/check_syntax.js#L14 so that it is inside of the function body after prepareMainThreadExecution() may help. |
MylesBorins
force-pushed
the
modules-lkgr-rebase
branch
2 times, most recently
from
eee6829 to
3be2e38
Compare
|
Took liberty to clean up the branch for upstreaming. 35 commits down to 7 (+1 for test fix). Did my best to split it up into observable bits. Still will need to go through it again and ensure that the test suite passes for each commit @guybedford @GeoffreyBooth @jdalton you all had commits that got squashed at some point. I've included all authors in commit meta data though. |
|
Looks like the CI build failed? |
|
@GeoffreyBooth looks like my solution introduced a "double require" which the linter is yelling about. Will need to refactor a bit, but going to compare against upstream first |
MylesBorins
force-pushed
the
modules-lkgr-rebase
branch
from
3be2e38 to
02bcd09
Compare
|
another try at CI: https://ci.nodejs.org/job/node-test-pull-request/21386/ |
| const vm = require('vm'); | ||
| const { | ||
| stripShebang, stripBOM | ||
| } = require('internal/modules/cjs/helpers'); | ||
|
|
||
| let CJSModule; | ||
| function CJSModuleInit() { | ||
| if (!CJSModule) |
There was a problem hiding this comment.
this guard is probably not necessary... but I'm being paranoid
MylesBorins
force-pushed
the
modules-lkgr-rebase
branch
from
02bcd09 to
a0b1964
Compare
Those three commits are basically: our Phase 0 work (remove stuff we’ll be replacing); add IRP; revert removal of loader code. I could see maybe the first and third being combined, but adding the IRP implementation feels like it should be its own commit. |
|
@GeoffreyBooth my thought process was that upstream there isn't necessarily any reason to keep this more granual, it is an artifact of our process. The loader revert is someone dependent on the types implementation in IRP... so it is a non-trivial amount of extra work to keep the tree clean. Keep in mind there is a non-zero chance all our work will be squashed into a single commit when landing on core. This is more about making review easier |
|
Sure, whatever you think is best. |
Refs: nodejs/modules#180 PR-URL: #6 PR-URL: #12 Co-authored-by: Myles Borins <[email protected]> Co-authored-by: John-David Dalton <[email protected]>
This reverts commit 1b0695b.
There are currently two supported values "explicit" and "node"
MylesBorins
force-pushed
the
modules-lkgr-rebase
branch
from
a0b1964 to
ea59221
Compare
|
landed in 2e2670a...ea59221 |
another day another conflict.