Skip to content

esm: link modules synchronously when no async loader hooks are used #59519

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

Closed
wants to merge 2 commits into from
Closed

esm: link modules synchronously when no async loader hooks are used #59519

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76a0770
esm: show race error message for inner module job race
joyeecheung Aug 18, 2025
1040990
esm: link modules synchronously when no async loader hooks are used
joyeecheung Aug 18, 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
22 changes: 17 additions & 5 deletions lib/internal/modules/esm/loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,8 @@ let debug = require('internal/util/debuglog').debuglog('esm', (fn) => {
debug = fn;
});

const { isPromise } = require('internal/util/types');

/**
* @typedef {import('./hooks.js').HooksProxy} HooksProxy
* @typedef {import('./module_job.js').ModuleJobBase} ModuleJobBase
Expand Down Expand Up @@ -471,6 +473,10 @@ class ModuleLoader {
const resolvedImportAttributes = resolveResult.importAttributes ?? importAttributes;
let job = this.loadCache.get(url, resolvedImportAttributes.type);
if (job !== undefined) {
// TODO(node:55782): this race may stop happening when the ESM resolution and loading become synchronous.
if (!job.module) {
assert.fail(getRaceMessage(url, parentURL));
}
// This module is being evaluated, which means it's imported in a previous link
// in a cycle.
if (job.module.getStatus() === kEvaluating) {
Expand Down Expand Up @@ -588,15 +594,21 @@ class ModuleLoader {

/**
* Load a module and translate it into a ModuleWrap for ordinary imported ESM.
* This is run asynchronously.
* This may be run asynchronously if there are asynchronous module loader hooks registered.
* @param {string} url URL of the module to be translated.
* @param {object} loadContext See {@link load}
* @param {boolean} isMain Whether the module to be translated is the entry point.
* @returns {Promise<ModuleWrap>}
* @returns {Promise<ModuleWrap>|ModuleWrap}
*/
async loadAndTranslate(url, loadContext, isMain) {
const { format, source } = await this.load(url, loadContext);
return this.#translate(url, format, source, isMain);
loadAndTranslate(url, loadContext, isMain) {
const maybePromise = this.load(url, loadContext);
const afterLoad = ({ format, source }) => {
return this.#translate(url, format, source, isMain);
};
if (isPromise(maybePromise)) {
return maybePromise.then(afterLoad);
}
return afterLoad(maybePromise);
}

/**
Expand Down
10 changes: 5 additions & 5 deletions lib/internal/modules/esm/module_job.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ const {
},
} = internalBinding('util');
const { decorateErrorStack, kEmptyObject } = require('internal/util');
const { isPromise } = require('internal/util/types');
const {
getSourceMapsSupport,
} = require('internal/source_map/source_map_cache');
Expand Down Expand Up @@ -138,12 +139,11 @@ class ModuleJob extends ModuleJobBase {
this.#loader = loader;

// Expose the promise to the ModuleWrap directly for linking below.
if (isForRequireInImportedCJS) {
this.module = moduleOrModulePromise;
assert(this.module instanceof ModuleWrap);
this.modulePromise = PromiseResolve(this.module);
} else {
if (isPromise(moduleOrModulePromise)) {
this.modulePromise = moduleOrModulePromise;
} else {
this.module = moduleOrModulePromise;
this.modulePromise = PromiseResolve(moduleOrModulePromise);
}

if (this.phase === kEvaluationPhase) {
Expand Down
4 changes: 3 additions & 1 deletion test/es-module/test-esm-error-cache.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ let error;
await assert.rejects(
() => import(file),
(e) => {
assert.strictEqual(error, e);
// The module may be compiled again and a new SyntaxError would be thrown but
// with the same content.
assert.deepStrictEqual(error, e);
Copy link
Member Author

@joyeecheung joyeecheung Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This seems fine IMO, because there's no guarantee that the module would not get compiled twice - the real guarantee should be that the module would not get evaluated twice.

return true;
}
);
Expand Down
4 changes: 2 additions & 2 deletions test/es-module/test-esm-import-attributes-errors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ async function test() {

await rejects(
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
Copy link
Member Author

@joyeecheung joyeecheung Aug 18, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This should've been ERR_IMPORT_ATTRIBUTE_UNSUPPORTED in the first place and it also has been ERR_IMPORT_ATTRIBUTE_UNSUPPORTED when this import is evaluated stand-alone in all active versions. The ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE asserted in the test appeared to be a bug that's caused by caching issues when the test retries the all import in one file (i.e. if I comment out other test cases in this file and only leave this one, the test would fail on the main branch). So I think this PR also fixes the bug. The same applies to other similar changes.

);

await rejects(
Expand All @@ -48,7 +48,7 @@ async function test() {

await rejects(
import(jsonModuleDataUrl, { with: { foo: 'bar' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_MISSING' }
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
);

await rejects(
Expand Down
4 changes: 2 additions & 2 deletions test/es-module/test-esm-import-attributes-errors.mjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ await rejects(

await rejects(
import(jsModuleDataUrl, { with: { type: 'json', other: 'unsupported' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_TYPE_INCOMPATIBLE' }
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
);

await rejects(
Expand All @@ -43,7 +43,7 @@ await rejects(

await rejects(
import(jsonModuleDataUrl, { with: { foo: 'bar' } }),
{ code: 'ERR_IMPORT_ATTRIBUTE_MISSING' }
{ code: 'ERR_IMPORT_ATTRIBUTE_UNSUPPORTED' }
);

await rejects(
Expand Down
Loading