module: link module with a module request record

[legendecas]

When a module is being statically linked with module requests, if two
module requests with a same specifier but different attributes are
resolved to two modules, the module requests should be linked to these
two modules.

For example,

import * as secret1 from 'a-json-package' with { type: 'json' };
import * as secret2 from 'a-json-package';

should result in two different module instances, if the second import
is been evaluated as a CommonJS/ESM module by a loader.

ECMA-262 requires that in HostLoadImportedModule, if the operation is called
multiple times with two (referrer, moduleRequest) pairs, it should return the same
result. But if the moduleRequest is different, and the module loader resolves to
different module instances, it should return different module instances.

Refs: https://tc39.es/ecma262/#sec-HostLoadImportedModule
Refs: https://github.com/tc39/proposal-import-attributes?tab=readme-ov-file#how-would-this-proposal-work-with-caching

[nodejs-github-bot]

Review requested:

• @nodejs/loaders
• @nodejs/vm

[codecov]

Codecov Report

❌ Patch coverage is 77.21519% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 90.08%. Comparing base (4d5ee24) to head (ed81a84).
⚠️ Report is 669 commits behind head on main.

Files with missing lines	Patch %	Lines
src/module_wrap.cc	69.38%	10 Missing and 5 partials ⚠️
src/module_wrap.h	72.72%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #58886      +/-   ##
==========================================
- Coverage   90.10%   90.08%   -0.03%     
==========================================
  Files         640      640              
  Lines      188493   188502       +9     
  Branches    36971    36977       +6     
==========================================
- Hits       169843   169805      -38     
- Misses      11358    11428      +70     
+ Partials     7292     7269      -23     

Files with missing lines	Coverage Δ
lib/internal/modules/esm/module_job.js	97.46% <100.00%> (-0.02%)	⬇️
lib/internal/vm/module.js	98.39% <100.00%> (-0.01%)	⬇️
src/module_wrap.h	44.44% <72.72%> (+44.44%)	⬆️
src/module_wrap.cc	73.15% <69.38%> (+0.40%)	⬆️

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

[nodejs-github-bot]

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

[aduh95]

But if the moduleRequest is different, it should return a different

That's not how I interpret the spec, AFAICT the spec says nothing about what the behavior should if they are different. If specifier and/or moduleRequest is different, the return value may or may not be different – e.g. import('fs') === import('node:fs').
Do we have a use case for supporting multiple modules with the same specifier?

[legendecas]

But if the moduleRequest is different, it should return a different

That's not how I interpret the spec, AFAICT the spec says nothing about what the behavior should if they are different. If specifier and/or moduleRequest is different, the return value may or may not be different – e.g. import('fs') === import('node:fs'). Do we have a use case for supporting multiple modules with the same specifier?

The spec didn't say the two specifier may be mapped to a single module.

The issue is that, if a loader resolves a single specifier with different import attributes, to two modules, the current implementation take the latter because the cache is keyed by specifier only.

If a loader interprets additional attributes, like:

import * as secret1 from 'a-json-package' with { type: 'json', integrity: 'frozen' };
import * as secret2 from 'a-json-package' with { type: 'json' };

This will be problematic because both namespaces secret1 and secret2 refer to the same mutable second resolution result. This is linked in https://github.com/tc39/proposal-import-attributes?tab=readme-ov-file#how-would-this-proposal-work-with-caching that the attributes should be part of the module cache key.

[guybedford]

This definitely better aligns with the specification, thanks for doing this work.

[guybedford]

I wasn't aware of a convention to import Module instead of module here. What makes Module different from other core libs here? Seems it should use the same casing unless there is a good reason.

[legendecas]

It's just my preference lasted from CJS that to prevent name collision with CJS module.

[joyeecheung]

In node:module, module.exports = Module, so the default export is also the Module constructor.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[legendecas]

@aduh95 @guybedford @joyeecheung updated PR to use hash+equality caching. PTAL again, thanks!

[nodejs-github-bot]

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

[joyeecheung]

I think we might still be able to avoid the extra string storage & transcoding with cppgc-managed wrappers; but that's probably a task for another PR.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[nodejs-github-bot]

Landed in cc856b3

[aduh95]

This doesn't land cleanly on v22.x-staging