esm: syncify default path of ModuleLoader.load

[JakobJingleheimer]

In #57390 I somehow inadvertently created a branch on the node repo instead of my fork, and then got stuck.

Parent issue: #55782

[nodejs-github-bot]

Review requested:

• @nodejs/loaders

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.20%. Comparing base (a4c7c9f) to head (ab09afb).
Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57419      +/-   ##
==========================================
- Coverage   90.20%   90.20%   -0.01%     
==========================================
  Files         635      635              
  Lines      187616   187591      -25     
  Branches    36847    36848       +1     
==========================================
- Hits       169234   169209      -25     
- Misses      11150    11156       +6     
+ Partials     7232     7226       -6     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/load.js	94.31% <100.00%> (+2.36%)	⬆️
lib/internal/modules/esm/loader.js	96.35% <100.00%> (ø)
lib/internal/modules/helpers.js	98.84% <100.00%> (ø)

... and 38 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[marco-ippolito]

I investigated a bit on the weird failure and apparently there is some scheduling issue.
it can be patched with marco-ippolito@e0d702b#diff-b4c24f634e3741e5ad9e8c29864a48f2bd284a9d66d8fed3d077ccee0c44087bR968 but idk why it happens

[JakobJingleheimer]

Sweet, thanks! I was gonna take a look this evening. I'll apply it as soon as I finish work 🙂

[JakobJingleheimer]

I think this test's setup is the actual issue (I'm not sure why it was ever working 🤔):

• global-hooks.js contains a non-awaited dynamic import at the root: import('node:test').then((test) => …), which left a dangling promise (so node can't guarantee the global file has finished executing before starting with the entry-point). It was probably done that way because TLA is not available in CJS, and the file is shared by both test cases.
• The global hooks setup file --import/--require'd itself is indeed run before the test files (added console.log('\n==global hooks==\n') at line 1, and it prints first).

I suspect it was set up this way in order to re-use for both cjs and esm. @avivkeller it looks like you authored this test ~6 months ago. Would you perchance remember whether there was another reason? I can't think of one, but if there is, my change could inadvertently cause something to no-longer be covered.

Splitting global-hooks.js into global-hooks.cjs + require & global-hooks.mjs + static import, the sequence issue appears to be fixed.

However, a few of the global test hooks are now just not printing at all for the --require case (red lines are missing in actual vs expected):

before(): global
before one: <root>
suite one
before two: <root>
suite two
- beforeEach(): global
beforeEach one: suite one - test
beforeEach two: suite one - test
suite one - test
- afterEach(): global
afterEach one: suite one - test
afterEach two: suite one - test
before suite two: suite two
- beforeEach(): global
beforeEach one: suite two - test
beforeEach two: suite two - test
suite two - test
- afterEach(): global
afterEach one: suite two - test
afterEach two: suite two - test
- after(): global
after one: <root>
after two: <root>

[avivkeller]

Sorry I didn't see this earlier!

I suspect it was set up this way in order to re-use for both cjs and esm.

Exactly—using a single test file for both ESM and CJS helped keep things simpler, as I could simply flip-flop the --import vs --require call.

---

That’s definitely an odd issue you’re encountering. I have a few theories (though no concrete evidence to back them up):

• It’s possible that an error occurs in the chain for all but before, though that seems unlikely.
• Another possibility is a race condition. Could it be that before runs successfully simply because it’s the first one executed in the file?

[JakobJingleheimer]

Thanks!

Race condition seems the most likely candidate since this PR is affecting a/sync behaviour. Strange that only this test is failing though—I would have expected a bunch of failures.

[avivkeller]

PTAL @nodejs/test_runner

We're encountering an issue where a global hook script loaded via --require does not execute all hooks—only before runs—whereas loading it via --import works as expected.

@JakobJingleheimer and I investigated the root cause, but since I haven’t worked on the test runner in a while, we could use some help.

We've determined that the issue stems from the test runner's loading of CJS files. When modifying the following code in runner.js (used to load ESM files):

const userImports = getOptionValue('--import').concat(getOptionValue('--require'));
for (let i = 0; i < userImports.length; i++) {
  await cascadedLoader.import(userImports[i], parentURL, kEmptyObject);
}

(This would need primordials, but we are only testing right now)

If I run a test with a --require-d before and after hook, we observe:

• The before hook executes twice
• The after hook executes once

This suggests that the initial CJS load (which we haven’t yet pinpointed) runs and executes only the first before hook, while the cascadedLoader.import call correctly executes all hooks.

We need help identifying where the CJS load originally happens and whether we can remove it in favor of the snippet above or modify it to use the ESM loader.

[JakobJingleheimer]

thanks @avivkeller! The issue actually is actually present is a simpler change (merely the dangling promise fix) #57595 So that means it's unrelated to this change 😁 Let's move the discussion over to that PR.

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/65947/

[github-actions]

Failed to start CI

   ⚠  Commits were pushed since the last approving review:
   ⚠  - esm: syncify default path of `ModuleLoader.load`
   ⚠  - !fixup: don't force `ModuleLoader.load` to be async (can via hooks)
   ⚠  - !fixup: remove obsolete `getSource` in favour of `getSourceSync`
   ⚠  - !fixup: account for `node:internal` in `isMain` check
   ✘  Refusing to run CI on potentially unsafe PR

https://github.com/nodejs/node/actions/runs/14236594120

[mcollina]

lgtm

[nodejs-github-bot]

CI: https://ci.nodejs.org/job/node-test-pull-request/66204/

[JakobJingleheimer]

The VM issue is addressed by Chengzhong in #58637

I'm gonna back out all the changes related to it from this PR (already done), and rebase once that PR lands.