modules: implement deferred import for dynamic modules

[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.

[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.39535% with 8 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.50%. Comparing base (4096767) to head (1ecb21a).
⚠️ Report is 35 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	63.63%	2 Missing and 6 partials ⚠️

Additional details and impacted files

@@           Coverage Diff            @@
##             main   #66727    +/-   ##
========================================
  Coverage   90.50%   90.50%            
========================================
  Files         797      797            
  Lines      277652   278244   +592     
  Branches    53345    53465   +120     
========================================
+ Hits       251282   251822   +540     
- Misses      16758    16799    +41     
- Partials     9612     9623    +11     

Files with missing lines	Coverage Δ
lib/internal/bootstrap/realm.js	97.50% <100.00%> (+0.40%)	⬆️
lib/internal/modules/esm/loader.js	99.90% <100.00%> (+<0.01%)	⬆️
lib/internal/modules/esm/module_job.js	99.58% <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%> (ø)
src/module_wrap.cc	74.20% <63.63%> (-0.21%)	⬇️

... and 62 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!