Skip to content

Conversation

@joyeecheung
Copy link
Member

@joyeecheung joyeecheung commented Oct 23, 2025

esm: use sync loading/resolving on non-loader-hook thread

ESM resolution and loading is now always synchronous from a
non-loader-hook thread. If no asynchrnous loader hooks are
registered, the resolution/loading is entirely synchronous.
If asynchronous loader hooks are registered, these would be
synchronous on the non-loader-hook thread, and asynchronous
on the loader hook thread.

This avoids several races caused by async/sync loading sharing
the same cache. In particular, asynchronous loader hooks
now works with require(esm) - previously it tends to break
due to races.

In addition, when an asynchronous loader hook
returns a promise that never settles, the main thread no longer
silently exits with exit code 13, leaving the code below
any module loading calls silently ignored without being executed.
Instead, it now throws ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED
which can be caught and handled by the main thread. If the module
request comes from import(), the never-settling promise is
now relayed to the result returned by import().

Drive-by: when annotating the error about importing undetectable
named exports from CommonJS, it now no longer reload the source
code of the CommonJS module, and instead reuses format information
cached when the module was loaded for linking.

Fixes: #59666

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/loaders
  • @nodejs/test_runner

@nodejs-github-bot nodejs-github-bot added lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run. labels Oct 23, 2025
ESM resolution and loading is now always synchronous from a
non-loader-hook thread. If no asynchrnous loader hooks are
registered, the resolution/loading is entirely synchronous.
If asynchronous loader hooks are registered, these would be
synchronous on the non-loader-hook thread, and asynchronous
on the loader hook thread.

This avoids several races caused by async/sync loading sharing
the same cache. In particular, asynchronous loader hooks
now works with `require(esm)` - previously it tends to break
due to races.

In addition, when an asynchronous loader hook
returns a promise that never settles, the main thread no longer
silently exits with exit code 13, leaving the code below
any module loading calls silently ignored without being executed.
Instead, it now throws ERR_ASYNC_LOADER_REQUEST_NEVER_SETTLED
which can be caught and handled by the main thread. If the module
request comes from `import()`, the never-settling promise is
now relayed to the result returned by `import()`.

Drive-by: when annotating the error about importing undetectable
named exports from CommonJS, it now no longer reload the source
code of the CommonJS module, and instead reuses format information
cached when the module was loaded for linking.
@codecov
Copy link

codecov bot commented Oct 23, 2025

Codecov Report

❌ Patch coverage is 94.57831% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 88.57%. Comparing base (b19525a) to head (f9deee5).
⚠️ Report is 3 commits behind head on main.

Files with missing lines Patch % Lines
lib/internal/modules/esm/loader.js 91.90% 14 Missing ⚠️
lib/internal/modules/esm/module_job.js 97.43% 3 Missing ⚠️
lib/internal/modules/esm/hooks.js 94.11% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #60380      +/-   ##
==========================================
- Coverage   88.57%   88.57%   -0.01%     
==========================================
  Files         704      704              
  Lines      208444   207753     -691     
  Branches    40057    40013      -44     
==========================================
- Hits       184627   184014     -613     
+ Misses      15848    15786      -62     
+ Partials     7969     7953      -16     
Files with missing lines Coverage Δ
lib/internal/errors.js 97.51% <100.00%> (+<0.01%) ⬆️
lib/internal/modules/esm/initialize_import_meta.js 100.00% <100.00%> (ø)
lib/internal/modules/esm/translators.js 92.93% <100.00%> (+0.03%) ⬆️
lib/internal/modules/esm/utils.js 100.00% <100.00%> (ø)
lib/internal/test_runner/mock/mock.js 98.72% <100.00%> (-0.01%) ⬇️
lib/internal/modules/esm/hooks.js 86.24% <94.11%> (-5.66%) ⬇️
lib/internal/modules/esm/module_job.js 96.59% <97.43%> (-0.18%) ⬇️
lib/internal/modules/esm/loader.js 96.84% <91.90%> (+1.05%) ⬆️

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

@GeoffreyBooth
Copy link
Member

I think this gets us a long way toward completing #55782? Like for example I think this means we can default to the ESM loader now if we want to (#50356). If so, this is a huge improvement!

Not to say that we necessarily want to switch the default loader anytime soon if ever, just that I assume we're no longer blocked from doing so by the async hooks tests.

@joyeecheung
Copy link
Member Author

joyeecheung commented Oct 23, 2025

Like for example I think this means we can default to the ESM loader now if we want to

I think that's a separate thing - the triggered hooks you see likely comes from the async activity from the outer layer of ModuleLoader.import, this PR only refactors some inner paths of it, it won't remove the async activity from the outer layer. There might also be other unwanted consequences of using the ESM loader to load a CJS entry point (e.g. overhead from lexing and creating a module facade, which is usually unnecessarily for an entrypoint, but needs to be done in case some other module comes back loading the entry point like a module, which is sometimes what people use require.main for)

@joyeecheung joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 23, 2025
@nodejs-github-bot
Copy link
Collaborator

@saico-0ps
Copy link

Thanks

@nodejs-github-bot
Copy link
Collaborator

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

lib / src Issues and PRs related to general changes in the lib or src directory. needs-ci PRs that need a full CI run.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Always run async loader hooks synchronously from the non-loader thread

4 participants