esm: syncify default path of `ModuleLoader.load` #57419

JakobJingleheimer · 2025-03-12T00:59:39Z

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 · 2025-03-12T00:59:43Z

Review requested:

@nodejs/loaders

codecov · 2025-03-12T02:09:15Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.21%. Comparing base (5a2614f) to head (558dfff).
Report is 82 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57419      +/-   ##
==========================================
- Coverage   90.23%   90.21%   -0.02%     
==========================================
  Files         630      630              
  Lines      185074   185502     +428     
  Branches    36221    36379     +158     
==========================================
+ Hits       166994   167350     +356     
  Misses      11036    11036              
- Partials     7044     7116      +72

Files with missing lines	Coverage Δ
lib/internal/modules/esm/load.js	`94.31% <100.00%> (+2.36%)`	⬆️
lib/internal/modules/esm/loader.js	`95.04% <100.00%> (ø)`
lib/internal/modules/esm/utils.js	`98.92% <100.00%> (+<0.01%)`	⬆️

... and 84 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 · 2025-03-17T14:26:05Z

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 · 2025-03-17T14:28:21Z

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

JakobJingleheimer · 2025-03-19T09:32:49Z

test/parallel/test-runner-no-isolation-hooks.mjs

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>

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?

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 · 2025-03-22T22:01:44Z

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 · 2025-03-22T23:00:36Z

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 · 2025-04-01T20:24:37Z

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

Co-Authored-By: Marco Ippolito <[email protected]>

github-actions · 2025-04-03T06:40:15Z

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

aduh95 · 2025-04-03T14:40:21Z

lib/internal/modules/esm/resolve.js

@@ -965,7 +965,7 @@ function defaultResolve(specifier, context = {}) {
  if (protocol === 'node:') { return { __proto__: null, url: specifier }; }


-  const isMain = parentURL === undefined;
+  const isMain = parentURL === undefined || StringPrototypeStartsWith(parentURL, 'node:internal/');


Could you add a comment explaining why we want isMain to be true for internal modules?

It came from here: #57419 (comment)

TLDR: we don't know.

What is the weird failure exactly? This addition does look strange, or could be painting over a bug...

Ahh, this looks familiar.

https://github.com/nodejs/node/actions/runs/14280143011/job/40028680516?pr=57419#step:11:5939

Path: es-module/test-vm-main-context-default-loader Error: --- stderr --- (node:66638) ExperimentalWarning: vm.USE_MAIN_CONTEXT_DEFAULT_LOADER is an experimental feature and might change at any time (Use `node --trace-warnings ...` to show where the warning was created) node:internal/process/promises:394 triggerUncaughtException(err, true /* fromPromise */); ^ AssertionError [ERR_ASSERTION]: function should not have been called at /Users/runner/work/node/node/node/test/es-module/test-vm-main-context-default-loader.js:146 called with arguments: AssertionError [ERR_ASSERTION]: Expected values to be strictly deep-equal: + actual - expected Comparison { + code: 'ERR_UNSUPPORTED_RESOLVE_REQUEST' - code: 'ERR_MODULE_NOT_FOUND' } at process.processTicksAndRejections (node:internal/process/task_queues:105:5) at async main (/Users/runner/work/node/node/node/test/es-module/test-vm-main-context-default-loader.js:124:7) { generatedMessage: true, code: 'ERR_ASSERTION', actual: [TypeError], expected: [Object], operator: 'rejects' } at mustNotCall (/Users/runner/work/node/node/node/test/common/index.js:493:12) at process.processTicksAndRejections (node:internal/process/task_queues:105:5) { generatedMessage: false, code: 'ERR_ASSERTION', actual: undefined, expected: undefined, operator: 'fail' }

@joyeecheung does this look like anything to you? I'm not familiar with the VM module.

I tried to look into this a bit, but the test is written for someone with deep understanding of what is happening, and I unfortunately don't. And there is no stack-trace to follow.

node/test/es-module/test-vm-main-context-default-loader.js

Lines 115 to 125 in 86bfdb5

// createContext() with null referrer also resolves to cwd.

{

const options = {

importModuleDynamically: USE_MAIN_CONTEXT_DEFAULT_LOADER,

};

const ctx = createContext({}, options);

const s = new Script('Promise.resolve("import(\'./message.mjs\')").then(eval)', {

importModuleDynamically: common.mustNotCall(),

});

await assert.rejects(s.runInContext(ctx), { code: 'ERR_MODULE_NOT_FOUND' });

}

If there is a bug being papered over, I'm pretty sure it is not caused by this change (but is perhaps uncovered by it). In which case, it should be fixed in a different PR; and since it already exists, it shouldn't be a blocker here.

This reverts commit f60650f.

mcollina

lgtm

This reverts commit 76c93d5.

lib/internal/modules/esm/resolve.js

joyeecheung · 2025-04-08T14:49:11Z

lib/internal/modules/esm/load.js

      context = { __proto__: context, source };
    }

    if (format == null) {
      // Now that we have the source for the module, run `defaultGetFormat` to detect its format.
-      format = await defaultGetFormat(urlInstance, context);
+      format = defaultGetFormat(urlInstance, context);

      if (format === 'commonjs') {


I guess it might happen from the branch here and the "reset source to null and then resolve twice - the second time resolving a full path - if it's commonjs" quirk of the sync loading path. Which might mean that this sync version may not be used as-is without getting rid of that quirk or patched here somehow to not trigger that quirk.

🤔 what makes you think it's that quirk? and why is it only exposed when going through that specific VM scenario?

nodejs-github-bot added esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. labels Mar 12, 2025

JakobJingleheimer mentioned this pull request Mar 12, 2025

esm: synchronise default path of ModuleLoader.load #57390

Closed

JakobJingleheimer force-pushed the esm/syncify-ModuleLoader-load branch from 0bcfae3 to e7941fe Compare March 19, 2025 08:57

JakobJingleheimer commented Mar 19, 2025

View reviewed changes

JakobJingleheimer mentioned this pull request Mar 22, 2025

test: fix dangling promise in test_runner no isolation test setup #57595

Merged

mcollina approved these changes Apr 1, 2025

View reviewed changes

mcollina added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Apr 1, 2025

This was referenced Apr 2, 2025

test_runner: most global hooks set up via --require don't trigger #57728

Open

Tracking Issue: Syncify the ESM Loader #55782

Open

JakobJingleheimer and others added 4 commits April 2, 2025 23:13

esm: syncify default path of ModuleLoader.load

3a0882a

!fixup: don't force ModuleLoader.load to be async (can via hooks)

d633804

!fixup: remove obsolete getSource in favour of getSourceSync

4276af1

!fixup: account for node:internal in isMain check

f60650f

Co-Authored-By: Marco Ippolito <[email protected]>

JakobJingleheimer force-pushed the esm/syncify-ModuleLoader-load branch from a2d0fcc to f60650f Compare April 2, 2025 21:14

JakobJingleheimer added the request-ci Add this label to start a Jenkins CI on a PR. label Apr 3, 2025

github-actions bot added request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. and removed request-ci Add this label to start a Jenkins CI on a PR. labels Apr 3, 2025

JakobJingleheimer requested a review from mcollina April 3, 2025 06:48

aduh95 reviewed Apr 3, 2025

View reviewed changes

Revert "!fixup: account for node:internal in isMain check"

76c93d5

This reverts commit f60650f.

mcollina approved these changes Apr 7, 2025

View reviewed changes

JakobJingleheimer added 2 commits April 8, 2025 12:25

Reapply "!fixup: account for node:internal in isMain check"

b697946

This reverts commit 76c93d5.

fixup!: add todo & note about mysterious fix/failure

8f93801

aduh95 reviewed Apr 8, 2025

View reviewed changes

lib/internal/modules/esm/resolve.js Outdated Show resolved Hide resolved

joyeecheung reviewed Apr 8, 2025

View reviewed changes

fixup!: move isInternal check to specific vm resolve path

558dfff

JakobJingleheimer requested a review from aduh95 April 10, 2025 10:10

JakobJingleheimer removed the request-ci-failed An error occurred while starting CI via request-ci label, and manual interventon is needed. label Apr 10, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

esm: syncify default path of `ModuleLoader.load` #57419

esm: syncify default path of `ModuleLoader.load` #57419

JakobJingleheimer commented Mar 12, 2025 •

edited

Loading

nodejs-github-bot commented Mar 12, 2025

codecov bot commented Mar 12, 2025 •

edited

Loading

marco-ippolito commented Mar 17, 2025

JakobJingleheimer commented Mar 17, 2025

JakobJingleheimer Mar 19, 2025 •

edited

Loading

avivkeller Mar 22, 2025 •

edited

Loading

JakobJingleheimer Mar 22, 2025

avivkeller commented Mar 22, 2025 •

edited

Loading

JakobJingleheimer commented Mar 22, 2025

mcollina left a comment

nodejs-github-bot commented Apr 1, 2025

github-actions bot commented Apr 3, 2025

aduh95 Apr 3, 2025

JakobJingleheimer Apr 3, 2025

joyeecheung Apr 3, 2025

This comment was marked as outdated.

JakobJingleheimer Apr 5, 2025

JakobJingleheimer Apr 7, 2025

JakobJingleheimer Apr 8, 2025

mcollina left a comment

joyeecheung Apr 8, 2025 •

edited

Loading

JakobJingleheimer Apr 9, 2025

	// createContext() with null referrer also resolves to cwd.
	{
	const options = {
	importModuleDynamically: USE_MAIN_CONTEXT_DEFAULT_LOADER,
	};
	const ctx = createContext({}, options);
	const s = new Script('Promise.resolve("import(\'./message.mjs\')").then(eval)', {
	importModuleDynamically: common.mustNotCall(),
	});
	await assert.rejects(s.runInContext(ctx), { code: 'ERR_MODULE_NOT_FOUND' });
	}

esm: syncify default path of ModuleLoader.load #57419

Are you sure you want to change the base?

esm: syncify default path of ModuleLoader.load #57419

Conversation

JakobJingleheimer commented Mar 12, 2025 • edited Loading

nodejs-github-bot commented Mar 12, 2025

codecov bot commented Mar 12, 2025 • edited Loading

Codecov Report

marco-ippolito commented Mar 17, 2025

JakobJingleheimer commented Mar 17, 2025

JakobJingleheimer Mar 19, 2025 • edited Loading

Choose a reason for hiding this comment

avivkeller Mar 22, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avivkeller commented Mar 22, 2025 • edited Loading

JakobJingleheimer commented Mar 22, 2025

mcollina left a comment

Choose a reason for hiding this comment

nodejs-github-bot commented Apr 1, 2025

github-actions bot commented Apr 3, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mcollina left a comment

Choose a reason for hiding this comment

joyeecheung Apr 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

esm: syncify default path of `ModuleLoader.load` #57419

esm: syncify default path of `ModuleLoader.load` #57419

JakobJingleheimer commented Mar 12, 2025 •

edited

Loading

codecov bot commented Mar 12, 2025 •

edited

Loading

JakobJingleheimer Mar 19, 2025 •

edited

Loading

avivkeller Mar 22, 2025 •

edited

Loading

avivkeller commented Mar 22, 2025 •

edited

Loading

joyeecheung Apr 8, 2025 •

edited

Loading