Skip to content

esm: syncify default path of ModuleLoader.load #57419

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

esm: syncify default path of ModuleLoader.load #57419

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
3a0882a
esm: syncify default path of `ModuleLoader.load`
JakobJingleheimer Mar 9, 2025
d633804
!fixup: don't force `ModuleLoader.load` to be async (can via hooks)
JakobJingleheimer Mar 9, 2025
4276af1
!fixup: remove obsolete `getSource` in favour of `getSourceSync`
JakobJingleheimer Mar 12, 2025
f60650f
!fixup: account for `node:internal` in `isMain` check
JakobJingleheimer Mar 17, 2025
76c93d5
Revert "!fixup: account for `node:internal` in `isMain` check"
JakobJingleheimer Apr 5, 2025
b697946
Reapply "!fixup: account for `node:internal` in `isMain` check"
JakobJingleheimer Apr 8, 2025
8f93801
fixup!: add todo & note about mysterious fix/failure
JakobJingleheimer Apr 8, 2025
558dfff
fixup!: move isInternal check to specific vm resolve path
JakobJingleheimer Apr 9, 2025
d765ed3
fixup!: remove `referrerName === 'node:internal/process/task_queues'`
JakobJingleheimer Apr 22, 2025
7c5787a
module: exclude `node:` prefixed referrer as "parsable url"
JakobJingleheimer Apr 22, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 4 additions & 29 deletions lib/internal/modules/esm/load.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,32 +26,7 @@ const {

/**
* @param {URL} url URL to the module
* @param {ESModuleContext} context used to decorate error messages
* @returns {Promise<{ responseURL: string, source: string | BufferView }>}
*/
async function getSource(url, context) {
const { protocol, href } = url;
const responseURL = href;
let source;
if (protocol === 'file:') {
const { readFile: readFileAsync } = require('internal/fs/promises').exports;
source = await readFileAsync(url);
} else if (protocol === 'data:') {
const result = dataURLProcessor(url);
if (result === 'failure') {
throw new ERR_INVALID_URL(responseURL, null);
}
source = BufferFrom(result.body);
} else {
const supportedSchemes = ['file', 'data'];
throw new ERR_UNSUPPORTED_ESM_URL_SCHEME(url, supportedSchemes);
}
return { __proto__: null, responseURL, source };
}

/**
* @param {URL} url URL to the module
* @param {ESModuleContext} context used to decorate error messages
* @param {LoadContext} context used to decorate error messages
* @returns {{ responseURL: string, source: string | BufferView }}
*/
function getSourceSync(url, context) {
Expand Down Expand Up @@ -80,7 +55,7 @@ function getSourceSync(url, context) {
* @param {LoadContext} context
* @returns {LoadReturn}
*/
async function defaultLoad(url, context = kEmptyObject) {
function defaultLoad(url, context = kEmptyObject) {
let responseURL = url;
let {
importAttributes,
Expand Down Expand Up @@ -110,13 +85,13 @@ async function defaultLoad(url, context = kEmptyObject) {
source = null;
} else if (format !== 'commonjs') {
if (source == null) {
({ responseURL, source } = await getSource(urlInstance, context));
({ responseURL, source } = getSourceSync(urlInstance, context));
context = { __proto__: context, source };
}

if (format == null) {
// Now that we have the source for the module, run `defaultGetFormat` to detect its format.
format = await defaultGetFormat(urlInstance, context);
format = defaultGetFormat(urlInstance, context);

if (format === 'commonjs') {
Copy link
Member

@joyeecheung joyeecheung Apr 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it might happen from the branch here and the "reset source to null and then resolve twice - the second time resolving a full path - if it's commonjs" quirk of the sync loading path. Which might mean that this sync version may not be used as-is without getting rid of that quirk or patched here somehow to not trigger that quirk.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤔 what makes you think it's that quirk? and why is it only exposed when going through that specific VM scenario?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because when this surfaces through the hooks it's documented as a caveat https://nodejs.org/api/module.html#caveat-in-the-asynchronous-load-hook. Internally this happens too, just less observable, I think this only happens in the sync loading, and the vm test isn't expecting this quirk to surface (now that import() switches to use sync loading that hits this quirk). Have you tried actually debugging the module loading to see what happens when the error gets thrown?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have you tried actually debugging the module loading to see what happens when the error gets thrown?

Not yet because I didn't know where to look (there was no stack-trace and I've never touched VM before). But now that I know where it happens, I have a place to start.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's probably less related to vm because USE_MAIN_CONTEXT_DEFAULT_LOADER = use the default loader as if it's being imported from the main context. You could debug the loader paths where ERR_UNSUPPORTED_RESOLVE_REQUEST gets thrown. It doesn't have a stack trace because assert.rejects is unable to capture the async stack trace (I wanted to use normal assert.strictEqual but @aduh95 suggested to use assert.rejects and I accepted his suggestions, maybe it's worth reverting back #51244 (comment) )

Copy link
Member Author

@JakobJingleheimer JakobJingleheimer Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It comes from the shouldBeTreatedAsRelativeOrAbsolutePath branch within moduleResolve:

TypeError [ERR_UNSUPPORTED_RESOLVE_REQUEST]: Failed to resolve module specifier "./message.mjs" from "node:internal/process/task_queues": Invalid relative URL or base scheme is not hierarchical

It's trying to instantiate a URL with 'node:internal/process/task_queues' as the "base".

I think perhaps normalizeReferrerURL is not supposed to recognise 'node:internal/process/task_queues' as a valid URL, in which case referrerName would be voided to undefined. Does that sound right to you?

I added an extra check ( 7c5787a ) within normalizeReferrerURL to exclude node: prefixed referrers, and this test and all the es-module tests pass. If all the tests pass, and we find it acceptable, I think that commit should be separated into its own PR that lands before this one.

Copy link
Member

@joyeecheung joyeecheung Apr 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It sounds like a bug, because internal/process/task_queues doesn't have import() inside, and the referrer should never be a node:-prefixed URL since we don't import() in any of our builtins (that would not work either because we don't implement the host dynamic import callback for builtins). It sounds like whatever that differs in the sync loading from the async one (maybe this "reset to null and then re-resolve + reload" behavior) might alter the request that gets send to resolve that ends up creating an invalid request. You might want to see what is the URL being resolved with that invalid base and debug to find where that invalid request is created under what circumstances and stop it from doing that.

I think if you have to land it with the bug it's better to make the special case completely about this specific URL and add a comment mentioning that the bug exists. Excluding node: completely for this merely enlarges the scope of the bug, because that's supposed to be unreachable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@joyeecheung so for the time-being, are you saying I should switch back to d765ed3? (and update the comment since it's not a mystery anymore)

I think fixing that bug is out of scope of this PR (ex if this got reverted, that would also revert the bugfix)

Copy link
Member

@joyeecheung joyeecheung Apr 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's still a mystery why an unreachable condition is somehow reachable, and is likely a bug caused by reusing a path that's not yet ready/meant to be reused this way without fixups. Another route is to not reuse getSourceSync() and write a more comparable version of synchronous getSource without that differing branch.

// For backward compatibility reasons, we need to discard the source in
Expand Down
4 changes: 2 additions & 2 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -804,9 +804,9 @@ class ModuleLoader {
* if any.
* @param {string} url The URL of the module to be loaded.
* @param {object} context Metadata about the module
* @returns {Promise<{ format: ModuleFormat, source: ModuleSource }>}
* @returns {Promise<{ format: ModuleFormat, source: ModuleSource }> | { format: ModuleFormat, source: ModuleSource }}
*/
async load(url, context) {
load(url, context) {
if (loadHooks.length) {
// Has module.registerHooks() hooks, use the synchronous variant that can handle both hooks.
return this.#loadSync(url, context);
Expand Down
9 changes: 7 additions & 2 deletions lib/internal/modules/helpers.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -263,8 +263,13 @@ function normalizeReferrerURL(referrerName) {
return pathToFileURL(referrerName).href;
}

if (StringPrototypeStartsWith(referrerName, 'file://') ||
URLCanParse(referrerName)) {
if (
StringPrototypeStartsWith(referrerName, 'file://') ||
(
!StringPrototypeStartsWith(referrerName, 'node:') &&
URLCanParse(referrerName)
)
) {
return referrerName;
}

Expand Down
Loading