esm: syncify default path of `ModuleLoader.load` by JakobJingleheimer · Pull Request #57419 · nodejs/node · GitHub

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

Parent issue: #55782

Review requested:

@nodejs/loaders

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 90.17%. Comparing base (6584d5f) to head (a31d408).
Report is 7 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #57419      +/-   ##
==========================================
- Coverage   90.18%   90.17%   -0.01%     
==========================================
  Files         636      636              
  Lines      188057   188032      -25     
  Branches    36900    36889      -11     
==========================================
- Hits       169596   169556      -40     
- Misses      11224    11231       +7     
- Partials     7237     7245       +8

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.37% <100.00%> (ø)

... and 34 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.

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

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

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.

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.

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.

lgtm

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

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

lgtm

Could I get a re-approval to unlock CI?

I think that this can be fully synchronous if it uses this.#customizations.loadSync below. But I am fine with leaving that to another PR.

Actually, isn't it only sync now?

Ack, nvmd. No.

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

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

Landed in 5e1537c

PR-URL: #57419 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Antoine du Hamel <duhamelantoine1995@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs/node#57419

* chore: bump node in DEPS to v22.18.0 * crypto: fix inclusion of OPENSSL_IS_BORINGSSL define nodejs/node#58845 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58960 * permission: propagate permission model flags on spawn nodejs/node#58853 * esm: syncify default path of ModuleLoader\.load nodejs/node#57419 * src: remove fast API for InternalModuleStat nodejs/node#58489 * src: simplify adding fast APIs to ExternalReferenceRegistry nodejs/node#58896 * chore: fixup patch indices * src: fix internalModuleStat v8 fast path nodejs/node#58054 * test: add tests to ensure that node.1 is kept in sync with cli.md nodejs/node#58878 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58942 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

chore: bump node to v22.18.0 (main) (#47937) * chore: bump node in DEPS to v22.18.0 * crypto: fix inclusion of OPENSSL_IS_BORINGSSL define nodejs/node#58845 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58960 * permission: propagate permission model flags on spawn nodejs/node#58853 * esm: syncify default path of ModuleLoader\.load nodejs/node#57419 * src: remove fast API for InternalModuleStat nodejs/node#58489 * src: simplify adding fast APIs to ExternalReferenceRegistry nodejs/node#58896 * chore: fixup patch indices * src: fix internalModuleStat v8 fast path nodejs/node#58054 * test: add tests to ensure that node.1 is kept in sync with cli.md nodejs/node#58878 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58942 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

* chore: bump node in DEPS to v22.18.0 * crypto: fix inclusion of OPENSSL_IS_BORINGSSL define nodejs/node#58845 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58960 * permission: propagate permission model flags on spawn nodejs/node#58853 * esm: syncify default path of ModuleLoader\.load nodejs/node#57419 * src: remove fast API for InternalModuleStat nodejs/node#58489 * src: simplify adding fast APIs to ExternalReferenceRegistry nodejs/node#58896 * chore: fixup patch indices * src: fix internalModuleStat v8 fast path nodejs/node#58054 * test: add tests to ensure that node.1 is kept in sync with cli.md nodejs/node#58878 * crypto: fix SHAKE128/256 breaking change introduced with OpenSSL 3.4 nodejs/node#58942 --------- Co-authored-by: electron-roller[bot] <84116207+electron-roller[bot]@users.noreply.github.com> Co-authored-by: Shelley Vohr <shelley.vohr@gmail.com>

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

Closed

Tracking Issue: Syncify the ESM Loader #55782

Open

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

Comment thread lib/internal/modules/esm/resolve.js Outdated

mcollina approved these changes Apr 7, 2025

View reviewed changes

aduh95 reviewed Apr 8, 2025

View reviewed changes

Comment thread lib/internal/modules/esm/resolve.js Outdated

joyeecheung reviewed Apr 8, 2025

View reviewed changes

Comment thread lib/internal/modules/esm/load.js Outdated

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

revert change to helper (detecting node: prefix)

a31d408

JakobJingleheimer force-pushed the esm/syncify-ModuleLoader-load branch from ab09afb to a31d408 Compare June 11, 2025 10:28

joyeecheung reviewed Jun 11, 2025

View reviewed changes

joyeecheung approved these changes Jun 11, 2025

View reviewed changes

JakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 11, 2025

nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Jun 11, 2025

This comment has been minimized.

Sign in to view

JakobJingleheimer added request-ci Add this label to start a Jenkins CI on a PR. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Jun 11, 2025

github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 11, 2025

JakobJingleheimer added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 11, 2025

nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 11, 2025

nodejs-github-bot merged commit 5e1537c into nodejs:main Jun 11, 2025

github-actions Bot mentioned this pull request Jun 16, 2025

2025-06-17, Version 24.3.0 (Current) #58727

Closed

This was referenced Jun 23, 2025

2025-06-24, Version 24.3.0 (Current) #58804

Closed

2025-06-24, Version 24.3.0 (Current) #58813

Merged

github-actions Bot mentioned this pull request Jul 28, 2025

2025-07-29, Version 22.18.0 'Jod' (LTS) #59256

Merged

codebytere added a commit to electron/electron that referenced this pull request Aug 4, 2025

esm: syncify default path of ModuleLoader\.load

9a56ed4

nodejs/node#57419

                  * @param {string} url The URL of the module to be loaded.
                  * @param {object} context Metadata about the module
-                 * @returns {Promise<{ format: ModuleFormat, source: ModuleSource }>}
+                 * @returns {Promise<{ format: ModuleFormat, source: ModuleSource }> | { format: ModuleFormat, source: ModuleSource }}

esm: syncify default path of `ModuleLoader.load` by JakobJingleheimer · Pull Request #57419 · nodejs/node · GitHub

Uh oh!

Uh oh!

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

esm: syncify default path of ModuleLoader.load#57419nodejs-github-bot merged 11 commits intonodejs:mainnodejs/node:mainfrom JakobJingleheimer:esm/syncify-ModuleLoader-loadJakobJingleheimer/node:esm/syncify-ModuleLoader-loadCopy head branch name to clipboard

Conversation

JakobJingleheimer commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nodejs-github-bot commented Mar 12, 2025

Uh oh!

codecov Bot commented Mar 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

marco-ippolito commented Mar 17, 2025

Uh oh!

JakobJingleheimer commented Mar 17, 2025

Uh oh!

JakobJingleheimer Mar 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

avivkeller Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Mar 22, 2025

Choose a reason for hiding this comment

Uh oh!

avivkeller commented Mar 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JakobJingleheimer commented Mar 22, 2025

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

nodejs-github-bot commented Apr 1, 2025

Uh oh!

github-actions Bot commented Apr 3, 2025

Uh oh!

Uh oh!

mcollina left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

JakobJingleheimer commented Jun 11, 2025

Uh oh!

joyeecheung Jun 11, 2025

Choose a reason for hiding this comment

Uh oh!

JakobJingleheimer Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

nodejs-github-bot commented Jun 11, 2025

Uh oh!

nodejs-github-bot commented Jun 11, 2025

Uh oh!

nodejs-github-bot commented Jun 11, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

esm: syncify default path of ModuleLoader.load#57419
nodejs-github-bot merged 11 commits into
nodejs:mainfrom
JakobJingleheimer:esm/syncify-ModuleLoader-load

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

JakobJingleheimer Jun 11, 2025 •
edited
Loading