[MayaLekova]

modules: implement deferred import for dynamic modules

This PR adds support for deferring the import of modules in additional
cases, such as importing a dynamic module. It also adds a test for
this case.

Refs: https://github.com/tc39/proposal-defer-import-eval
Signed-off-by: Maya Lekova maya@igalia.com

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/security-wg
• @nodejs/startup
• @nodejs/typescript
• @nodejs/vm

[MayaLekova]

I tried fixing the cctest failures, but apparently the change to ModuleWrap::Evaluate signature also changed the behaviour of the VM module. Still experiencing this error:

(node:50255) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)
node:internal/vm/module:215
      validateObject(options, 'options');
      ^

TypeError [ERR_INVALID_ARG_TYPE]: The "options" argument must be of type object. Received type number (-1)
    at SourceTextModule.evaluate (node:internal/vm/module:215:7)
    at /home/maya/Work/node/out/Release/cctest:1:328
    at process.processTicksAndRejections (node:internal/process/task_queues:104:5) {
  code: 'ERR_INVALID_ARG_TYPE'
}

@joyeecheung just FYI, I'll keep trying to fix it.

[joyeecheung]

This is the public API for the vm API, which is not used by the built-in loader at all, and it's not the same as the module wrap's evaluate, which is internal and used by both. There's no need to touch this.

[MayaLekova]

Gotcha, I don't know why exactly it hits the changed version of ModuleWrap::Evaluate. Without it, the same test crashes, but with different error:

[ RUN      ] EnvironmentTest.EnvironmentWithESMLoader
(node:156113) ExperimentalWarning: VM Modules is an experimental feature and might change at any time
(Use `node --trace-warnings ...` to show where the warning was created)

  #  Node.js[156113]: static void node::loader::ModuleWrap::Evaluate(const v8::FunctionCallbackInfo<v8::Value>&) at ../../src/module_wrap.cc:789
  #  Assertion failed: (args.Length()) == (3)

----- Native stack trace -----

 1: 0x5eb63c38fce4  [out/Release/cctest]
 2: 0x5eb63c3909f9 node::Assert(node::AssertionInfo const&) [out/Release/cctest]
 3: 0x5eb63c30ddd4 node::loader::ModuleWrap::Evaluate(v8::FunctionCallbackInfo<v8::Value> const&) [out/Release/cctest]
 4: 0x785c5e58ffe9 

----- JavaScript stack trace -----

1: evaluate (node:internal/vm/module:233:26)
2: /home/maya/Work/node/out/Release/cctest:1:328
3: processTicksAndRejections (node:internal/process/task_queues:104:5)

Aborted (core dumped)

[joyeecheung]

That is the public API wrapper, I think you need to pass in kEvaluationPhase in

node/lib/internal/vm/module.js

Line 233 in 46dc5d7

return this[kWrap].evaluate(timeout, breakOnSigint);

- it will be a different matter to actually allow users to customize it in the API.

[MayaLekova]

Thanks, this was unintended to change the API.

[joyeecheung]

Can you drop the amaro update commit? I don't think that's supposed to be in this PR?

[MayaLekova]

Can you drop the amaro update commit? I don't think that's supposed to be in this PR?

Yep, done.

[MayaLekova]

Hi @joyeecheung , could you please take a look again? This is the first version, with the cctest failure before my change to the test.
It seems the CI didn't run this time, but I copied the failure in the comments. I'll apply another fix later today.

[codecov]

Codecov Report

❌ Patch coverage is 81.42857% with 13 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.72%. Comparing base (4096767) to head (21c9e24).
⚠️ Report is 339 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	68.57%	2 Missing and 9 partials ⚠️
lib/internal/modules/esm/module_job.js	92.30%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #66727      +/-   ##
==========================================
+ Coverage   90.50%   90.72%   +0.21%     
==========================================
  Files         797      798       +1     
  Lines      277652   280058    +2406     
  Branches    53345    53874     +529     
==========================================
+ Hits       251282   254071    +2789     
+ Misses      16758    16281     -477     
- Partials     9612     9706      +94     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/realm.js	96.73% <100.00%> (-0.37%)	⬇️
lib/internal/modules/esm/loader.js	99.90% <100.00%> (+<0.01%)	⬆️
lib/internal/modules/esm/utils.js	99.51% <100.00%> (+<0.01%)	⬆️
lib/internal/vm/module.js	96.35% <100.00%> (ø)
lib/internal/modules/esm/module_job.js	99.32% <92.30%> (-0.27%)	⬇️
src/module_wrap.cc	74.21% <68.57%> (-0.20%)	⬇️

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

[MayaLekova]

Hi @joyeecheung, the PR seems in a good shape already. Could you please take a look?

Also @guybedford, could you take a look as well? Just saw you were landing other module-related changes. This is wiring the import defer machinery for dynamically loaded modules.

Thank you both!

[caiolima]

Would it be a good idea add tests where the imported module on import.defer is a synthetic module, like CommonJS or Built-in modules? This will also be useful as documentation on how those modules would behave on such scenario.

[MayaLekova]

Would it be a good idea add tests where the imported module on import.defer is a synthetic module, like CommonJS or Built-in modules? This will also be useful as documentation on how those modules would behave on such scenario.

Thanks for the point! The synthetic modules code path is tested by this one:
https://github.com/nodejs/node/blob/main/test/es-module/test-defer-import-json-module.mjs

CommonJS and builtin modules seem to follow different paths:

• the builtin modules are still evaluated eagerly. Adding support for deferring them is more involved, as they don't follow a common scheme and I think it doesn't necesserily provide the same performance benefit.
• CJS have their own wrapper (a few, actually) and deferred evaluation for them was implemented earlier. Not sure exactly which PR adds it, the test is in:
  https://github.com/nodejs/node/blob/main/test/es-module/test-cjs-defer-static-import-eval.mjs

[MayaLekova]

Would it be a good idea add tests where the imported module on import.defer is a synthetic module, like CommonJS or Built-in modules? This will also be useful as documentation on how those modules would behave on such scenario.

There's test/esmodule/test-defer-import-builtin-module.mjs that does that. I think CJS modules need a special handling anyway.

[MayaLekova]

@joyeecheung I've updated the implementation and made the JS tests more robust. Still there's this cctest failing, which shouldn't have its execution path affected:
EnvironmentTest/SharedIsolateTest.EnvironmentsComeAndGoWhileSiblingsRun/SharedIsolateData

Node.js v27.0.0-pre
/home/maya/Work/node/out/Debug/cctest:19
if (process.argv.includes('--throw')) throw new Error('uncaught');
                                      ^

Error: uncaught
    at /home/maya/Work/node/out/Debug/cctest:19:45
    at embedderRunCjs (node:internal/main/embedding:97:10)
    at embedderRunEntryPoint (node:internal/main/embedding:175:12)

[joyeecheung]

LGTM % nits, thanks

[joyeecheung]

Was this not true before? I think we should assert that it was empty before accessing the namespace too.

[MayaLekova]

It used to be true, as the module-with-module-tree.mjs was adding one entry to the eval_list (its imported eagerly and its dependency, which modifies the eval_list is also imported eagerly). I changed the latter, so now the dependency is deferred, too. Thus moved the assertion to where it makes sense, after the module-deferred-eval module evaluation is triggered, after accessing deferred.foo.

Also, adding the check you suggested, thanks!

[nodejs-github-bot]

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