module: link module with a module request record by legendecas · Pull Request #58886 · nodejs/node · GitHub

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

Review requested:

@nodejs/loaders
@nodejs/vm

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.

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

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?

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.

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

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.

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

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

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

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

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

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

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

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.

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

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.

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

Landed in cc856b3

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. PR-URL: #58886 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 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

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

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. PR-URL: #58886 Backport-PR-URL: #60000 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 Reviewed-By: Guy Bedford <guybedford@gmail.com> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. esm Issues and PRs related to the ECMAScript Modules implementation. needs-ci PRs that need a full CI run. vm Issues and PRs related to the vm subsystem. labels Jun 29, 2025

legendecas force-pushed the module-link-requests branch 4 times, most recently from a8faeb0 to 5ea8994 Compare June 29, 2025 17:55

joyeecheung reviewed Jun 30, 2025

View reviewed changes

Comment thread src/module_wrap.h Outdated

joyeecheung reviewed Jun 30, 2025

View reviewed changes

Comment thread src/module_wrap.cc Outdated

guybedford reviewed Jun 30, 2025

View reviewed changes

legendecas force-pushed the module-link-requests branch 2 times, most recently from 6a8b7e7 to 1abf30b Compare June 30, 2025 23:20

joyeecheung reviewed Jul 1, 2025

View reviewed changes

Comment thread src/module_wrap.cc Outdated

Comment thread src/module_wrap.h Outdated

legendecas force-pushed the module-link-requests branch from 1abf30b to f7185b1 Compare July 2, 2025 10:43

guybedford reviewed Jul 2, 2025

View reviewed changes

Comment thread src/module_wrap.h Outdated

legendecas force-pushed the module-link-requests branch from f7185b1 to 008345a Compare July 3, 2025 09:09

guybedford approved these changes Jul 3, 2025

View reviewed changes

Comment thread src/module_wrap.cc Outdated

joyeecheung approved these changes Jul 3, 2025

View reviewed changes

joyeecheung added the request-ci Add this label to start a Jenkins CI on a PR. label Jul 3, 2025

guybedford reviewed Jul 3, 2025

View reviewed changes

Comment thread src/module_wrap.cc Outdated

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

module: link module with a module request record

ed81a84

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.

legendecas force-pushed the module-link-requests branch from 008345a to ed81a84 Compare July 4, 2025 08:57

guybedford approved these changes Jul 4, 2025

View reviewed changes

legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Jul 4, 2025

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

nodejs-github-bot merged commit cc856b3 into nodejs:main Jul 4, 2025

legendecas deleted the module-link-requests branch July 4, 2025 19:57

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

2025-07-09, Version 24.4.0 (Current) #58993

Merged

aduh95 added the backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. label Jul 21, 2025

legendecas mentioned this pull request Sep 24, 2025

[v22.x backport] module: link module with a module request record #60000

Merged

legendecas added backport-open-v22.x Indicate that the PR has an open backport and removed backport-requested-v22.x PRs awaiting manual backport to the v22.x-staging branch. labels Sep 24, 2025

aduh95 added backported-to-v22.x PRs backported to the v22.x-staging branch. and removed backport-open-v22.x Indicate that the PR has an open backport labels Oct 7, 2025

github-actions Bot mentioned this pull request Oct 12, 2025

2025-10-20, Version 22.21.0 'Jod' (LTS) #60230

Merged

module: link module with a module request record by legendecas · Pull Request #58886 · nodejs/node · GitHub

Uh oh!

Uh oh!

module: link module with a module request record - #58886

module: link module with a module request record#58886nodejs-github-bot merged 1 commit intonodejs:mainnodejs/node:mainfrom legendecas:module-link-requestslegendecas/node:module-link-requestsCopy head branch name to clipboard

Conversation

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

Uh oh!

Uh oh!

nodejs-github-bot commented Jun 29, 2025

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

nodejs-github-bot commented Jun 29, 2025

Uh oh!

aduh95 commented Jun 30, 2025

Uh oh!

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

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guybedford left a comment

Choose a reason for hiding this comment

Uh oh!

guybedford Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

legendecas Jun 30, 2025

Choose a reason for hiding this comment

Uh oh!

joyeecheung Jul 3, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 1, 2025

Uh oh!

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 2, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 3, 2025

Uh oh!

legendecas commented Jul 3, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 3, 2025

Uh oh!

joyeecheung commented Jul 3, 2025

Uh oh!

Uh oh!

nodejs-github-bot commented Jul 3, 2025

Uh oh!

nodejs-github-bot commented Jul 4, 2025

Uh oh!

nodejs-github-bot commented Jul 4, 2025

Uh oh!

aduh95 commented Jul 21, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

module: link module with a module request record#58886
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
legendecas:module-link-requests

legendecas commented Jun 29, 2025 •
edited
Loading

codecov Bot commented Jun 29, 2025 •
edited
Loading

legendecas commented Jun 30, 2025 •
edited
Loading